arm64/acpi: NULL dereference reports from UBSAN at boot

arm64/acpi: NULL dereference reports from UBSAN at boot

[Will Deacon]

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.

Please can you take a look when you get a chance? I haven't had time to see
if this is a regression or not, but I don't think it's particularly serious
as I have all sorts of horrible stuff enabled in my .config, since I'm
trying to chase down another bug:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/plain/arch/arm64/configs/fuzzing.config?h=fuzzing/arm64-kernelci-20200519&id=c149cf6a51aa4f72d53fc681c6661094e93ef660

(on top of defconfig)

CONFIG_FAIL_PAGE_ALLOC may be to blame.

Cheers,

Will

--->8

[    0.000000][    T0] ================================================================================
[    0.000000][    T0] UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
[    0.000000][    T0] member access within null pointer of type 'struct acpi_table_fadt'
[    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
[    0.000000][    T0] Call trace:
[    0.000000][    T0]  dump_backtrace+0x0/0x384
[    0.000000][    T0]  show_stack+0x28/0x38
[    0.000000][    T0]  dump_stack+0xec/0x174
[    0.000000][    T0]  handle_null_ptr_deref+0x134/0x174
[    0.000000][    T0]  __ubsan_handle_type_mismatch_v1+0x84/0xa4
[    0.000000][    T0]  acpi_tb_create_local_fadt+0x1d4/0x1418
[    0.000000][    T0]  acpi_tb_parse_fadt+0x108/0x4b8
[    0.000000][    T0]  acpi_tb_parse_root_table+0x380/0x578
[    0.000000][    T0]  acpi_initialize_tables+0x140/0x194
[    0.000000][    T0]  acpi_table_init+0x90/0xcc
[    0.000000][    T0]  acpi_boot_table_init+0xfc/0x1c8
[    0.000000][    T0]  setup_arch+0x2b4/0x3ec
[    0.000000][    T0]  start_kernel+0x98/0x6f4
[    0.000000][    T0] ================================================================================

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

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

[Lorenzo Pieralisi]

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.

> Please can you take a look when you get a chance? I haven't had time to see
> if this is a regression or not, but I don't think it's particularly serious
> as I have all sorts of horrible stuff enabled in my .config, since I'm
> trying to chase down another bug:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/plain/arch/arm64/configs/fuzzing.config?h=fuzzing/arm64-kernelci-20200519&id=c149cf6a51aa4f72d53fc681c6661094e93ef660
> 
> (on top of defconfig)
> 
> CONFIG_FAIL_PAGE_ALLOC may be to blame.

Not sure about that, they are both quite cryptic, I wonder if UBSAN
is not tricked by the ACPI_OFFSET macro - need to debug it further
to understand what's going on here.

Thanks,
Lorenzo

> Cheers,
> 
> Will
> 
> --->8
> 
> [    0.000000][    T0] ================================================================================
> [    0.000000][    T0] UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
> [    0.000000][    T0] member access within null pointer of type 'struct acpi_table_fadt'
> [    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
> [    0.000000][    T0] Call trace:
> [    0.000000][    T0]  dump_backtrace+0x0/0x384
> [    0.000000][    T0]  show_stack+0x28/0x38
> [    0.000000][    T0]  dump_stack+0xec/0x174
> [    0.000000][    T0]  handle_null_ptr_deref+0x134/0x174
> [    0.000000][    T0]  __ubsan_handle_type_mismatch_v1+0x84/0xa4
> [    0.000000][    T0]  acpi_tb_create_local_fadt+0x1d4/0x1418
> [    0.000000][    T0]  acpi_tb_parse_fadt+0x108/0x4b8
> [    0.000000][    T0]  acpi_tb_parse_root_table+0x380/0x578
> [    0.000000][    T0]  acpi_initialize_tables+0x140/0x194
> [    0.000000][    T0]  acpi_table_init+0x90/0xcc
> [    0.000000][    T0]  acpi_boot_table_init+0xfc/0x1c8
> [    0.000000][    T0]  setup_arch+0x2b4/0x3ec
> [    0.000000][    T0]  start_kernel+0x98/0x6f4
> [    0.000000][    T0] ================================================================================
> 
> [    0.000000][    T0] ================================================================================
> [    0.000000][    T0] UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
> [    0.000000][    T0] member access within null pointer of type 'struct acpi_madt_generic_interrupt'
> [    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
> [    0.000000][    T0] Call trace:
> [    0.000000][    T0]  dump_backtrace+0x0/0x384
> [    0.000000][    T0]  show_stack+0x28/0x38
> [    0.000000][    T0]  dump_stack+0xec/0x174
> [    0.000000][    T0]  handle_null_ptr_deref+0x134/0x174
> [    0.000000][    T0]  __ubsan_handle_type_mismatch_v1+0x84/0xa4
> [    0.000000][    T0]  acpi_parse_gic_cpu_interface+0x60/0xe8
> [    0.000000][    T0]  acpi_parse_entries_array+0x288/0x498
> [    0.000000][    T0]  acpi_table_parse_entries_array+0x178/0x1b4
> [    0.000000][    T0]  acpi_table_parse_madt+0xa4/0x110
> [    0.000000][    T0]  acpi_parse_and_init_cpus+0x38/0x100
> [    0.000000][    T0]  smp_init_cpus+0x74/0x258
> [    0.000000][    T0]  setup_arch+0x350/0x3ec
> [    0.000000][    T0]  start_kernel+0x98/0x6f4
> [    0.000000][    T0] ================================================================================

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

[Will Deacon]

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

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

[Lorenzo Pieralisi]

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

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

[Will Deacon]

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

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

[Nick Desaulniers]

On Mon, Jun 1, 2020 at 12:05 AM Will Deacon <will@kernel.org> 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

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

[Ard Biesheuvel]

On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Mon, Jun 1, 2020 at 12:05 AM Will Deacon <will@kernel.org> 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)

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

[Nick Desaulniers]

On Mon, Jun 1, 2020 at 2:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <ndesaulniers@google.com> 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

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

[Ard Biesheuvel]

On Tue, 2 Jun 2020 at 00:19, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Mon, Jun 1, 2020 at 2:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 1 Jun 2020 at 23:52, Nick Desaulniers <ndesaulniers@google.com> 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.

[PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

[Nick Desaulniers]

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: stable@vger.kernel.org
Reported-by: Will Deacon <will@kernel.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 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

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

[Peter Collingbourne]

On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers <ndesaulniers@google.com> 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: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  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
>

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

[Nick Desaulniers]

On Mon, Jun 1, 2020 at 4:37 PM Peter Collingbourne <pcc@google.com> wrote:
>
> On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers <ndesaulniers@google.com> 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

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

[Kaneda, Erik]

> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
> <erik.kaneda@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> Len Brown <lenb@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
> glider@google.com; guohanjun@huawei.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
> ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
> devel@acpica.org
> 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: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  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

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

[Nick Desaulniers]

On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <erik.kaneda@intel.com> 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

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

[Will Deacon]

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 <erik.kaneda@intel.com> 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

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

[Nick Desaulniers]

On Mon, Jun 8, 2020 at 7:51 AM Will Deacon <will@kernel.org> 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 <erik.kaneda@intel.com> 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

[PATCH v2] arm64: acpi: fix UBSAN warning

[Nick Desaulniers]

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: stable@vger.kernel.org
Reported-by: Will Deacon <will@kernel.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
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

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

[Lorenzo Pieralisi]

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: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> 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 <lorenzo.pieralisi@arm.com>

> 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
> 

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

[Jeremy Linton]

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 <jeremy.linton@arm.com>

Thanks,

> 
> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
> Cc: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> 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 */
> 

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

[Will Deacon]

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

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

[Kaneda, Erik]

> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: Tuesday, June 2, 2020 11:47 AM
> To: Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Moore, Robert <robert.moore@intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Ard
> Biesheuvel <ardb@kernel.org>; dvyukov@google.com; glider@google.com;
> guohanjun@huawei.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; lorenzo.pieralisi@arm.com;
> mark.rutland@arm.com; pcc@google.com; rjw@rjwysocki.net;
> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
> devel@acpica.org
> Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
> 
> On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik <erik.kaneda@intel.com>
> 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

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

[Kaneda, Erik]

+JKim (for FreeBSD's perspective)

> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: Monday, June 1, 2020 4:18 PM
> To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
> <erik.kaneda@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> Len Brown <lenb@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
> glider@google.com; guohanjun@huawei.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
> ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
> devel@acpica.org
> 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: stable@vger.kernel.org
> Reported-by: Will Deacon <will@kernel.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  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

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

[Nick Desaulniers]

On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <erik.kaneda@intel.com> wrote:
>
> +JKim (for FreeBSD's perspective)
>
> > -----Original Message-----
> > From: Nick Desaulniers <ndesaulniers@google.com>
> > Sent: Monday, June 1, 2020 4:18 PM
> > To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
> > <erik.kaneda@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> > Len Brown <lenb@kernel.org>
> > Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
> > glider@google.com; guohanjun@huawei.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
> > ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
> > will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
> > devel@acpica.org
> > 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: stable@vger.kernel.org
> > Reported-by: Will Deacon <will@kernel.org>
> > Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  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

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

[Jung-uk Kim]

On 20. 6. 10., Nick Desaulniers wrote:
> On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <erik.kaneda@intel.com> wrote:
>>
>> +JKim (for FreeBSD's perspective)
>>
>>> -----Original Message-----
>>> From: Nick Desaulniers <ndesaulniers@google.com>
>>> Sent: Monday, June 1, 2020 4:18 PM
>>> To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
>>> <erik.kaneda@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
>>> Len Brown <lenb@kernel.org>
>>> Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
>>> glider@google.com; guohanjun@huawei.com; linux-arm-
>>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>>> lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
>>> ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
>>> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
>>> devel@acpica.org
>>> 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: stable@vger.kernel.org
>>> Reported-by: Will Deacon <will@kernel.org>
>>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
>>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>>> ---
>>>  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

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

[Kaneda, Erik]

> -----Original Message-----
> From: Jung-uk Kim <jkim@FreeBSD.org>
> Sent: Wednesday, June 10, 2020 4:46 PM
> To: Nick Desaulniers <ndesaulniers@google.com>; Kaneda, Erik
> <erik.kaneda@intel.com>
> Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Ard Biesheuvel
> <ardb@kernel.org>; dvyukov@google.com; glider@google.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com;
> rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-
> acpi@vger.kernel.org; devel@acpica.org
> 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 <erik.kaneda@intel.com>
> wrote:
> >>
> >> +JKim (for FreeBSD's perspective)
> >>
> >>> -----Original Message-----
> >>> From: Nick Desaulniers <ndesaulniers@google.com>
> >>> Sent: Monday, June 1, 2020 4:18 PM
> >>> To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
> >>> <erik.kaneda@intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>;
> >>> Len Brown <lenb@kernel.org>
> >>> Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
> >>> glider@google.com; guohanjun@huawei.com; linux-arm-
> >>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> >>> lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
> >>> ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
> >>> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
> >>> devel@acpica.org
> >>> 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: stable@vger.kernel.org
> >>> Reported-by: Will Deacon <will@kernel.org>
> >>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> >>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> >>> ---
> >>>  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 -- devel@acpica.org
> To unsubscribe send an email to devel-leave@acpica.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

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

[Nick Desaulniers]

On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik <erik.kaneda@intel.com> wrote:
>
> > From: Jung-uk Kim <jkim@FreeBSD.org>
> >
> > 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

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

[Kaneda, Erik]

> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: Thursday, June 11, 2020 10:06 AM
> To: Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Jung-uk Kim <jkim@freebsd.org>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Ard Biesheuvel <ardb@kernel.org>;
> dvyukov@google.com; glider@google.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com;
> rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-
> acpi@vger.kernel.org; devel@acpica.org
> Subject: Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using
> __builtin_offsetof
> 
> On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik <erik.kaneda@intel.com>
> wrote:
> >
> > > From: Jung-uk Kim <jkim@FreeBSD.org>
> > >
> > > 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
> >
> 
Hi,

Sorry about the delayed response.

> 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.

Actypes.h is intended for ACPICA use. For files using ACPI_OFFSET, they include headers like acpi.h that ends up including aclinux.h before including actypes.h. For those using ACPI_OFFSET, precedence will be given to the definition in aclinux.h before falling back to the definition in actypes.

Erik

> 
> --
> Thanks,
> ~Nick Desaulniers

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

[Jung-uk Kim]

On 20. 6. 10., Kaneda, Erik wrote:
> +JKim (for FreeBSD's perspective)
> 
>> -----Original Message-----
>> From: Nick Desaulniers <ndesaulniers@google.com>
>> Sent: Monday, June 1, 2020 4:18 PM
>> To: Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
>> <erik.kaneda@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
>> Len Brown <lenb@kernel.org>
>> Cc: Ard Biesheuvel <ardb@kernel.org>; dvyukov@google.com;
>> glider@google.com; guohanjun@huawei.com; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> lorenzo.pieralisi@arm.com; mark.rutland@arm.com;
>> ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net;
>> will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org;
>> devel@acpica.org
>> 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: stable@vger.kernel.org
>> Reported-by: Will Deacon <will@kernel.org>
>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>> ---
>>  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
> 

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

[Hanjun Guo]

Hi Will,

On 2020/5/21 18:09, 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.
> 
> Please can you take a look when you get a chance? I haven't had time to see
> if this is a regression or not, but I don't think it's particularly serious
> as I have all sorts of horrible stuff enabled in my .config, since I'm
> trying to chase down another bug:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/plain/arch/arm64/configs/fuzzing.config?h=fuzzing/arm64-kernelci-20200519&id=c149cf6a51aa4f72d53fc681c6661094e93ef660
> 
> (on top of defconfig)
> 
> CONFIG_FAIL_PAGE_ALLOC may be to blame.

I enabled UBSAN and CONFIG_FAIL_PAGE_ALLOC on top of defconfig,
testing against the for-kernelci branch on the D06 board, I
can see some UBSAN warnings from megaraid_sas driver [0], but not
from any other subsystem including ACPI, I will try all your
configs above to see if I can get more warnings.

Thanks
Hanjun

[0]:
[   18.244272] 
================================================================================
[   18.252673] UBSAN: array-index-out-of-bounds in 
drivers/scsi/megaraid/megaraid_sas_fp.c:104:32
[   18.261244] index 1 is out of range for type 'MR_LD_SPAN_MAP [1]'
[   18.267313] CPU: 0 PID: 656 Comm: kworker/0:1 Not tainted 
5.7.0-rc6-1-14703-gf4582661223d-dirty #20
[   18.276314] Hardware name: Huawei TaiShan 2280 V2/BC82AMDC, BIOS 
2280-V2 CS V3.B210.01 03/12/2020
[   18.285151] Workqueue: events work_for_cpu_fn
[   18.289488] Call trace:
[   18.291925]  dump_backtrace+0x0/0x248
[   18.295572]  show_stack+0x18/0x28
[   18.298873]  dump_stack+0xc0/0x10c
[   18.302261]  ubsan_epilogue+0x10/0x58
[   18.305905]  __ubsan_handle_out_of_bounds+0x8c/0xa8
[   18.310763]  mr_update_load_balance_params+0x118/0x120
[   18.315877]  MR_ValidateMapInfo+0x300/0xb00
[   18.320040]  megasas_get_map_info+0x134/0x1f8
[   18.324377]  megasas_init_adapter_fusion+0xba8/0x10a0
[   18.329403]  megasas_probe_one+0x6e0/0x1b70
[   18.333569]  local_pci_probe+0x40/0xb0
[   18.337299]  work_for_cpu_fn+0x1c/0x30
[   18.341031]  process_one_work+0x1f8/0x378
[   18.345022]  worker_thread+0x21c/0x4c0
[   18.348753]  kthread+0x150/0x158
[   18.351967]  ret_from_fork+0x10/0x18
[   18.355529] 
================================================================================

[   18.592274] 
================================================================================
[   18.600672] UBSAN: array-index-out-of-bounds in 
drivers/scsi/megaraid/megaraid_sas_fp.c:141:9
[   18.609155] index 1 is out of range for type 'MR_LD_SPAN_MAP [1]'
[   18.615221] CPU: 0 PID: 656 Comm: kworker/0:1 Not tainted 
5.7.0-rc6-1-14703-gf4582661223d-dirty #20
[   18.624222] Hardware name: Huawei TaiShan 2280 V2/BC82AMDC, BIOS 
2280-V2 CS V3.B210.01 03/12/2020
[   18.633050] Workqueue: events work_for_cpu_fn
[   18.637387] Call trace:
[   18.639822]  dump_backtrace+0x0/0x248
[   18.643467]  show_stack+0x18/0x28
[   18.646767]  dump_stack+0xc0/0x10c
[   18.650152]  ubsan_epilogue+0x10/0x58
[   18.653796]  __ubsan_handle_out_of_bounds+0x8c/0xa8
[   18.658652]  MR_GetLDTgtId+0x58/0x60
[   18.662211]  megasas_sync_map_info+0xd0/0x1c0
[   18.666547]  megasas_init_adapter_fusion+0xd60/0x10a0
[   18.671574]  megasas_probe_one+0x6e0/0x1b70
[   18.675736]  local_pci_probe+0x40/0xb0
[   18.679466]  work_for_cpu_fn+0x1c/0x30
[   18.683197]  process_one_work+0x1f8/0x378
[   18.687188]  worker_thread+0x21c/0x4c0
[   18.690920]  kthread+0x150/0x158
[   18.694123]  ret_from_fork+0x10/0x18
[   18.697683] 
================================================================================

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

[Hanjun Guo]

On 2020/5/22 16:07, Hanjun Guo wrote:
> Hi Will,
> 
> On 2020/5/21 18:09, 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.
>>
>> Please can you take a look when you get a chance? I haven't had time 
>> to see
>> if this is a regression or not, but I don't think it's particularly 
>> serious
>> as I have all sorts of horrible stuff enabled in my .config, since I'm
>> trying to chase down another bug:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/plain/arch/arm64/configs/fuzzing.config?h=fuzzing/arm64-kernelci-20200519&id=c149cf6a51aa4f72d53fc681c6661094e93ef660 
>>
>>
>> (on top of defconfig)
>>
>> CONFIG_FAIL_PAGE_ALLOC may be to blame.
> 
> I enabled UBSAN and CONFIG_FAIL_PAGE_ALLOC on top of defconfig,
> testing against the for-kernelci branch on the D06 board, I
> can see some UBSAN warnings from megaraid_sas driver [0], but not
> from any other subsystem including ACPI, I will try all your
> configs above to see if I can get more warnings.

Enabled all the configs except "CONFIG_MODULES=n" and
"CONFIG_SHADOW_CALL_STACK=y" (can't do that via make menuconfig,
do it manually?) in the link, but still got the same warnings and
no other warnings as before, the system is in good function, please
let me know I can do anything more to help the debug.

Thanks
Hanjun