Re: [PATCH] ACPICA: use designated initializers

[Kees Cook]

On Wed, Mar 29, 2017 at 7:12 PM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
> Hi,
>
>> From: keescook@xxxxxxxxxx [mailto:keescook@xxxxxxxxxx] On Behalf Of Kees Cook
>> Subject: Re: [PATCH] ACPICA: use designated initializers
>>
>> On Sun, Dec 18, 2016 at 10:06 PM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
>> > Hi,
>> >
>> >> From: Kees Cook [mailto:keescook@xxxxxxxxxxxx]
>> >> Subject: [PATCH] ACPICA: use designated initializers
>> >>
>> >> Prepare to mark sensitive kernel structures for randomization by making
>> >> sure they're using designated initializers. These were identified during
>> >> allyesconfig builds of x86, arm, and arm64, with most initializer fixes
>> >> extracted from grsecurity.
>> >
>> > This commit is not suitable for ACPICA upstream.
>> > It's not portable. Please drop.
>>
>> What compilers are building this that do not support designated
>> initializers? Also, couldn't this be made into a macro so it could be
>> supported in either case?
>
> It's MSVC.
> In ACPICA upstream, it supports Intel compiler, GCC and MSVC.
>
>>
>> #ifdef __GNUC__
>> # define ACPI_SLEEP_FUNCTIONS(legacy, extended) { \
>>     .legacy_function = legacy, \
>>     .extended_function = extended, \
>> }
>> #else
>> # define ACPI_SLEEP_FUNCTIONS(legacy, extended) { legacy, extended }
>> #endif
>>
>> ...
>>
>>  static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
>>         ACPI_SLEEP_FUNCTIONS(
>>                ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
>>                acpi_hw_extended_sleep),
>> ...
>
> There are many such cases in ACPICA, and I couldn't see the benefit to introduce such mechanism to such a software whose purposes contain portability.
> Unless you can invent a mechanism that can be utilized by all such cases.
> Then you should put it into acgcc.h and implement a replaceable in acmsvc.h.
> After that, you surely need to do a cleanup in the entire ACPICA code base using this new mechanism.

This is only needed in a few cases, so I think general solution would
be overkill. That said, it looks like MSVC supports designated
initializers. Are people building this with especially old compilers?

-Kees

>
> Thanks
> Lv
>
>>
>>
>> -Kees
>>
>> >
>> > Thanks
>> > Lv
>> >
>> >>
>> >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> >> ---
>> >>  drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
>> >>  1 file changed, 6 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
>> >> index f76e0eab32b8..25cd5c66e102 100644
>> >> --- a/drivers/acpi/acpica/hwxfsleep.c
>> >> +++ b/drivers/acpi/acpica/hwxfsleep.c
>> >> @@ -70,11 +70,12 @@ static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
>> >>  /* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
>> >>
>> >>  static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
>> >> -     {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
>> >> -      acpi_hw_extended_sleep},
>> >> -     {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
>> >> -      acpi_hw_extended_wake_prep},
>> >> -     {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
>> >> +     { .legacy_function = ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
>> >> +       .extended_function = acpi_hw_extended_sleep },
>> >> +     { .legacy_function = ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
>> >> +       .extended_function = acpi_hw_extended_wake_prep },
>> >> +     { .legacy_function = ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake),
>> >> +       .extended_function = acpi_hw_extended_wake }
>> >>  };
>> >>
>> >>  /*
>> >> --
>> >> 2.7.4
>> >>
>> >>
>> >> --
>> >> Kees Cook
>> >> Nexus Security
>>
>>
>>
>> --
>> Kees Cook
>> Pixel Security



-- 
Kees Cook
Pixel Security