| Message ID | 20200518222722.GA7791@embeddedor (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ACPICA: Replace one-element array and use struct_size() helper | expand |
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. 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 >
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 > >
On Wed, May 20, 2020 at 12:46 AM Gustavo A. R. Silva <gustavoars@kernel.org> wrote: > > 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? I meant whether or not the patch is accepted by the ACPICA upstream. > 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? I'm not actually sure what you mean here.
On Wed, May 20, 2020 at 11:15:18AM +0200, Rafael J. Wysocki wrote: > On Wed, May 20, 2020 at 12:46 AM Gustavo A. R. Silva > <gustavoars@kernel.org> wrote: > > > > 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? > > I meant whether or not the patch is accepted by the ACPICA upstream. Is that here? https://github.com/acpica/acpica/commits/master > > > 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? > > I'm not actually sure what you mean here. I think this was just a misunderstanding about what "upstream" meant. :) I hope ACPICA will take these changes -- it seems like we keep running into these issues with the kernel's language feature clean-ups and ACPICA upstream, though each have been resolved so far! :) Flexible array members are a C99 feature, so it's hardly a new way to express things. In fact, it looks like ACPICA already builds with -c99 by default: https://github.com/acpica/acpica/blob/master/generate/unix/Makefile.config#L202 https://github.com/acpica/acpica/blob/master/generate/efi/Makefile.config#L93 MSVC has supported them (called "unsized arrays") since 7.1 in 2003. Gustavo, can you build a merge request for the ACPICA project directly?
> -----Original Message----- > From: linux-acpi-owner@vger.kernel.org <linux-acpi- > owner@vger.kernel.org> On Behalf Of Kees Cook > Sent: Wednesday, May 20, 2020 8:41 AM > To: Rafael J. Wysocki <rafael@kernel.org> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>; Moore, Robert > <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>; Wysocki, > Rafael J <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; ACPI > Devel Maling List <linux-acpi@vger.kernel.org>; open list:ACPI COMPONENT > ARCHITECTURE (ACPICA) <devel@acpica.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; Gustavo A. R. Silva > <gustavo@embeddedor.com> > Subject: Re: [PATCH] ACPICA: Replace one-element array and use > struct_size() helper > > On Wed, May 20, 2020 at 11:15:18AM +0200, Rafael J. Wysocki wrote: > > On Wed, May 20, 2020 at 12:46 AM Gustavo A. R. Silva > > <gustavoars@kernel.org> wrote: > > > > > > 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? > > > > I meant whether or not the patch is accepted by the ACPICA upstream. > > Is that here? https://github.com/acpica/acpica/commits/master > > > > > > 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/l > > > og/?qt=grep&q=flexible-array > > > > > > Is this what you mean? > > > > I'm not actually sure what you mean here. > > I think this was just a misunderstanding about what "upstream" meant. :) > > I hope ACPICA will take these changes -- it seems like we keep running into > these issues with the kernel's language feature clean-ups and ACPICA > upstream, though each have been resolved so far! :) Flexible array members > are a C99 feature, so it's hardly a new way to express things. > In fact, it looks like ACPICA already builds with -c99 by default: > https://github.com/acpica/acpica/blob/master/generate/unix/Makefile.conf > ig#L202 > https://github.com/acpica/acpica/blob/master/generate/efi/Makefile.config > #L93 > > MSVC has supported them (called "unsized arrays") since 7.1 in 2003. > > Gustavo, can you build a merge request for the ACPICA project directly? The flexible array members will be fine. The struct_size helper uses the typeof operator which is a GCC extension. ACPICA codebase is intended to be compiled by many different compilers (not just GCC and MSVC) so we cannot support struct_size. Erik > > -- > Kees Cook
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 */ }; /*
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. 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(-)