Re: [PATCH] ACPICA: Replace one-element array and use struct_size() helper

["Gustavo A. R. Silva" <gustavoars@kernel.org>]

On Tue, May 19, 2020 at 12:25:13PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 19, 2020 at 12:22 AM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > The current codebase makes use of one-element arrays in the following
> > form:
> >
> > 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);
> >
> > 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.
> 
> However, the ACPICA code in the kernel comes from an external project
> and changes of this type are generally not applicable to it unless
> accepted upstream.

Hi Rafael,

By _accepted upstream_, in this case, you mean the adoption of the
flexible-arrays in the whole codebase, first?  If this is the case
notice that there are hundreds of these flexible-array conversions
in mainline, already:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=flexible-array

Is this what you mean?

Thanks
--
Gustavo

> 
> I'll let Erik and Bob take over.
> 
> > Also, make use of the new struct_size() helper to properly calculate the
> > size of struct acpi_pnp_device_id_list.
> >
> > This issue was found with the help of Coccinellea and, audited and
> > fixed _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@kernel.org>
> > ---
> >  drivers/acpi/acpica/utids.c | 4 +---
> >  include/acpi/actypes.h      | 2 +-
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/utids.c b/drivers/acpi/acpica/utids.c
> > index 3bb06935a2ad3..c2f819a39424a 100644
> > --- a/drivers/acpi/acpica/utids.c
> > +++ b/drivers/acpi/acpica/utids.c
> > @@ -262,9 +262,7 @@ acpi_ut_execute_CID(struct acpi_namespace_node *device_node,
> >          * 2) Size of the CID PNP_DEVICE_ID array +
> >          * 3) Size of the actual CID strings
> >          */
> > -       cid_list_size = sizeof(struct acpi_pnp_device_id_list) +
> > -           ((count - 1) * sizeof(struct acpi_pnp_device_id)) +
> > -           string_area_size;
> > +       cid_list_size = struct_size(cid_list, ids, count) + string_area_size;
> >
> >         cid_list = ACPI_ALLOCATE_ZEROED(cid_list_size);
> >         if (!cid_list) {
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > index 4defed58ea338..c7bcda0ad366a 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -1145,7 +1145,7 @@ struct acpi_pnp_device_id {
> >  struct acpi_pnp_device_id_list {
> >         u32 count;              /* Number of IDs in Ids array */
> >         u32 list_size;          /* Size of list, including ID strings */
> > -       struct acpi_pnp_device_id ids[1];       /* ID array */
> > +       struct acpi_pnp_device_id ids[];        /* ID array */
> >  };
> >
> >  /*
> > --
> > 2.26.2
> >