On Fri, 29 May 2020 at 19:32, Gustavo A. R. Silva <gustavoars@xxxxxxxxxx> wrote: > > On Fri, May 29, 2020 at 01:31:54AM -0700, Kees Cook wrote: > > On Wed, May 27, 2020 at 12:14:25PM -0500, Gustavo A. R. Silva wrote: > > > The current codebase makes use of the zero-length array language > > > extension to the C90 standard, but the preferred mechanism to declare > > > variable-length types such as these ones is a flexible array member[1][2], > > > introduced in C99: > > > > > > struct foo { > > > int stuff; > > > struct boo array[]; > > > }; > > > > > > 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 > > > inadvertently introduced[3] to the codebase from now on. > > > > > > Also, notice that, dynamic memory allocations won't be affected by > > > this change: > > > > > > "Flexible array members have incomplete type, and so the sizeof operator > > > may not be applied. As a quirk of the original implementation of > > > zero-length arrays, sizeof evaluates to zero."[1] > > > > > > sizeof(flexible-array-member) triggers a warning because flexible array > > > members have incomplete type[1]. There are some instances of code in > > > which the sizeof operator is being incorrectly/erroneously applied to > > > zero-length arrays and the result is zero. Such instances may be hiding > > > some bugs. So, this work (flexible-array member conversions) will also > > > help to get completely rid of those sorts of issues. > > > > > > Lastly, make use of the sizeof_field() helper instead of an open-coded > > > version. > > > > > > This issue was found with the help of Coccinelle and audited _manually_. > > > > > > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html > > > [2] https://github.com/KSPP/linux/issues/21 > > > [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour") > > > > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx> > > > > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> > > > > Thanks :) > Queued in efi/urgent, thanks > Please, see more comments below... > > > > --- > > > drivers/firmware/efi/efi.c | 3 ++- > > > include/linux/efi.h | 7 ++----- > > > 2 files changed, 4 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > > > index 7f1657b6c30df..edc5d36caf54e 100644 > > > --- a/drivers/firmware/efi/efi.c > > > +++ b/drivers/firmware/efi/efi.c > > > @@ -622,7 +622,8 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables, > > > rsv = (void *)(p + prsv % PAGE_SIZE); > > > > > > /* reserve the entry itself */ > > > - memblock_reserve(prsv, EFI_MEMRESERVE_SIZE(rsv->size)); > > > + memblock_reserve(prsv, > > > + struct_size(rsv, entry, rsv->size)); > > > > > > for (i = 0; i < atomic_read(&rsv->count); i++) { > > > memblock_reserve(rsv->entry[i].base, > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > index c45ac969ea4eb..328cc52a5fd45 100644 > > > --- a/include/linux/efi.h > > > +++ b/include/linux/efi.h > > > @@ -1234,14 +1234,11 @@ struct linux_efi_memreserve { > > > struct { > > > phys_addr_t base; > > > phys_addr_t size; > > > - } entry[0]; > > > + } entry[]; > > > }; > > > > > > -#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \ > > > - (count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0])) > > > - > > > #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \ > > > - / sizeof(((struct linux_efi_memreserve *)0)->entry[0])) > > > + / sizeof_field(struct linux_efi_memreserve, entry[0])) > > > > Whoa. This is kind of a "reverse struct_size()". I wonder if any other > > places in the kernel do a similar calculation? > > > > So far this is the only intance of this I've run into. > > What I've found is that there are many instances of the open-coded > version of sizeof_field() and offsetof(). I'm addressing them on the > way. > > Thanks > -- > Gustavo