ACPICA: use designated initializers

PATCH In review Kees Cook 4 participants 12 messages Dec 17, 2016 – May 8, 2017

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Kees Cook

author

2016-12-17 01:04 UTC #0 lore raw

PATCH 1/1 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.

Signed-off-by Kees Cook <[email protected]>

---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/acpi/acpica/hwxfsleep.c +6 -5

index f76e0eab32b8..25cd5c66e102 100644
@@ -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 }
 };
 
 /*

Signature · 6 lines

2.7.4


-- 
Kees Cook
Nexus Security

Rafael J. Wysocki

maintainer

2016-12-17 02:17 UTC #1 lore raw

Quoted 23 lines · Kees Cook

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.
Signed-off-by Kees Cook <[email protected]>
---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/acpi/acpica/hwxfsleep.c +0 -5
index f76e0eab32b8..25cd5c66e102 100644
@@ -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}

drivers/acpi/acpica/hwxfsleep.c static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);

Quoted 10 lines

drivers/acpi/acpica/hwxfsleep.c +6 -1

+       { .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 }
 };
 
 /*
--

The problem here is like in the other patch sent earlier today. If we change that here, it will diverge from the upstream file it has been generated from.

Thanks,
Rafael

Zheng, Lv

2016-12-19 06:06 UTC #2 lore raw

In reply to Kees Cook

Hi,

Quoted 7 lines

From: Kees Cook [mailto:keescook(a)chromium.org]
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.

Thanks
Lv

Quoted 35 lines

Signed-off-by Kees Cook <keescook(a)chromium.org>

---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/acpi/acpica/hwxfsleep.c +6 -6

index f76e0eab32b8..25cd5c66e102 100644
@@ -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

author

2017-03-29 21:16 UTC #3 lore raw

Quoted 5 lines · Lv

Hi,
Quoted 3 lines
From: Kees Cook [mailto:[email protected]]
Subject: [PATCH] ACPICA: use designated initializers

Quoted 7 lines

Quoted 4 lines
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?

#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),

...

-Kees

Quoted 39 lines

Thanks
Lv

Quoted 35 lines

Signed-off-by Kees Cook <[email protected]>

---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/acpi/acpica/hwxfsleep.c +6 -6

index f76e0eab32b8..25cd5c66e102 100644
@@ -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

Signature · 2 lines

Kees Cook
Pixel Security

Zheng, Lv

2017-03-30 02:12 UTC #4 lore raw

Hi,

Quoted 14 lines

From: keescook(a)google.com [mailto:keescook(a)google.com] On Behalf Of Kees Cook
Subject: Re: [PATCH] ACPICA: use designated initializers
Quoted 10 lines · Lv
Hi,
Quoted 7 lines
From: Kees Cook [mailto:keescook(a)chromium.org]
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 10 lines

#ifdef __GNUC__

# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { \
    .legacy_function = legacy, \
    .extended_function = extended, \
}
#else
# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { legacy, extended }
#endif

Quoted 7 lines

...

 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.

Thanks
Lv

Quoted 49 lines

-Kees

Quoted 31 lines

Thanks
Lv

Quoted 27 lines

Signed-off-by Kees Cook <keescook(a)chromium.org>

---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/acpi/acpica/hwxfsleep.c +6 -5

index f76e0eab32b8..25cd5c66e102 100644
@@ -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 }
 };

Quoted 8 lines

Quoted 8 lines
drivers/acpi/acpica/hwxfsleep.c +0 -1
 /*
--
2.7.4
--
Kees Cook
Nexus Security

--
Kees Cook
Pixel Security

Moore, Robert

2017-03-30 14:26 UTC #5 lore raw

Yes, ACPICA must support many compilers, so we keep the code as simple as possible.

Many of the corporate OS vendors use their own compilers.

Quoted 124 lines

-----Original Message-----
From: Zheng, Lv
Sent: Wednesday, March 29, 2017 7:12 PM
To: Kees Cook <keescook(a)chromium.org>
Cc: linux-kernel(a)vger.kernel.org; Moore, Robert <robert.moore(a)intel.com>; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; Len Brown <lenb(a)kernel.org>; linux- acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: RE: [PATCH] ACPICA: use designated initializers
Hi,
Quoted 5 lines
From: keescook(a)google.com [mailto:keescook(a)google.com] 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(a)intel.com>
Quoted 10 lines · wrote:
Quoted 10 lines
Hi,
Quoted 7 lines
From: Kees Cook [mailto:keescook(a)chromium.org]
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 10 lines
#ifdef __GNUC__
# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { \
    .legacy_function = legacy, \
    .extended_function = extended, \
}
#else
# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { legacy, extended }
#endif
Quoted 6 lines
...
 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.
Thanks
Lv
Quoted 24 lines
-Kees
Quoted 20 lines
Thanks
Lv
Quoted 16 lines
Signed-off-by Kees Cook <keescook(a)chromium.org>
---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
(quoted diff) +0 -0
b/drivers/acpi/acpica/hwxfsleep.c index f76e0eab32b8..25cd5c66e102 100644
drivers/acpi/acpica/hwxfsleep.c +0 -0
@@ -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
 */
 
Quoted 6 lines
Quoted 6 lines
drivers/acpi/acpica/hwxfsleep.c static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
Quoted 6 lines
drivers/acpi/acpica/hwxfsleep.c +0 -5
 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}
Quoted 1 line
Quoted 1 line
drivers/acpi/acpica/hwxfsleep.c static acpi_status acpi_hw_sleep_dispatch(u8 · line 75
Quoted 1 line
drivers/acpi/acpica/hwxfsleep.c +1 -0
+     { .legacy_function =
ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
Quoted 2 lines
Quoted 2 lines
drivers/acpi/acpica/hwxfsleep.c static acpi_status acpi_hw_sleep_dispatch(u8 · line 77
Quoted 2 lines
drivers/acpi/acpica/hwxfsleep.c +2 -0
+       .extended_function = acpi_hw_extended_sleep },
+     { .legacy_function =
ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
Quoted 2 lines
Quoted 2 lines
drivers/acpi/acpica/hwxfsleep.c static acpi_status acpi_hw_sleep_dispatch(u8 · line 79
Quoted 2 lines
drivers/acpi/acpica/hwxfsleep.c +2 -0
+       .extended_function = acpi_hw_extended_wake_prep },
+     { .legacy_function =
ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake),
Quoted 17 lines
Quoted 11 lines
Quoted 11 lines
drivers/acpi/acpica/hwxfsleep.c +1 -0
+       .extended_function = acpi_hw_extended_wake }
 };
/*
--
2.7.4
--
Kees Cook
Nexus Security
--
Kees Cook
Pixel Security

Kees Cook

author

2017-04-01 00:26 UTC #6 lore raw

In reply to Zheng, Lv

Quoted 44 lines · Lv

Hi,
Quoted 14 lines
From: [email protected] [mailto:[email protected]] On Behalf Of Kees Cook
Subject: Re: [PATCH] ACPICA: use designated initializers
Quoted 10 lines · Lv
Hi,
Quoted 7 lines
From: Kees Cook [mailto:[email protected]]
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 10 lines
#ifdef __GNUC__
# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { \
    .legacy_function = legacy, \
    .extended_function = extended, \
}
#else
# define ACPI_SLEEP_FUNCTIONS(legacy, extended) { legacy, extended }
#endif
Quoted 7 lines
...
 static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
        ACPI_SLEEP_FUNCTIONS(
               ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
               acpi_hw_extended_sleep),
...

Quoted 4 lines

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

Quoted 53 lines

Thanks
Lv

Quoted 49 lines

-Kees

Quoted 31 lines

Thanks
Lv

Quoted 27 lines

Signed-off-by Kees Cook <[email protected]>

---
 drivers/acpi/acpica/hwxfsleep.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/acpi/acpica/hwxfsleep.c +6 -5

index f76e0eab32b8..25cd5c66e102 100644
@@ -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 }
 };

Quoted 8 lines

Quoted 8 lines
drivers/acpi/acpica/hwxfsleep.c +0 -1
 /*
--
2.7.4
--
Kees Cook
Nexus Security

--
Kees Cook
Pixel Security

Signature · 2 lines

Kees Cook
Pixel Security

Moore, Robert

2017-04-01 00:45 UTC #7 lore raw

Acpica is built with many compilers, even very old ones. It runs on at least 12 known operating systems, and very probably more.

I'm sorry, but no, we are not going to start adding compiler-specific ifdefs/code in the base ACPICA code.

I don't care what you do in the Linux-specific or gcc-specific headers, however. If this breaks a customer build, we (you) will hear about it rather quickly.

Bob

Kees Cook

author

2017-04-03 17:29 UTC #8 lore raw

Quoted 5 lines · Robert

Acpica is built with many compilers, even very old ones. It runs on at least 12 known operating systems, and very probably more.
I'm sorry, but no, we are not going to start adding compiler-specific ifdefs/code in the base ACPICA code.
I don't care what you do in the Linux-specific or gcc-specific headers, however. If this breaks a customer build, we (you) will hear about it rather quickly.

Since the change is specific to the one place ACPICA uses an all-function-pointer structure, I made the change local:

https://github.com/acpica/acpica/pull/248

would you rather this is in the .h files instead?

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Moore, Robert

2017-04-04 15:02 UTC #9 lore raw

Quoted 27 lines

-----Original Message-----
From: keescook(a)google.com [mailto:keescook(a)google.com] On Behalf Of Kees Cook
Sent: Monday, April 3, 2017 10:29 AM
To: Moore, Robert <robert.moore(a)intel.com>
Cc: Zheng, Lv <lv.zheng(a)intel.com>; linux-kernel(a)vger.kernel.org; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; Len Brown <lenb(a)kernel.org>; linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA: use designated initializers
Quoted 1 line · Robert
Acpica is built with many compilers, even very old ones. It runs on at
least 12 known operating systems, and very probably more.
Quoted 2 lines
I'm sorry, but no, we are not going to start adding compiler-specific
ifdefs/code in the base ACPICA code.
Quoted 2 lines
I don't care what you do in the Linux-specific or gcc-specific
headers, however. If this breaks a customer build, we (you) will hear about it rather quickly.
Since the change is specific to the one place ACPICA uses an all- function-pointer structure, I made the change local:
https://github.com/acpica/acpica/pull/248

Quoted 7 lines

would you rather this is in the .h files instead?
-Kees
--
Kees Cook
Pixel Security

[Moore, Robert]

I have some questions about this entire issue:

+ * Some compilers can handle designated initializers, which is needed
+ * under Linux kernel builds for structures that are entirely function
* pointers.

I don't understand why this is coming up now, since ACPICA has been integrated with Linux for something like the last 15 years. It's the "which is needed under Linux kernel builds" wording that concerns me the most. Are you saying that the ACPICA build for Linux is broken and does not work?

Further, there are quite a few similar dispatch tables in ACPICA, why are these not a problem?

Bob

Kees Cook

author

2017-04-04 15:55 UTC #10 lore raw

Quoted 40 lines · Robert

Quoted 27 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kees Cook
Sent: Monday, April 3, 2017 10:29 AM
To: Moore, Robert <[email protected]>
Cc: Zheng, Lv <[email protected]>; [email protected]; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH] ACPICA: use designated initializers
Quoted 1 line · Robert
Acpica is built with many compilers, even very old ones. It runs on at
least 12 known operating systems, and very probably more.
Quoted 2 lines
I'm sorry, but no, we are not going to start adding compiler-specific
ifdefs/code in the base ACPICA code.
Quoted 2 lines
I don't care what you do in the Linux-specific or gcc-specific
headers, however. If this breaks a customer build, we (you) will hear about it rather quickly.
Since the change is specific to the one place ACPICA uses an all- function-pointer structure, I made the change local:
https://github.com/acpica/acpica/pull/248
Quoted 7 lines
would you rather this is in the .h files instead?
-Kees
--
Kees Cook
Pixel Security
[Moore, Robert]

Quoted 7 lines

I have some questions about this entire issue:
+ * Some compilers can handle designated initializers, which is needed
+ * under Linux kernel builds for structures that are entirely function
* pointers.
I don't understand why this is coming up now, since ACPICA has been integrated with Linux for something like the last 15 years. It's the "which is needed under Linux kernel builds" wording that concerns me the most. Are you saying that the ACPICA build for Linux is broken and does not work?

I was trying to avoid being overly Linux-specific in the ACPICA commit message. More accurately, this is "for future Linux builds using the structure layout randomization plugin." That plugin will randomize the layout of manually marked structures and automatically for structures that are entirely function pointers. (And this acpica structure is one noticed by the plugin.)

Quoted 1 line

Further, there are quite a few similar dispatch tables in ACPICA, why are these not a problem?

I can double-check, but I think this was the only one that showed up in an x86 allyesconfig with the plugin enabled.

FWIW, I've been making these changes in lots of places, not just ACPICA. ACPICA just has external requirements. :P

$ git log --oneline v4.9..next-20170404 | grep -i "designated initializers"
b3c829193253 reiserfs: use designated initializers
8291798dcf05 TOMOYO: Use designated initializers
f231aebfc4ca rbtree: use designated initializers
c4d27f4b4dc9 [media] solo6x10: use designated initializers
6351db2b4df3 [media] mtk-vcodec: use designated initializers
613e61a0252c drm/amdgpu: use designated initializers
8486adf0d755 apparmor: use designated initializers
7f6856b789ff RDMA/i40iw: use designated initializers
6554c9f7f749 RDMA/nes: use designated initializers
a641261e9998 video: fbdev: matroxfb: use designated initializers
6895aff47170 video: fbdev: sh_mobile_lcdcfb: use designated initializers
54e22bbf11ca staging: comedi: daqboard2000: use designated initializers
5f5fca6db3d3 scsi: cciss: use designated initializers
93380123fbb5 scsi: hpsa: use designated initializers
2fd2434c3f47 staging: lustre: ldlm: use designated initializers
f93a1c9e5e6a ALSA: synth: use designated initializers
2fa70bb9b564 drm/nouveau: use designated initializers
5ca16d8efa66 drm/vmwgfx: use designated initializers
4e98c378a137 drm/ttm: use designated initializers
c92f72370571 drm/ttm: use designated initializers
ffc7dc8d838c x86/floppy: Use designated initializers
e999cb43d51f net/x25: use designated initializers
ebf12f1320c7 isdn: use designated initializers
9751362a4fe7 bna: use designated initializers
aabd7ad94924 WAN: use designated initializers
9d1c0ca5e1d6 net: use designated initializers
99a5e178bde4 ATM: use designated initializers
4794195058b9 isdn/gigaset: use designated initializers

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

author

2017-05-08 20:05 UTC #11 lore raw

Quoted 47 lines · Kees Cook

Quoted 46 lines · Robert
Quoted 27 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kees Cook
Sent: Monday, April 3, 2017 10:29 AM
To: Moore, Robert <[email protected]>
Cc: Zheng, Lv <[email protected]>; [email protected]; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH] ACPICA: use designated initializers
Quoted 1 line · Robert
Acpica is built with many compilers, even very old ones. It runs on at
least 12 known operating systems, and very probably more.
Quoted 2 lines
I'm sorry, but no, we are not going to start adding compiler-specific
ifdefs/code in the base ACPICA code.
Quoted 2 lines
I don't care what you do in the Linux-specific or gcc-specific
headers, however. If this breaks a customer build, we (you) will hear about it rather quickly.
Since the change is specific to the one place ACPICA uses an all- function-pointer structure, I made the change local:
https://github.com/acpica/acpica/pull/248
Quoted 7 lines
would you rather this is in the .h files instead?
-Kees
--
Kees Cook
Pixel Security
[Moore, Robert]
I have some questions about this entire issue:
+ * Some compilers can handle designated initializers, which is needed
+ * under Linux kernel builds for structures that are entirely function
* pointers.

Quoted 8 lines

Quoted 1 line
I don't understand why this is coming up now, since ACPICA has been integrated with Linux for something like the last 15 years. It's the "which is needed under Linux kernel builds" wording that concerns me the most. Are you saying that the ACPICA build for Linux is broken and does not work?
I was trying to avoid being overly Linux-specific in the ACPICA commit message. More accurately, this is "for future Linux builds using the structure layout randomization plugin." That plugin will randomize the layout of manually marked structures and automatically for structures that are entirely function pointers. (And this acpica structure is one noticed by the plugin.)

Here is the merge request in upstream ACPICA. Can anyone re-review it?

https://github.com/acpica/acpica/pull/248/

Thanks!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security