linux-kernel - Re: [PATCH] acpi: zero-initialize acpi

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <lbffqd6wuigivpd4ryrxwirhkc5ghj6k4nenm75dtgeea42535@7v2fzt5i7app>
Date: Thu, 24 Oct 2024 14:58:27 -0700
From: Payam Moradshahi <payamm@...gle.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] acpi: zero-initialize acpi_object union structure

Hi Rafael,

Thank you for your response. Please see below for my response.

Payam

On Mon, Oct 21, 2024 at 01:31:39PM GMT, Rafael J. Wysocki wrote:
> On Thu, Oct 17, 2024 at 5:59 AM Payam Moradshahi <payamm@...gle.com>  
> wrote:
> >
> > The way in which acpi_object union is being initialized varies based on
> > compiler type, version and flags used. Some will zero-initialize the
> > entire union structure and some will only initialize the first N-bytes
> > of the union structure.

> Any details?
I dumped acpi_object after initialization and noticed either the
entire structure was zero-initialized or just the first 8 bytes
depending on compiler type and version.

gcc 13.2.0: bad
CLANG_CL=362121269: good
CLANG_CL=609443126: bad
CLANG_CL=684773960: good


> > This could lead to uninitialized union members.

> So this is working around a compiler bug AFAICS.

> If the compiler has this bug, is it guaranteed to compile the rest of
> the kernel correctly?
This is not a compiler bug. The way acpi_object union is being
initialized is not c-spec compliant.

Based on C Standard ISO/IEC 9899:202x (E):

Section 6.2.6.1 (7) says:

When a value is stored in a member of an object of union type, the bytes of  
the
object representation that do not correspond to that member but do  
correspond
to other members take unspecified values

Section 6.7.9 says:

If an object that has automatic storage duration is not initialized  
explicitly,
its value is indeterminate.

If an object that has static or thread storage duration is not initialized
explicitly, then:
  - if it is a union, the first named member is initialized (recursively)
    according to these rules, and any padding is initialized to zero bits;

The above guarantees zero only in the case of static or thread storage,
which is not the case here.


> > This bug was confirmed by observing non-zero value for object->processor
> > structure variables.

> Where has it been observed?  What compiler version(s)? etc.
See above for some examples

> > non-zero initialized members of acpi_object union structure causes
> > incorrect error reporting by the driver.
> >
> > If a BIOS is using "Device" statement as opposed to "Processor"
> > statement, object variable may contain uninitialized members causing the
> > driver to report "Invalid PBLK length" incorrectly.
> >
> > Using memset to zero-initialize the union structure fixes this issue and
> > also removes the dependency of this function on compiler versions and
> > flags being used.
> >
> > Tested: Tested on ARM64 hardware that was printing this error and
> > confirmed the prints were gone.
> >
> > Also confirmed this does not cause regression on ARM64 and X86
> > machines.
> >
> > Signed-off-by: Payam Moradshahi <payamm@...gle.com>
> > ---
> >  drivers/acpi/acpi_processor.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpi_processor.c  
> b/drivers/acpi/acpi_processor.c
> > index 7cf6101cb4c73..6696ad4937d21 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -275,7 +275,7 @@ static inline int acpi_processor_hotadd_init(struct  
> acpi_processor *pr,
> >
> >  static int acpi_processor_get_info(struct acpi_device *device)
> >  {
> > -       union acpi_object object = { 0 };
> > +       union acpi_object object;
> >         struct acpi_buffer buffer = { sizeof(union acpi_object),  
> &object };
> >         struct acpi_processor *pr = acpi_driver_data(device);
> >         int device_declaration = 0;
> > @@ -284,6 +284,8 @@ static int acpi_processor_get_info(struct  
> acpi_device *device)
> >         unsigned long long value;
> >         int ret;
> >
> > +       memset(&object, 0, sizeof(union acpi_object));
> > +
> >         acpi_processor_errata();
> >
> >         /*
> > --