linux-kernel - Re: [PATCH] ACPICA: Replace one-element array and use struct

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 May 2020 17:50:58 -0500
From:   "Gustavo A. R. Silva" <gustavoars@...nel.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Robert Moore <robert.moore@...el.com>,
        Erik Kaneda <erik.kaneda@...el.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Len Brown <lenb@...nel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "open list:ACPI COMPONENT ARCHITECTURE (ACPICA)" <devel@...ica.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] ACPICA: Replace one-element array and use struct_size()
 helper

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@...nel.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@...nel.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
> >