LinuxLists.cc - arm64/acpi: NULL dereference reports from UBSAN at boot

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

Hi Lorenzo, Hanjun, [+Nick]

On Thu, May 21, 2020 at 06:37:38PM +0100, Lorenzo Pieralisi wrote:
> On Thu, May 21, 2020 at 11:09:53AM +0100, Will Deacon wrote:
> > Hi folks,
> >
> > I just tried booting the arm64 for-kernelci branch under QEMU (version
> > 4.2.50 (v4.2.0-779-g4354edb6dcc7)) with UBSAN enabled, and I see a
> > couple of NULL pointer dereferences reported at boot. I think they're
> > both GIC related (log below). I don't see a panic with UBSAN disabled,
> > so something's fishy here.
>
> May I ask you the QEMU command line please - just to make sure I can
> replicate it.

As it turns out, I'm only able to reproduce this when building with Clang,
but I don't know whether that's because GCC is missing something of Clang
is signalling a false positive. You also don't need all of those whacky
fuzzing options enabled.

Anyway, to reproduce:

$ git checkout for-next/kernelci
$ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- defconfig
<then do a menuconfig and enable UBSAN>
$ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- Image

I throw that at QEMU using:

qemu-system-aarch64 -M virt -machine virtualization=true \
-machine virt,gic-version=3 \
-cpu max,sve=off -smp 2 -m 4096 \
-drive if=pflash,format=raw,file=efi.img,readonly \
-drive if=pflash,format=raw,file=varstore.img \
-drive if=virtio,format=raw,file=disk.img \
-device virtio-scsi-pci,id=scsi0 \
-device virtio-rng-pci \
-device virtio-net-pci,netdev=net0 \
-netdev user,id=net0,hostfwd=tcp::8222-:22 \
-nographic \
-kernel ~/work/linux/arch/arm64/boot/Image \
-append "earlycon root=/dev/vda2"

I built QEMU a while ago according to:

https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html

and its version 4.2.50 (v4.2.0-779-g4354edb6dcc7).

My clang is version 11.0.1.

Will

2020-05-27 17:55:38

by Lorenzo Pieralisi

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Tue, May 26, 2020 at 09:21:57PM +0100, Will Deacon wrote:
> Hi Lorenzo, Hanjun, [+Nick]
>
> On Thu, May 21, 2020 at 06:37:38PM +0100, Lorenzo Pieralisi wrote:
> > On Thu, May 21, 2020 at 11:09:53AM +0100, Will Deacon wrote:
> > > Hi folks,
> > >
> > > I just tried booting the arm64 for-kernelci branch under QEMU (version
> > > 4.2.50 (v4.2.0-779-g4354edb6dcc7)) with UBSAN enabled, and I see a
> > > couple of NULL pointer dereferences reported at boot. I think they're
> > > both GIC related (log below). I don't see a panic with UBSAN disabled,
> > > so something's fishy here.
> >
> > May I ask you the QEMU command line please - just to make sure I can
> > replicate it.
>
> As it turns out, I'm only able to reproduce this when building with Clang,
> but I don't know whether that's because GCC is missing something of Clang
> is signalling a false positive. You also don't need all of those whacky
> fuzzing options enabled.
>
> Anyway, to reproduce:
>
> $ git checkout for-next/kernelci
> $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- defconfig
> <then do a menuconfig and enable UBSAN>
> $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- Image
>
> I throw that at QEMU using:
>
> qemu-system-aarch64 -M virt -machine virtualization=true \
> -machine virt,gic-version=3 \
> -cpu max,sve=off -smp 2 -m 4096 \
> -drive if=pflash,format=raw,file=efi.img,readonly \
> -drive if=pflash,format=raw,file=varstore.img \
> -drive if=virtio,format=raw,file=disk.img \
> -device virtio-scsi-pci,id=scsi0 \
> -device virtio-rng-pci \
> -device virtio-net-pci,netdev=net0 \
> -netdev user,id=net0,hostfwd=tcp::8222-:22 \
> -nographic \
> -kernel ~/work/linux/arch/arm64/boot/Image \
> -append "earlycon root=/dev/vda2"
>
> I built QEMU a while ago according to:
>
> https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html
>
> and its version 4.2.50 (v4.2.0-779-g4354edb6dcc7).
>
> My clang is version 11.0.1.

Thanks a lot Will.

I *think* I was right - it is the ACPI_OFFSET() macro:

#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)

that triggers the warnings (I suspected it because at least in one of
the warnings I could not see any dereference of any dynamically
allocated data).

Now on what to do with it - thoughts welcome.

Lorenzo

2020-06-01 07:09:22

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Wed, May 27, 2020 at 02:41:04PM +0100, Lorenzo Pieralisi wrote:
> On Tue, May 26, 2020 at 09:21:57PM +0100, Will Deacon wrote:
> > Hi Lorenzo, Hanjun, [+Nick]
> >
> > On Thu, May 21, 2020 at 06:37:38PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, May 21, 2020 at 11:09:53AM +0100, Will Deacon wrote:
> > > > Hi folks,
> > > >
> > > > I just tried booting the arm64 for-kernelci branch under QEMU (version
> > > > 4.2.50 (v4.2.0-779-g4354edb6dcc7)) with UBSAN enabled, and I see a
> > > > couple of NULL pointer dereferences reported at boot. I think they're
> > > > both GIC related (log below). I don't see a panic with UBSAN disabled,
> > > > so something's fishy here.
> > >
> > > May I ask you the QEMU command line please - just to make sure I can
> > > replicate it.
> >
> > As it turns out, I'm only able to reproduce this when building with Clang,
> > but I don't know whether that's because GCC is missing something of Clang
> > is signalling a false positive. You also don't need all of those whacky
> > fuzzing options enabled.
> >
> > Anyway, to reproduce:
> >
> > $ git checkout for-next/kernelci
> > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- defconfig
> > <then do a menuconfig and enable UBSAN>
> > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- Image
> >
> > I throw that at QEMU using:
> >
> > qemu-system-aarch64 -M virt -machine virtualization=true \
> > -machine virt,gic-version=3 \
> > -cpu max,sve=off -smp 2 -m 4096 \
> > -drive if=pflash,format=raw,file=efi.img,readonly \
> > -drive if=pflash,format=raw,file=varstore.img \
> > -drive if=virtio,format=raw,file=disk.img \
> > -device virtio-scsi-pci,id=scsi0 \
> > -device virtio-rng-pci \
> > -device virtio-net-pci,netdev=net0 \
> > -netdev user,id=net0,hostfwd=tcp::8222-:22 \
> > -nographic \
> > -kernel ~/work/linux/arch/arm64/boot/Image \
> > -append "earlycon root=/dev/vda2"
> >
> > I built QEMU a while ago according to:
> >
> > https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html
> >
> > and its version 4.2.50 (v4.2.0-779-g4354edb6dcc7).
> >
> > My clang is version 11.0.1.
>
> Thanks a lot Will.
>
> I *think* I was right - it is the ACPI_OFFSET() macro:
>
> #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
>
> that triggers the warnings (I suspected it because at least in one of
> the warnings I could not see any dereference of any dynamically
> allocated data).

Cheers, Lorenzo.

> Now on what to do with it - thoughts welcome.

Nick -- any idea what to do about the above? The '#define' pasted by
Lorenzo is causing a couple of spurious UBSAN splats when compiling with
clang 11.

Cheers,

Will

2020-06-01 21:55:07

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Mon, Jun 1, 2020 at 12:05 AM Will Deacon <[email protected]> wrote:
>
> On Wed, May 27, 2020 at 02:41:04PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, May 26, 2020 at 09:21:57PM +0100, Will Deacon wrote:
> > > Hi Lorenzo, Hanjun, [+Nick]
> > >
> > > On Thu, May 21, 2020 at 06:37:38PM +0100, Lorenzo Pieralisi wrote:
> > > > On Thu, May 21, 2020 at 11:09:53AM +0100, Will Deacon wrote:
> > > > > Hi folks,
> > > > >
> > > > > I just tried booting the arm64 for-kernelci branch under QEMU (version
> > > > > 4.2.50 (v4.2.0-779-g4354edb6dcc7)) with UBSAN enabled, and I see a
> > > > > couple of NULL pointer dereferences reported at boot. I think they're
> > > > > both GIC related (log below). I don't see a panic with UBSAN disabled,
> > > > > so something's fishy here.
> > > >
> > > > May I ask you the QEMU command line please - just to make sure I can
> > > > replicate it.
> > >
> > > As it turns out, I'm only able to reproduce this when building with Clang,
> > > but I don't know whether that's because GCC is missing something of Clang
> > > is signalling a false positive. You also don't need all of those whacky
> > > fuzzing options enabled.
> > >
> > > Anyway, to reproduce:
> > >
> > > $ git checkout for-next/kernelci
> > > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- defconfig
> > > <then do a menuconfig and enable UBSAN>
> > > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- Image
> > >
> > > I throw that at QEMU using:
> > >
> > > qemu-system-aarch64 -M virt -machine virtualization=true \
> > > -machine virt,gic-version=3 \
> > > -cpu max,sve=off -smp 2 -m 4096 \
> > > -drive if=pflash,format=raw,file=efi.img,readonly \
> > > -drive if=pflash,format=raw,file=varstore.img \
> > > -drive if=virtio,format=raw,file=disk.img \
> > > -device virtio-scsi-pci,id=scsi0 \
> > > -device virtio-rng-pci \
> > > -device virtio-net-pci,netdev=net0 \
> > > -netdev user,id=net0,hostfwd=tcp::8222-:22 \
> > > -nographic \
> > > -kernel ~/work/linux/arch/arm64/boot/Image \
> > > -append "earlycon root=/dev/vda2"
> > >
> > > I built QEMU a while ago according to:
> > >
> > > https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html
> > >
> > > and its version 4.2.50 (v4.2.0-779-g4354edb6dcc7).
> > >
> > > My clang is version 11.0.1.
> >
> > Thanks a lot Will.
> >
> > I *think* I was right - it is the ACPI_OFFSET() macro:
> >
> > #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> >
> > that triggers the warnings (I suspected it because at least in one of
> > the warnings I could not see any dereference of any dynamically
> > allocated data).
>
> Cheers, Lorenzo.
>
> > Now on what to do with it - thoughts welcome.
>
> Nick -- any idea what to do about the above? The '#define' pasted by
> Lorenzo is causing a couple of spurious UBSAN splats when compiling with
> clang 11.

If there's undefined behavior from that macro soup, we should be able
to reproduce it outside of the kernel and regardless of target
architecture, right? The macros aren't too much to throw into a file:

```foo.c
#define acpi_uintptr_t void *
#define ACPI_CAST_PTR(t, p) ((t *) (acpi_uintptr_t) (p))
typedef unsigned char u8;
typedef unsigned long u64;
typedef u64 acpi_size;
#define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) -
ACPI_CAST_PTR (u8, (b))))
#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)

struct foo {
unsigned char bar;
int baz;
};

int main() {
return ACPI_OFFSET(struct foo, baz);
}
```
I think looks right. If we run that through -E, and clean that up
further, we get:
```bar.c
typedef unsigned char u8;
typedef unsigned long u64;

struct foo {
unsigned char bar;
int baz;
};

int main() {
return ((u64) (((u8 *) (void *) ((&(((struct foo *) 0)->baz)))) - ((u8
*) (void *) (((void *) 0)))));
}
```
I may be miscounting my parentheses, but how do you take the address
of `type`->`member`? What does that even mean?

+ some more sanitizer folks and Ard for ACPI.

anyways, running foo.c through a compiler:
$ clang -O2 foo.c -fsanitize=undefined
$ ./a.out
foo.c:15:12: runtime error: member access within null pointer of type
'struct foo'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior foo.c:15:12 in

(msg looks truncated, wtf)

Anyways, it looks like the address of member from NULL subexpression
looks problematic. I wonder if offsetof can be used here?

#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (offsetof(d, f), (void *) 0)

Seems to work in my basic test case. Untested in the kernel.

IIUC, ACPI_OFFSET is trying to calculate the difference between the
offset of a member of a struct and 0? Isn't that the tautology `x - 0
== x`?
--
Thanks,
~Nick Desaulniers

2020-06-01 22:01:54

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <[email protected]> wrote:
>
> On Mon, Jun 1, 2020 at 12:05 AM Will Deacon <[email protected]> wrote:
> >
> > On Wed, May 27, 2020 at 02:41:04PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, May 26, 2020 at 09:21:57PM +0100, Will Deacon wrote:
> > > > Hi Lorenzo, Hanjun, [+Nick]
> > > >
> > > > On Thu, May 21, 2020 at 06:37:38PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Thu, May 21, 2020 at 11:09:53AM +0100, Will Deacon wrote:
> > > > > > Hi folks,
> > > > > >
> > > > > > I just tried booting the arm64 for-kernelci branch under QEMU (version
> > > > > > 4.2.50 (v4.2.0-779-g4354edb6dcc7)) with UBSAN enabled, and I see a
> > > > > > couple of NULL pointer dereferences reported at boot. I think they're
> > > > > > both GIC related (log below). I don't see a panic with UBSAN disabled,
> > > > > > so something's fishy here.
> > > > >
> > > > > May I ask you the QEMU command line please - just to make sure I can
> > > > > replicate it.
> > > >
> > > > As it turns out, I'm only able to reproduce this when building with Clang,
> > > > but I don't know whether that's because GCC is missing something of Clang
> > > > is signalling a false positive. You also don't need all of those whacky
> > > > fuzzing options enabled.
> > > >
> > > > Anyway, to reproduce:
> > > >
> > > > $ git checkout for-next/kernelci
> > > > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- defconfig
> > > > <then do a menuconfig and enable UBSAN>
> > > > $ make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- Image
> > > >
> > > > I throw that at QEMU using:
> > > >
> > > > qemu-system-aarch64 -M virt -machine virtualization=true \
> > > > -machine virt,gic-version=3 \
> > > > -cpu max,sve=off -smp 2 -m 4096 \
> > > > -drive if=pflash,format=raw,file=efi.img,readonly \
> > > > -drive if=pflash,format=raw,file=varstore.img \
> > > > -drive if=virtio,format=raw,file=disk.img \
> > > > -device virtio-scsi-pci,id=scsi0 \
> > > > -device virtio-rng-pci \
> > > > -device virtio-net-pci,netdev=net0 \
> > > > -netdev user,id=net0,hostfwd=tcp::8222-:22 \
> > > > -nographic \
> > > > -kernel ~/work/linux/arch/arm64/boot/Image \
> > > > -append "earlycon root=/dev/vda2"
> > > >
> > > > I built QEMU a while ago according to:
> > > >
> > > > https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html
> > > >
> > > > and its version 4.2.50 (v4.2.0-779-g4354edb6dcc7).
> > > >
> > > > My clang is version 11.0.1.
> > >
> > > Thanks a lot Will.
> > >
> > > I *think* I was right - it is the ACPI_OFFSET() macro:
> > >
> > > #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> > >
> > > that triggers the warnings (I suspected it because at least in one of
> > > the warnings I could not see any dereference of any dynamically
> > > allocated data).
> >
> > Cheers, Lorenzo.
> >
> > > Now on what to do with it - thoughts welcome.
> >
> > Nick -- any idea what to do about the above? The '#define' pasted by
> > Lorenzo is causing a couple of spurious UBSAN splats when compiling with
> > clang 11.
>
> If there's undefined behavior from that macro soup, we should be able
> to reproduce it outside of the kernel and regardless of target
> architecture, right? The macros aren't too much to throw into a file:
>
> ```foo.c
> #define acpi_uintptr_t void *
> #define ACPI_CAST_PTR(t, p) ((t *) (acpi_uintptr_t) (p))
> typedef unsigned char u8;
> typedef unsigned long u64;
> typedef u64 acpi_size;
> #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) -
> ACPI_CAST_PTR (u8, (b))))
> #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
>
> struct foo {
> unsigned char bar;
> int baz;
> };
>
> int main() {
> return ACPI_OFFSET(struct foo, baz);
> }
> ```
> I think looks right. If we run that through -E, and clean that up
> further, we get:
> ```bar.c
> typedef unsigned char u8;
> typedef unsigned long u64;
>
> struct foo {
> unsigned char bar;
> int baz;
> };
>
> int main() {
> return ((u64) (((u8 *) (void *) ((&(((struct foo *) 0)->baz)))) - ((u8
> *) (void *) (((void *) 0)))));
> }
> ```
> I may be miscounting my parentheses, but how do you take the address
> of `type`->`member`? What does that even mean?
>
> + some more sanitizer folks and Ard for ACPI.
>
> anyways, running foo.c through a compiler:
> $ clang -O2 foo.c -fsanitize=undefined
> $ ./a.out
> foo.c:15:12: runtime error: member access within null pointer of type
> 'struct foo'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior foo.c:15:12 in
>
> (msg looks truncated, wtf)
>
> Anyways, it looks like the address of member from NULL subexpression
> looks problematic. I wonder if offsetof can be used here?
>
> #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (offsetof(d, f), (void *) 0)
>
> Seems to work in my basic test case. Untested in the kernel.
>
> IIUC, ACPI_OFFSET is trying to calculate the difference between the
> offset of a member of a struct and 0? Isn't that the tautology `x - 0
> == x`?

No. ACPI_OFFSET() is just a poor person's version of offsetof().

(Note that it calculates the difference between &(((d *) 0)->f) and
(void *)0x0, so the 0x0 term is there on both sides)

2020-06-01 22:23:49

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Mon, Jun 1, 2020 at 2:57 PM Ard Biesheuvel <[email protected]> wrote:
>
> On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <[email protected]> wrote:
> >
> > Anyways, it looks like the address of member from NULL subexpression
> > looks problematic. I wonder if offsetof can be used here?
> >
> > #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (offsetof(d, f), (void *) 0)
> >
> > Seems to work in my basic test case. Untested in the kernel.
> >
> > IIUC, ACPI_OFFSET is trying to calculate the difference between the
> > offset of a member of a struct and 0? Isn't that the tautology `x - 0
> > == x`?
>
> No. ACPI_OFFSET() is just a poor person's version of offsetof().
>
> (Note that it calculates the difference between &(((d *) 0)->f) and
> (void *)0x0, so the 0x0 term is there on both sides)

Got it. So we're trying to avoid including stddef.h? Can
__builtin_offsetof be used here?
#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (__builtin_offsetof(d, f), (void *) 0)
The oldest version of GCC in godbolt.org (4.1) supports this builtin.
--
Thanks,
~Nick Desaulniers

2020-06-01 22:31:01

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: arm64/acpi: NULL dereference reports from UBSAN at boot

On Tue, 2 Jun 2020 at 00:19, Nick Desaulniers <[email protected]> wrote:
>
> On Mon, Jun 1, 2020 at 2:57 PM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <[email protected]> wrote:
> > >
> > > Anyways, it looks like the address of member from NULL subexpression
> > > looks problematic. I wonder if offsetof can be used here?
> > >
> > > #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (offsetof(d, f), (void *) 0)
> > >
> > > Seems to work in my basic test case. Untested in the kernel.
> > >
> > > IIUC, ACPI_OFFSET is trying to calculate the difference between the
> > > offset of a member of a struct and 0? Isn't that the tautology `x - 0
> > > == x`?
> >
> > No. ACPI_OFFSET() is just a poor person's version of offsetof().
> >
> > (Note that it calculates the difference between &(((d *) 0)->f) and
> > (void *)0x0, so the 0x0 term is there on both sides)
>
> Got it. So we're trying to avoid including stddef.h? Can
> __builtin_offsetof be used here?
> #define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (__builtin_offsetof(d, f), (void *) 0)

Drop the 0x0:

#define ACPI_OFFSET __builtin_offsetof

should be all we need.

> The oldest version of GCC in godbolt.org (4.1) supports this builtin.

Yeah I think that should be fine.

Alternatively, using any arbitrary address other than 0x0 on both
sides should work as well to get rid of the undefined behavior
(assuming the use of NULL pointers is what is causing it), but I don't
see why we need to invent our own helper here.

BTW some other macros looks dodgy as well.
761f0b82393353507930b6721ae4311a9df2ca36 provides a nice set of
candidates to go and clean up.

2020-06-01 23:20:32

[permalink] [raw]

Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

Will reported UBSAN warnings:
UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6

Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
can avoid this by using the compiler builtin, __builtin_offsetof.

The non-kernel runtime of UBSAN would print:
runtime error: member access within null pointer of type
for this macro.

Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
Cc: [email protected]
Reported-by: Will Deacon <[email protected]>
Suggested-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
include/acpi/actypes.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 4defed58ea33..04359c70b198 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -508,7 +508,7 @@ typedef u64 acpi_integer;

#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
-#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
+#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
#define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
#define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

--
2.27.0.rc2.251.g90737beb825-goog

2020-06-01 23:41:36

by Peter Collingbourne

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers <[email protected]> wrote:
>
> Will reported UBSAN warnings:
> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
>
> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> can avoid this by using the compiler builtin, __builtin_offsetof.

Would it be better to s/ACPI_OFFSET/offsetof/g the existing users of
this macro and remove it? It looks like offsetof is already being used
pervasively in the kernel, and its definition comes from
<linux/stddef.h>.

Peter

Peter

> The non-kernel runtime of UBSAN would print:
> runtime error: member access within null pointer of type
> for this macro.
>
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: [email protected]
> Reported-by: Will Deacon <[email protected]>
> Suggested-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> include/acpi/actypes.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index 4defed58ea33..04359c70b198 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>
> --
> 2.27.0.rc2.251.g90737beb825-goog
>

2020-06-01 23:51:15

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Mon, Jun 1, 2020 at 4:37 PM Peter Collingbourne <[email protected]> wrote:
>
> On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers <[email protected]> wrote:
> >
> > Will reported UBSAN warnings:
> > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> >
> > Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> > can avoid this by using the compiler builtin, __builtin_offsetof.
>
> Would it be better to s/ACPI_OFFSET/offsetof/g the existing users of
> this macro and remove it? It looks like offsetof is already being used
> pervasively in the kernel, and its definition comes from
> <linux/stddef.h>.

I count only 9 uses in the tree, so not too bad a yak shave. Good
idea; I'll send tomorrow short of any other feedback. I still think
we want the builtin, since we don't want to include stddef.h in the
kernel, I think.
--
Thanks,
~Nick Desaulniers

2020-06-02 00:06:05

[permalink] [raw]

Subject: RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

> -----Original Message-----
> From: Nick Desaulniers <[email protected]>
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert <[email protected]>; Kaneda, Erik
> <[email protected]>; Wysocki, Rafael J <[email protected]>;
> Len Brown <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>
Hi,

> Will reported UBSAN warnings:
> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
>
> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> can avoid this by using the compiler builtin, __builtin_offsetof.

I'll take a look at this tomorrow
>
> The non-kernel runtime of UBSAN would print:
> runtime error: member access within null pointer of type for this macro.

actypes.h is owned by ACPICA so we typically do not allow compiler-specific
extensions because the code is intended to be compiled using the C99 standard
without compiler extensions. We could allow this sort of thing in a Linux-specific
header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..

Erik
>
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: [email protected]
> Reported-by: Will Deacon <[email protected]>
> Suggested-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> include/acpi/actypes.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> 4defed58ea33..04359c70b198 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
> 0)
> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>
> --
> 2.27.0.rc2.251.g90737beb825-goog

2020-06-02 18:49:23

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <[email protected]> wrote:
>
>
> Hi,
>
> > Will reported UBSAN warnings:
> > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> >
> > Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> > can avoid this by using the compiler builtin, __builtin_offsetof.
>
> I'll take a look at this tomorrow
> >
> > The non-kernel runtime of UBSAN would print:
> > runtime error: member access within null pointer of type for this macro.
>
> actypes.h is owned by ACPICA so we typically do not allow compiler-specific
> extensions because the code is intended to be compiled using the C99 standard
> without compiler extensions. We could allow this sort of thing in a Linux-specific
> header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..

If I'm not allowed to touch that header, it looks like I can include
<linux/stddef.h> (rather than my host's <stddef.h>) to get a
definition of `offsetof` thats implemented in terms of
`__builtin_offsetof`. I should be able to use that to replace uses of
ACPI_OFFSET. Are any of these off limits?

$ grep -rn ACPI_OFFSET
arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH
ACPI_OFFSET( \
arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE
(ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
include/acpi/actbl.h:376:#define ACPI_FADT_OFFSET(f) (u16)
ACPI_OFFSET (struct acpi_table_fadt, f)
drivers/acpi/acpica/acresrc.h:84:#define ACPI_RS_OFFSET(f)
(u8) ACPI_OFFSET (struct acpi_resource,f)
drivers/acpi/acpica/acresrc.h:85:#define AML_OFFSET(f)
(u8) ACPI_OFFSET (union aml_resource,f)
drivers/acpi/acpica/acinterp.h:17:#define ACPI_EXD_OFFSET(f)
(u8) ACPI_OFFSET (union acpi_operand_object,f)
drivers/acpi/acpica/acinterp.h:18:#define ACPI_EXD_NSOFFSET(f)
(u8) ACPI_OFFSET (struct acpi_namespace_node,f)
drivers/acpi/acpica/rsdumpinfo.c:16:#define ACPI_RSD_OFFSET(f)
(u8) ACPI_OFFSET (union acpi_resource_data,f)
drivers/acpi/acpica/rsdumpinfo.c:17:#define ACPI_PRT_OFFSET(f)
(u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)

--
Thanks,
~Nick Desaulniers

2020-06-08 14:54:34

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

Hey Nick,

On Tue, Jun 02, 2020 at 11:46:31AM -0700, Nick Desaulniers wrote:
> On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <[email protected]> wrote:
> > > Will reported UBSAN warnings:
> > > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> > >
> > > Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> > > can avoid this by using the compiler builtin, __builtin_offsetof.
> >
> > I'll take a look at this tomorrow
> > >
> > > The non-kernel runtime of UBSAN would print:
> > > runtime error: member access within null pointer of type for this macro.
> >
> > actypes.h is owned by ACPICA so we typically do not allow compiler-specific
> > extensions because the code is intended to be compiled using the C99 standard
> > without compiler extensions. We could allow this sort of thing in a Linux-specific
> > header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
>
> If I'm not allowed to touch that header, it looks like I can include
> <linux/stddef.h> (rather than my host's <stddef.h>) to get a
> definition of `offsetof` thats implemented in terms of
> `__builtin_offsetof`. I should be able to use that to replace uses of
> ACPI_OFFSET. Are any of these off limits?

It's not so much about not being allowed to touch the header, but rather
that the kernel imports the code from a different project:

https://acpica.org/community

> $ grep -rn ACPI_OFFSET
> arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH
> ACPI_OFFSET( \
> arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE
> (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \

I'm happy to take patches to the stuff under arch/arm64/, fwiw.

Will

2020-06-08 20:33:32

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Mon, Jun 8, 2020 at 7:51 AM Will Deacon <[email protected]> wrote:
>
> Hey Nick,
>
> On Tue, Jun 02, 2020 at 11:46:31AM -0700, Nick Desaulniers wrote:
> > On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <[email protected]> wrote:
> > > > Will reported UBSAN warnings:
> > > > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > > > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> > > >
> > > > Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> > > > can avoid this by using the compiler builtin, __builtin_offsetof.
> > >
> > > I'll take a look at this tomorrow
> > > >
> > > > The non-kernel runtime of UBSAN would print:
> > > > runtime error: member access within null pointer of type for this macro.
> > >
> > > actypes.h is owned by ACPICA so we typically do not allow compiler-specific
> > > extensions because the code is intended to be compiled using the C99 standard
> > > without compiler extensions. We could allow this sort of thing in a Linux-specific
> > > header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
> >
> > If I'm not allowed to touch that header, it looks like I can include
> > <linux/stddef.h> (rather than my host's <stddef.h>) to get a
> > definition of `offsetof` thats implemented in terms of
> > `__builtin_offsetof`. I should be able to use that to replace uses of
> > ACPI_OFFSET. Are any of these off limits?
>
> It's not so much about not being allowed to touch the header, but rather
> that the kernel imports the code from a different project:
>
> https://acpica.org/community
>
> > $ grep -rn ACPI_OFFSET
> > arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH
> > ACPI_OFFSET( \
> > arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE
> > (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
>
> I'm happy to take patches to the stuff under arch/arm64/, fwiw.

Not really sure how to untangle this. Those two cases under
arch/arm64/ are straightforward to fix:
```
diff --git a/arch/arm64/include/asm/acpi.h
b/arch/arm64/include/asm/acpi.h
index b263e239cb59..a45366c3909b 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,6 +12,7 @@
#include <linux/efi.h>
#include <linux/memblock.h>
#include <linux/psci.h>
+#include <linux/stddef.h>

#include <asm/cputype.h>
#include <asm/io.h>
@@ -31,14 +32,14 @@
* is therefore used to delimit the MADT GICC structure minimum length
* appropriately.
*/
-#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
+#define ACPI_MADT_GICC_MIN_LENGTH offsetof( \
struct acpi_madt_generic_interrupt, efficiency_class)

#define BAD_MADT_GICC_ENTRY(entry, end)
\
(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
(unsigned long)(entry) + (entry)->header.length > (end))

-#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
+#define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \
spe_interrupt) + sizeof(u16))

/* Basic configuration for ACPI */
```

But for one of the warnings you reported, as an example:
UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37

```
$ ag ACPI_FADT_V2_SIZE
include/acpi/actbl.h
394:#define ACPI_FADT_V2_SIZE (u32) (ACPI_FADT_OFFSET
(minor_revision) + 1)

drivers/acpi/acpica/tbfadt.c
459: if (acpi_gbl_FADT.header.length <= ACPI_FADT_V2_SIZE) {

$ ag ACPI_FADT_OFFSET
...
include/acpi/actbl.h
376:#define ACPI_FADT_OFFSET(f) (u16) ACPI_OFFSET (struct
acpi_table_fadt, f)
...
```
So the use of ACPI_FADT_V2_SIZE in drivers/acpi/acpica/tbfadt.c is
triggering one of the warnings. ACPI_FADT_V2_SIZE is defined in terms
of ACPI_FADT_OFFSET which is defined in terms of ACPI_OFFSET in
include/acpi/actbl.h. From the link you posted, include/acpi/actbl.h
is from the project under source/include/.

Further, drivers/acpi/acpica/tbfadt.c seems to also be from the
upstream project under source/components/tables/tbfadt.c.

Regardless, the second of the two warnings is definitely fixed by my
above diff, so let me rephrase the previous commit message with that
diff and resend.
--
Thanks,
~Nick Desaulniers

2020-06-08 20:42:45

[permalink] [raw]

Subject: [PATCH v2] arm64: acpi: fix UBSAN warning

Will reported a UBSAN warning:

UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
member access within null pointer of type 'struct acpi_madt_generic_interrupt'
CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
Call trace:
dump_backtrace+0x0/0x384
show_stack+0x28/0x38
dump_stack+0xec/0x174
handle_null_ptr_deref+0x134/0x174
__ubsan_handle_type_mismatch_v1+0x84/0xa4
acpi_parse_gic_cpu_interface+0x60/0xe8
acpi_parse_entries_array+0x288/0x498
acpi_table_parse_entries_array+0x178/0x1b4
acpi_table_parse_madt+0xa4/0x110
acpi_parse_and_init_cpus+0x38/0x100
smp_init_cpus+0x74/0x258
setup_arch+0x350/0x3ec
start_kernel+0x98/0x6f4

This is from the use of the ACPI_OFFSET in
arch/arm64/include/asm/acpi.h. Replace its use with offsetof from
include/linux/stddef.h which should implement the same logic using
__builtin_offsetof, so that UBSAN wont warn.

Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
Cc: [email protected]
Reported-by: Will Deacon <[email protected]>
Suggested-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
Changes V1 -> V2:
* Just fix one of the two warnings, specific to arm64.
* Put warning in commit message.

arch/arm64/include/asm/acpi.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index b263e239cb59..a45366c3909b 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,6 +12,7 @@
#include <linux/efi.h>
#include <linux/memblock.h>
#include <linux/psci.h>
+#include <linux/stddef.h>

#include <asm/cputype.h>
#include <asm/io.h>
@@ -31,14 +32,14 @@
* is therefore used to delimit the MADT GICC structure minimum length
* appropriately.
*/
-#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
+#define ACPI_MADT_GICC_MIN_LENGTH offsetof( \
struct acpi_madt_generic_interrupt, efficiency_class)

#define BAD_MADT_GICC_ENTRY(entry, end) \
(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
(unsigned long)(entry) + (entry)->header.length > (end))

-#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
+#define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \
spe_interrupt) + sizeof(u16))

/* Basic configuration for ACPI */
--
2.27.0.278.ge193c7cf3a9-goog

2020-06-09 00:10:21

[permalink] [raw]

Subject: RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

> -----Original Message-----
> From: Nick Desaulniers <[email protected]>
> Sent: Tuesday, June 2, 2020 11:47 AM
> To: Kaneda, Erik <[email protected]>
> Cc: Moore, Robert <[email protected]>; Wysocki, Rafael J
> <[email protected]>; Len Brown <[email protected]>; Ard
> Biesheuvel <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>
> On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <[email protected]>
> wrote:
> >
> >
> > Hi,
> >
> > > Will reported UBSAN warnings:
> > > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> > >
> > > Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
> > > We can avoid this by using the compiler builtin, __builtin_offsetof.
> >
> > I'll take a look at this tomorrow
> > >
> > > The non-kernel runtime of UBSAN would print:
> > > runtime error: member access within null pointer of type for this macro.
> >
> > actypes.h is owned by ACPICA so we typically do not allow
> > compiler-specific extensions because the code is intended to be
> > compiled using the C99 standard without compiler extensions. We could
> > allow this sort of thing in a Linux-specific header file like
> include/acpi/platform/aclinux.h but I'll take a look at the error as well..
>
Hi,

> If I'm not allowed to touch that header, it looks like I can include
> <linux/stddef.h> (rather than my host's <stddef.h>) to get a definition of

Why not use your host's stddef.h?

> `offsetof` thats implemented in terms of `__builtin_offsetof`. I should be
> able to use that to replace uses of ACPI_OFFSET. Are any of these off limits?

Yes, the idea is to define ACPI_OFFSET in a way that you want so that we don't touch the uses below.

Erik
>
> $ grep -rn ACPI_OFFSET
> arch/arm64/include/asm/acpi.h:34:#define
> ACPI_MADT_GICC_MIN_LENGTH
> ACPI_OFFSET( \ arch/arm64/include/asm/acpi.h:41:#define
> ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
> include/acpi/actbl.h:376:#define ACPI_FADT_OFFSET(f) (u16)
> ACPI_OFFSET (struct acpi_table_fadt, f)
> drivers/acpi/acpica/acresrc.h:84:#define ACPI_RS_OFFSET(f)
> (u8) ACPI_OFFSET (struct acpi_resource,f)
> drivers/acpi/acpica/acresrc.h:85:#define AML_OFFSET(f)
> (u8) ACPI_OFFSET (union aml_resource,f)
> drivers/acpi/acpica/acinterp.h:17:#define ACPI_EXD_OFFSET(f)
> (u8) ACPI_OFFSET (union acpi_operand_object,f)
> drivers/acpi/acpica/acinterp.h:18:#define ACPI_EXD_NSOFFSET(f)
> (u8) ACPI_OFFSET (struct acpi_namespace_node,f)
> drivers/acpi/acpica/rsdumpinfo.c:16:#define ACPI_RSD_OFFSET(f)
> (u8) ACPI_OFFSET (union acpi_resource_data,f)
> drivers/acpi/acpica/rsdumpinfo.c:17:#define ACPI_PRT_OFFSET(f)
> (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
>
> --
> Thanks,
> ~Nick Desaulniers

2020-06-09 20:33:03

by Lorenzo Pieralisi

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: acpi: fix UBSAN warning

On Mon, Jun 08, 2020 at 01:38:17PM -0700, Nick Desaulniers wrote:
> Will reported a UBSAN warning:
>
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> member access within null pointer of type 'struct acpi_madt_generic_interrupt'
> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
> Call trace:
> dump_backtrace+0x0/0x384
> show_stack+0x28/0x38
> dump_stack+0xec/0x174
> handle_null_ptr_deref+0x134/0x174
> __ubsan_handle_type_mismatch_v1+0x84/0xa4
> acpi_parse_gic_cpu_interface+0x60/0xe8
> acpi_parse_entries_array+0x288/0x498
> acpi_table_parse_entries_array+0x178/0x1b4
> acpi_table_parse_madt+0xa4/0x110
> acpi_parse_and_init_cpus+0x38/0x100
> smp_init_cpus+0x74/0x258
> setup_arch+0x350/0x3ec
> start_kernel+0x98/0x6f4
>
> This is from the use of the ACPI_OFFSET in
> arch/arm64/include/asm/acpi.h. Replace its use with offsetof from
> include/linux/stddef.h which should implement the same logic using
> __builtin_offsetof, so that UBSAN wont warn.
>
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: [email protected]
> Reported-by: Will Deacon <[email protected]>
> Suggested-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> Changes V1 -> V2:
> * Just fix one of the two warnings, specific to arm64.
> * Put warning in commit message.
>
> arch/arm64/include/asm/acpi.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)

Acked-by: Lorenzo Pieralisi <[email protected]>

> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index b263e239cb59..a45366c3909b 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -12,6 +12,7 @@
> #include <linux/efi.h>
> #include <linux/memblock.h>
> #include <linux/psci.h>
> +#include <linux/stddef.h>
>
> #include <asm/cputype.h>
> #include <asm/io.h>
> @@ -31,14 +32,14 @@
> * is therefore used to delimit the MADT GICC structure minimum length
> * appropriately.
> */
> -#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
> +#define ACPI_MADT_GICC_MIN_LENGTH offsetof( \
> struct acpi_madt_generic_interrupt, efficiency_class)
>
> #define BAD_MADT_GICC_ENTRY(entry, end) \
> (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
> (unsigned long)(entry) + (entry)->header.length > (end))
>
> -#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
> +#define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \
> spe_interrupt) + sizeof(u16))
>
> /* Basic configuration for ACPI */
> --
> 2.27.0.278.ge193c7cf3a9-goog
>

2020-06-09 20:40:08

by Jeremy Linton

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: acpi: fix UBSAN warning

Hi,

On 6/8/20 3:38 PM, Nick Desaulniers wrote:
> Will reported a UBSAN warning:
>
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> member access within null pointer of type 'struct acpi_madt_generic_interrupt'
> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
> Call trace:
> dump_backtrace+0x0/0x384
> show_stack+0x28/0x38
> dump_stack+0xec/0x174
> handle_null_ptr_deref+0x134/0x174
> __ubsan_handle_type_mismatch_v1+0x84/0xa4
> acpi_parse_gic_cpu_interface+0x60/0xe8
> acpi_parse_entries_array+0x288/0x498
> acpi_table_parse_entries_array+0x178/0x1b4
> acpi_table_parse_madt+0xa4/0x110
> acpi_parse_and_init_cpus+0x38/0x100
> smp_init_cpus+0x74/0x258
> setup_arch+0x350/0x3ec
> start_kernel+0x98/0x6f4
>
> This is from the use of the ACPI_OFFSET in
> arch/arm64/include/asm/acpi.h. Replace its use with offsetof from
> include/linux/stddef.h which should implement the same logic using
> __builtin_offsetof, so that UBSAN wont warn.

I looked at the longer thread about this, given that it appears to be a
false positive with respect to the macro, I tend to prefer Ard's
suggestion of just changing the offset value (1 should be sufficient
rather than 0) to avoid this kind of problem in the future.

But either way, this change looks fine too.

Reviewed-by: Jeremy Linton <[email protected]>

Thanks,

>
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: [email protected]
> Reported-by: Will Deacon <[email protected]>
> Suggested-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> Changes V1 -> V2:
> * Just fix one of the two warnings, specific to arm64.
> * Put warning in commit message.
>
> arch/arm64/include/asm/acpi.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index b263e239cb59..a45366c3909b 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -12,6 +12,7 @@
> #include <linux/efi.h>
> #include <linux/memblock.h>
> #include <linux/psci.h>
> +#include <linux/stddef.h>
>
> #include <asm/cputype.h>
> #include <asm/io.h>
> @@ -31,14 +32,14 @@
> * is therefore used to delimit the MADT GICC structure minimum length
> * appropriately.
> */
> -#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \
> +#define ACPI_MADT_GICC_MIN_LENGTH offsetof( \
> struct acpi_madt_generic_interrupt, efficiency_class)
>
> #define BAD_MADT_GICC_ENTRY(entry, end) \
> (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
> (unsigned long)(entry) + (entry)->header.length > (end))
>
> -#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
> +#define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \
> spe_interrupt) + sizeof(u16))
>
> /* Basic configuration for ACPI */
>

2020-06-10 15:51:13

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: acpi: fix UBSAN warning

On Mon, 8 Jun 2020 13:38:17 -0700, Nick Desaulniers wrote:
> Will reported a UBSAN warning:
>
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> member access within null pointer of type 'struct acpi_madt_generic_interrupt'
> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
> Call trace:
> dump_backtrace+0x0/0x384
> show_stack+0x28/0x38
> dump_stack+0xec/0x174
> handle_null_ptr_deref+0x134/0x174
> __ubsan_handle_type_mismatch_v1+0x84/0xa4
> acpi_parse_gic_cpu_interface+0x60/0xe8
> acpi_parse_entries_array+0x288/0x498
> acpi_table_parse_entries_array+0x178/0x1b4
> acpi_table_parse_madt+0xa4/0x110
> acpi_parse_and_init_cpus+0x38/0x100
> smp_init_cpus+0x74/0x258
> setup_arch+0x350/0x3ec
> start_kernel+0x98/0x6f4
>
> [...]

Applied to arm64 (for-next/core), thanks!

[1/1] arm64: acpi: fix UBSAN warning
https://git.kernel.org/arm64/c/a194c33f45f8

Cheers,
--
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

2020-06-10 23:10:09

[permalink] [raw]

Subject: RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

+JKim (for FreeBSD's perspective)

> -----Original Message-----
> From: Nick Desaulniers <[email protected]>
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert <[email protected]>; Kaneda, Erik
> <[email protected]>; Wysocki, Rafael J <[email protected]>;
> Len Brown <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>
> Will reported UBSAN warnings:
> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
>
Hi,

> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> can avoid this by using the compiler builtin, __builtin_offsetof.
>
This doesn't really fly because __builtin_offsetof is a compiler extension.

It looks like a lot of stddef.h files do this:

#define offsetof(a,b) __builtin_offset(a,b)

So does anyone have objections to ACPI_OFFSET being defined to offsetof()?

This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's.
If they don't have a definition for offsetof, we can supply the old one as a fallback.

Here's a patch:

--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -504,11 +504,17 @@ typedef u64 acpi_integer;
#define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b)))
#define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))

+/* Use an existing definiton for offsetof */
+
+#ifndef offsetof
+#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
+#endif
+
/* Pointer/Integer type conversions */

#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
-#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
+#define ACPI_OFFSET(d, f) offsetof (d,f)
#define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
#define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

Thanks,
Erik

> The non-kernel runtime of UBSAN would print:
> runtime error: member access within null pointer of type for this macro.
>
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: [email protected]
> Reported-by: Will Deacon <[email protected]>
> Suggested-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> include/acpi/actypes.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> 4defed58ea33..04359c70b198 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
> 0)
> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>
> --
> 2.27.0.rc2.251.g90737beb825-goog

2020-06-10 23:34:58

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <[email protected]> wrote:
>
> +JKim (for FreeBSD's perspective)
>
> > -----Original Message-----
> > From: Nick Desaulniers <[email protected]>
> > Sent: Monday, June 1, 2020 4:18 PM
> > To: Moore, Robert <[email protected]>; Kaneda, Erik
> > <[email protected]>; Wysocki, Rafael J <[email protected]>;
> > Len Brown <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>; [email protected];
> > [email protected]; [email protected]; linux-arm-
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> >
> > Will reported UBSAN warnings:
> > UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> > UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> >
> Hi,
>
> > Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
> > can avoid this by using the compiler builtin, __builtin_offsetof.
> >
> This doesn't really fly because __builtin_offsetof is a compiler extension.
>
> It looks like a lot of stddef.h files do this:
>
> #define offsetof(a,b) __builtin_offset(a,b)
>
> So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
>
> This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's.
> If they don't have a definition for offsetof, we can supply the old one as a fallback.
>
> Here's a patch:
>
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -504,11 +504,17 @@ typedef u64 acpi_integer;
> #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b)))
> #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
>
> +/* Use an existing definiton for offsetof */

s/definiton/definition/

> +
> +#ifndef offsetof
> +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#endif

If this header doesn't explicitly include <stddef.h> or
<linux/stddef.h>, won't translation units that include
<acpi/actypes.h> get different definitions of ACPI_OFFSET based on
whether they explicitly or transitively included <stddef.h> before
including <acpi/actypes.h>? Theoretically, a translation unit in the
kernel could include actypes.h, have no includes of linux/stddef.h,
then get UBSAN errors again from using this definition?

I don't mind using offsetof in place of the builtin (since it
typically will be implemented in terms of the builtin, or is at least
for the case specific to the Linux kernel). But if it's used, we
should include the header that defines it properly, and we should not
use the host's <stddef.h> IMO. Is there a platform specific way to
include the platform's stddef.h here?

Maybe linux/stddef.h should be included in
include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h
included in include/acpi/actypes.h, such that ACPI_OFFSET is defined
in terms of offsetof defined from that transitive dependency of
headers? (or do we get a circular inclusion trying to do that?)

> +
> /* Pointer/Integer type conversions */
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#define ACPI_OFFSET(d, f) offsetof (d,f)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>
> Thanks,
> Erik
>
> > The non-kernel runtime of UBSAN would print:
> > runtime error: member access within null pointer of type for this macro.
> >
> > Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> > Cc: [email protected]
> > Reported-by: Will Deacon <[email protected]>
> > Suggested-by: Ard Biesheuvel <[email protected]>
> > Signed-off-by: Nick Desaulniers <[email protected]>
> > ---
> > include/acpi/actypes.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> > 4defed58ea33..04359c70b198 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
> >
> > #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> > #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> > -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
> > 0)
> > +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
> > #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> > #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> >
> > --
> > 2.27.0.rc2.251.g90737beb825-goog
>

--
Thanks,
~Nick Desaulniers

2020-06-10 23:35:04

by Jung-uk Kim

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On 20. 6. 10., Kaneda, Erik wrote:
> +JKim (for FreeBSD's perspective)
>
>> -----Original Message-----
>> From: Nick Desaulniers <[email protected]>
>> Sent: Monday, June 1, 2020 4:18 PM
>> To: Moore, Robert <[email protected]>; Kaneda, Erik
>> <[email protected]>; Wysocki, Rafael J <[email protected]>;
>> Len Brown <[email protected]>
>> Cc: Ard Biesheuvel <[email protected]>; [email protected];
>> [email protected]; [email protected]; linux-arm-
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>>
>> Will reported UBSAN warnings:
>> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
>> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
>>
> Hi,
>
>> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
>> can avoid this by using the compiler builtin, __builtin_offsetof.
>>
> This doesn't really fly because __builtin_offsetof is a compiler extension.
>
> It looks like a lot of stddef.h files do this:
>
> #define offsetof(a,b) __builtin_offset(a,b)
>
> So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
>
> This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's.
> If they don't have a definition for offsetof, we can supply the old one as a fallback.
>
> Here's a patch:
>
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -504,11 +504,17 @@ typedef u64 acpi_integer;
> #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b)))
> #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
>
> +/* Use an existing definiton for offsetof */
> +
> +#ifndef offsetof
> +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#endif
> +
> /* Pointer/Integer type conversions */
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#define ACPI_OFFSET(d, f) offsetof (d,f)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

LGTM.

Jung-uk Kim

>> The non-kernel runtime of UBSAN would print:
>> runtime error: member access within null pointer of type for this macro.
>>
>> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
>> Cc: [email protected]
>> Reported-by: Will Deacon <[email protected]>
>> Suggested-by: Ard Biesheuvel <[email protected]>
>> Signed-off-by: Nick Desaulniers <[email protected]>
>> ---
>> include/acpi/actypes.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
>> 4defed58ea33..04359c70b198 100644
>> --- a/include/acpi/actypes.h
>> +++ b/include/acpi/actypes.h
>> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
>>
>> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
>> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
>> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
>> 0)
>> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
>> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
>> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>>
>> --
>> 2.27.0.rc2.251.g90737beb825-goog
>

2020-06-10 23:48:28

by Jung-uk Kim

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On 20. 6. 10., Nick Desaulniers wrote:
> On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <[email protected]> wrote:
>>
>> +JKim (for FreeBSD's perspective)
>>
>>> -----Original Message-----
>>> From: Nick Desaulniers <[email protected]>
>>> Sent: Monday, June 1, 2020 4:18 PM
>>> To: Moore, Robert <[email protected]>; Kaneda, Erik
>>> <[email protected]>; Wysocki, Rafael J <[email protected]>;
>>> Len Brown <[email protected]>
>>> Cc: Ard Biesheuvel <[email protected]>; [email protected];
>>> [email protected]; [email protected]; linux-arm-
>>> [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]
>>> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
>>>
>>> Will reported UBSAN warnings:
>>> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
>>> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
>>>
>> Hi,
>>
>>> Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We
>>> can avoid this by using the compiler builtin, __builtin_offsetof.
>>>
>> This doesn't really fly because __builtin_offsetof is a compiler extension.
>>
>> It looks like a lot of stddef.h files do this:
>>
>> #define offsetof(a,b) __builtin_offset(a,b)
>>
>> So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
>>
>> This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's.
>> If they don't have a definition for offsetof, we can supply the old one as a fallback.
>>
>> Here's a patch:
>>
>> --- a/include/acpi/actypes.h
>> +++ b/include/acpi/actypes.h
>> @@ -504,11 +504,17 @@ typedef u64 acpi_integer;
>> #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b)))
>> #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
>>
>> +/* Use an existing definiton for offsetof */
>
> s/definiton/definition/
>
>> +
>> +#ifndef offsetof
>> +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
>> +#endif
>
> If this header doesn't explicitly include <stddef.h> or
> <linux/stddef.h>, won't translation units that include
> <acpi/actypes.h> get different definitions of ACPI_OFFSET based on
> whether they explicitly or transitively included <stddef.h> before
> including <acpi/actypes.h>? Theoretically, a translation unit in the
> kernel could include actypes.h, have no includes of linux/stddef.h,
> then get UBSAN errors again from using this definition?
>
> I don't mind using offsetof in place of the builtin (since it
> typically will be implemented in terms of the builtin, or is at least
> for the case specific to the Linux kernel). But if it's used, we
> should include the header that defines it properly, and we should not
> use the host's <stddef.h> IMO. Is there a platform specific way to
> include the platform's stddef.h here?
>
> Maybe linux/stddef.h should be included in
> include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h
> included in include/acpi/actypes.h, such that ACPI_OFFSET is defined
> in terms of offsetof defined from that transitive dependency of
> headers? (or do we get a circular inclusion trying to do that?)

Actually, I think we should let platform-specific acfoo.h decide what to
do here, i.e.,

#ifndef ACPI_OFFSET
#define ACPI_OFFSET(d, f) ...
#endif

Jung-uk Kim

>> +
>> /* Pointer/Integer type conversions */
>>
>> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
>> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
>> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
>> +#define ACPI_OFFSET(d, f) offsetof (d,f)
>> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
>> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>>
>> Thanks,
>> Erik
>>
>>> The non-kernel runtime of UBSAN would print:
>>> runtime error: member access within null pointer of type for this macro.
>>>
>>> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
>>> Cc: [email protected]
>>> Reported-by: Will Deacon <[email protected]>
>>> Suggested-by: Ard Biesheuvel <[email protected]>
>>> Signed-off-by: Nick Desaulniers <[email protected]>
>>> ---
>>> include/acpi/actypes.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
>>> 4defed58ea33..04359c70b198 100644
>>> --- a/include/acpi/actypes.h
>>> +++ b/include/acpi/actypes.h
>>> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
>>>
>>> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
>>> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
>>> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
>>> 0)
>>> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
>>> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
>>> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>>>
>>> --
>>> 2.27.0.rc2.251.g90737beb825-goog

2020-06-11 16:49:03

[permalink] [raw]

Subject: RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

> -----Original Message-----
> From: Jung-uk Kim <[email protected]>
> Sent: Wednesday, June 10, 2020 4:46 PM
> To: Nick Desaulniers <[email protected]>; Kaneda, Erik
> <[email protected]>
> Cc: Wysocki, Rafael J <[email protected]>; Ard Biesheuvel
> <[email protected]>; [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using
> __builtin_offsetof
>
> On 20. 6. 10., Nick Desaulniers wrote:
> > On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <[email protected]>
> wrote:
> >>
> >> +JKim (for FreeBSD's perspective)
> >>
> >>> -----Original Message-----
> >>> From: Nick Desaulniers <[email protected]>
> >>> Sent: Monday, June 1, 2020 4:18 PM
> >>> To: Moore, Robert <[email protected]>; Kaneda, Erik
> >>> <[email protected]>; Wysocki, Rafael J
> <[email protected]>;
> >>> Len Brown <[email protected]>
> >>> Cc: Ard Biesheuvel <[email protected]>; [email protected];
> >>> [email protected]; [email protected]; linux-arm-
> >>> [email protected]; [email protected];
> >>> [email protected]; [email protected];
> >>> [email protected]; [email protected]; [email protected];
> >>> [email protected]; [email protected]; [email protected];
> >>> [email protected]
> >>> Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> >>>
> >>> Will reported UBSAN warnings:
> >>> UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> >>> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> >>>
> >> Hi,
> >>
> >>> Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
> We
> >>> can avoid this by using the compiler builtin, __builtin_offsetof.
> >>>
> >> This doesn't really fly because __builtin_offsetof is a compiler extension.
> >>
> >> It looks like a lot of stddef.h files do this:
> >>
> >> #define offsetof(a,b) __builtin_offset(a,b)
> >>
> >> So does anyone have objections to ACPI_OFFSET being defined to
> offsetof()?
> >>
> >> This will allow a host OS project project to use their own definitions of
> offsetof in place of ACPICA's.
> >> If they don't have a definition for offsetof, we can supply the old one as a
> fallback.
> >>
> >> Here's a patch:
> >>
> >> --- a/include/acpi/actypes.h
> >> +++ b/include/acpi/actypes.h
> >> @@ -504,11 +504,17 @@ typedef u64 acpi_integer;
> >> #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR
> (u8, (a)) - (acpi_size)(b)))
> >> #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a))
> - ACPI_CAST_PTR (u8, (b))))
> >>
> >> +/* Use an existing definiton for offsetof */
> >
> > s/definiton/definition/
> >
> >> +
> >> +#ifndef offsetof
> >> +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
> 0)
> >> +#endif
> >
> > If this header doesn't explicitly include <stddef.h> or
> > <linux/stddef.h>, won't translation units that include
> > <acpi/actypes.h> get different definitions of ACPI_OFFSET based on
> > whether they explicitly or transitively included <stddef.h> before
> > including <acpi/actypes.h>? Theoretically, a translation unit in the
> > kernel could include actypes.h, have no includes of linux/stddef.h,
> > then get UBSAN errors again from using this definition?
> >
> > I don't mind using offsetof in place of the builtin (since it
> > typically will be implemented in terms of the builtin, or is at least
> > for the case specific to the Linux kernel). But if it's used, we
> > should include the header that defines it properly, and we should not
> > use the host's <stddef.h> IMO. Is there a platform specific way to
> > include the platform's stddef.h here?
> >
> > Maybe linux/stddef.h should be included in
> > include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h
> > included in include/acpi/actypes.h, such that ACPI_OFFSET is defined
> > in terms of offsetof defined from that transitive dependency of
> > headers? (or do we get a circular inclusion trying to do that?)
>
Hi,

> Actually, I think we should let platform-specific acfoo.h decide what to
> do here, i.e.,

That's a better solution. For Linux, it would look something like this:

--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -508,10 +508,15 @@ typedef u64 acpi_integer;

#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
-#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
#define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
#define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

+/* Platforms may want to define their own ACPI_OFFSET */
+
+#ifndef ACPI_OFFSET
+#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
+#endif
+
/* Optimizations for 4-character (32-bit) acpi_name manipulation */

#ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 987e2af7c335..5d1ca6015fce 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -71,6 +71,11 @@
#undef ACPI_DEBUG_DEFAULT
#define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR)

+/* Use gcc's builtin offset instead of the default */
+
+#undef ACPI_OFFSET
+#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b)
+
#ifndef CONFIG_ACPI

>
> #ifndef ACPI_OFFSET
> #define ACPI_OFFSET(d, f) ...
> #endif
>
> Jung-uk Kim
>
> >> +
> >> /* Pointer/Integer type conversions */
> >>
> >> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> >> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> >> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
> *) 0)
> >> +#define ACPI_OFFSET(d, f) offsetof (d,f)
> >> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> >> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> >>
> >> Thanks,
> >> Erik
> >>
> >>> The non-kernel runtime of UBSAN would print:
> >>> runtime error: member access within null pointer of type for this macro.
> >>>
> >>> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-
> truck/
> >>> Cc: [email protected]
> >>> Reported-by: Will Deacon <[email protected]>
> >>> Suggested-by: Ard Biesheuvel <[email protected]>
> >>> Signed-off-by: Nick Desaulniers <[email protected]>
> >>> ---
> >>> include/acpi/actypes.h | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index
> >>> 4defed58ea33..04359c70b198 100644
> >>> --- a/include/acpi/actypes.h
> >>> +++ b/include/acpi/actypes.h
> >>> @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
> >>>
> >>> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size)
> (i))
> >>> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> >>> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
> *)
> >>> 0)
> >>> +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f)
> >>> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> >>> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
> >>>
> >>> --
> >>> 2.27.0.rc2.251.g90737beb825-goog
>
> _______________________________________________
> Devel mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

2020-06-11 17:08:55

[permalink] [raw]

Subject: Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik <[email protected]> wrote:
>
> > From: Jung-uk Kim <[email protected]>
> >
> > Actually, I think we should let platform-specific acfoo.h decide what to
> > do here, i.e.,
>
> That's a better solution. For Linux, it would look something like this:
>
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -508,10 +508,15 @@ typedef u64 acpi_integer;
>
> #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i))
> #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
> -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
> #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
>
> +/* Platforms may want to define their own ACPI_OFFSET */
> +
> +#ifndef ACPI_OFFSET
> +#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0)
> +#endif
> +
> /* Optimizations for 4-character (32-bit) acpi_name manipulation */
>
> #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 987e2af7c335..5d1ca6015fce 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -71,6 +71,11 @@
> #undef ACPI_DEBUG_DEFAULT
> #define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR)
>
> +/* Use gcc's builtin offset instead of the default */
> +
> +#undef ACPI_OFFSET
> +#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b)
> +
> #ifndef CONFIG_ACPI
>

Looks good at first glance. Wouldn't actypes.h need to include
platform/acenv.h first though? Otherwise you put some header
inclusion order dependency on folks who include actypes.h to first
include acenv.h otherwise we're not getting the definition in terms of
__builtin_offsetof.

--
Thanks,
~Nick Desaulniers

2020-06-16 21:41:41