linux-kernel - Re: [PATCH] char: hpet: Use flexible-array member

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200123185609.GA2075008@kroah.com>
Date:   Thu, 23 Jan 2020 19:56:09 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     Clemens Ladisch <clemens@...isch.de>,
        Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] char: hpet: Use flexible-array member

On Thu, Jan 23, 2020 at 12:46:42PM -0600, Gustavo A. R. Silva wrote:
> 
> 
> On 1/23/20 12:25, Greg Kroah-Hartman wrote:
> > On Mon, Jan 20, 2020 at 05:53:26PM -0600, Gustavo A. R. Silva wrote:
> >> Old code in the kernel uses 1-byte and 0-byte arrays to indicate the
> >> presence of a "variable length array":
> >>
> >> struct something {
> >>     int length;
> >>     u8 data[1];
> >> };
> >>
> >> struct something *instance;
> >>
> >> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> >> instance->length = size;
> >> memcpy(instance->data, source, size);
> >>
> >> There is also 0-byte arrays. Both cases pose confusion for things like
> >> sizeof(), CONFIG_FORTIFY_SOURCE, etc.[1] Instead, the preferred mechanism
> >> to declare variable-length types such as the one above is a flexible array
> >> member[2] which need to be the last member of a structure and empty-sized:
> >>
> >> struct something {
> >>         int stuff;
> >>         u8 data[];
> >> };
> >>
> >> Also, by making use of the mechanism above, we will get a compiler warning
> >> in case the flexible array does not occur last in the structure, which
> >> will help us prevent some kind of undefined behavior bugs from being
> >> unadvertenly introduced[3] to the codebase from now on.
> >>
> >> [1] https://github.com/KSPP/linux/issues/21
> >> [2] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> >>
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.com>
> >> ---
> >>  drivers/char/hpet.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> >> index 9ac6671bb514..aed2c45f7968 100644
> >> --- a/drivers/char/hpet.c
> >> +++ b/drivers/char/hpet.c
> >> @@ -110,7 +110,7 @@ struct hpets {
> >>  	unsigned long hp_delta;
> >>  	unsigned int hp_ntimer;
> >>  	unsigned int hp_which;
> >> -	struct hpet_dev hp_dev[1];
> >> +	struct hpet_dev hp_dev[];
> > 
> > Are you sure the allocation size is the same again?  Much like the
> > n_hdlc patch was, I think you need to adjust the variable size here.
> > Maybe, it's a bit of a pain to figure out at a quick glance, I just want
> > to make sure you at least do look at that :)
> > 
> 
> Yep. The allocation thing was already handled almost a year ago by the
> following patch, and it didn't require to increase the size at that time:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=401c9bd10beef4b030eb9e34d16b5341dc6c683b

Great, thanks for verifying, I'll go queue this up now.

greg k-h