Replace fake flexible arrays with actual flexible array members #813
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
OK, but I’m thinking that using these “new” structures will require changes that affect both the Data Table Compiler and the Data Table disassembler.
Bob
From: Kees Cook ***@***.***>
Sent: Thursday, November 17, 2022 8:56 PM
To: acpica/acpica ***@***.***>
Cc: Subscribed ***@***.***>
Subject: [acpica/acpica] Replace fake flexible arrays with actual flexible array members (PR #813)
This continues earlier C99 work to modernize the code to work correctly with compile-time and run-time bounds checkers.
…________________________________
You can view, comment on, or merge this pull request online at:
#813
Commit Summary
* 84851f6<84851f6> ACPI_RESOURCE_EXTENDED_IRQ: Replace 1-element array with flexible array
* c892aca<c892aca> ACPI_PCI_ROUTING_TABLE: Replace fixed-size array with flex array member
File Changes
(2 files<https://github.com/acpica/acpica/pull/813/files>)
* M source/include/acrestyp.h<https://github.com/acpica/acpica/pull/813/files#diff-0d6df2e054e0561eede97cac47d0cd3e005f62038a5fcc4f999b38ef4de9b694> (8)
* M source/include/platform/acenv.h<https://github.com/acpica/acpica/pull/813/files#diff-4d7183c49a06f644e22fd6921bbbc0282a4c641c7ca60b1d410031f843ac890c> (11)
Patch Links:
* https://github.com/acpica/acpica/pull/813.patch
* https://github.com/acpica/acpica/pull/813.diff
—
Reply to this email directly, view it on GitHub<#813>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHL7LB6DO4TBAOYOVPTE4LWI4D4NANCNFSM6AAAAAASEC7KJE>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.******@***.***>>
|
Add helper to work around the pointless C99 requirement that flex array members not be allowed in unions (nor alone in structs), which is not actually a real-world problem.
Use ACPI_FLEX_ARRAY() helper to define flexible array member alone in a struct. Fixes issue acpica#812.
Similar to commit 7ba2f3d ("Replace one-element array with flexible-array"), replace the 1-element array with a proper flexible array member as defined by C99. This allows the code to operate without tripping compile-time and run-time bounds checkers (e.g. via __builtin_object_size(), -fsanitize=bounds, and/or -fstrict-flex-arrays=3).
The "Source" array is actually a dynamically sized array, but it is defined as a fixed-size 4 byte array. This results in tripping both compile-time and run-time bounds checkers (e.g. via either __builtin_object_size() or -fsanitize=bounds). To retain the padding, create a union with an unused Pad variable of size 4, and redefine Source as a proper flexible array member.
I don't know what this means. :) How can I find/test these? |
|
It depends on the actual table(s) that are affected.
For examples, see compiler/dttable*.c, compiler/dtcompile.c, common/dmtbinfo*.c, and common/dmtbdump*.c
From: Kees Cook ***@***.***>
Sent: Friday, November 18, 2022 9:53 AM
To: acpica/acpica ***@***.***>
Cc: Moore, Robert ***@***.***>; Comment ***@***.***>
Subject: Re: [acpica/acpica] Replace fake flexible arrays with actual flexible array members (PR #813)
OK, but I’m thinking that using these “new” structures will require changes that affect both the Data Table Compiler and the Data Table disassembler.
I don't know what this means. :) How can I find/test these?
—
Reply to this email directly, view it on GitHub<#813 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHL7LG7QGBSO4DDBANDSJTWI67AFANCNFSM6AAAAAASEC7KJE>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
|
@acpibob What if this change is made locally in Linux? I don't think it will become unmanageable going forward. |
rafaeljw
approved these changes
Jan 18, 2023
roxell
pushed a commit
to roxell/linux
that referenced
this pull request
Jan 19, 2023
Functionally identical to ACPICA upstream pull request 813: acpica/acpica#813 One-element arrays (and multi-element arrays being treated as dynamically sized) are deprecated[1] and are being replaced with flexible array members in support of the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3. Replace one-element array with flexible-array member in struct acpi_resource_extended_irq. Replace 4-byte fixed-size array with 4-byte padding in a union with a flexible-array member in struct acpi_pci_routing_table. This results in no differences in binary output. Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Cc: Robert Moore <robert.moore@intel.com> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> Cc: linux-acpi@vger.kernel.org Link: https://lore.kernel.org/all/20221118181538.never.225-kees@kernel.org/ Signed-off-by: Kees Cook <keescook@chromium.org>
intel-lab-lkp
pushed a commit
to intel-lab-lkp/linux
that referenced
this pull request
Jan 20, 2023
Functionally identical to ACPICA upstream pull request 813: acpica/acpica#813 One-element arrays (and multi-element arrays being treated as dynamically sized) are deprecated[1] and are being replaced with flexible array members in support of the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3. Replace one-element array with flexible-array member in struct acpi_resource_extended_irq. Replace 4-byte fixed-size array with 4-byte padding in a union with a flexible-array member in struct acpi_pci_routing_table. This results in no differences in binary output. Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Cc: Robert Moore <robert.moore@intel.com> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> Cc: linux-acpi@vger.kernel.org Link: https://lore.kernel.org/all/20221118181538.never.225-kees@kernel.org/ Signed-off-by: Kees Cook <keescook@chromium.org>
intel-lab-lkp
pushed a commit
to intel-lab-lkp/linux
that referenced
this pull request
Jan 28, 2023
One-element arrays (and multi-element arrays being treated as dynamically sized) are deprecated[1] and are being replaced with flexible array members in support of the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3. Replace one-element array with flexible-array member in struct acpi_resource_extended_irq. Replace 4-byte fixed-size array with 4-byte padding in a union with a flexible-array member in struct acpi_pci_routing_table. This results in no differences in binary output. Link: acpica/acpica#813 Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
jrtc27
reviewed
Jan 31, 2023
| */ | ||
| #define ACPI_FLEX_ARRAY(TYPE, NAME) \ | ||
| struct { \ | ||
| struct { } __Empty_ ## NAME; \ |
There was a problem hiding this comment.
Empty structs are a GNU extension
| * limitation. | ||
| */ | ||
| #define ACPI_FLEX_ARRAY(TYPE, NAME) \ | ||
| struct { \ |
There was a problem hiding this comment.
Anonymous structs require C11 (or a compiler that adds them as an extension to pre-C11)
| @@ -1550,7 +1550,7 @@ enum AcpiMadtLpcPicVersion { | |||
|
|
|||
| typedef struct acpi_madt_oem_data | |||
| { | |||
| UINT8 OemData[]; | |||
| ACPI_FLEX_ARRAY(UINT8, OemData); | |||
There was a problem hiding this comment.
Structs with flexible array members can't be nested inside other structs, only unions, without extensions
There was a problem hiding this comment.
Yes, this works around a deficiency in the C spec using GNU extensions which have been supported by GCC and Clang for a very long time.
There was a problem hiding this comment.
I realise that, but you're just swapping one extension for another which doesn't achieve anything as far as I can tell other than adding extra complication.
If your goal is to avoid extensions then just typedef UINT8 ACPI_MADT_OEM_DATA[], which is an API break but just requires changing Expr->OemData to (*Expr).
|
OK, I'll try this PR as it stands. |
This was referenced Feb 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This continues earlier C99 work to modernize the code to work correctly with compile-time and run-time bounds checkers.