* arm64/acpi: NULL dereference reports from UBSAN at boot @ 2020-05-21 10:09 Will Deacon 2020-05-21 17:37 ` Lorenzo Pieralisi 2020-05-22 8:07 ` arm64/acpi: NULL dereference reports from UBSAN at boot Hanjun Guo 0 siblings, 2 replies; 30+ messages in thread From: Will Deacon @ 2020-05-21 10:09 UTC (permalink / raw) To: lorenzo.pieralisi, guohanjun Cc: rjw, linux-arm-kernel, linux-kernel, mark.rutland 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] ================================================================================ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-21 10:09 arm64/acpi: NULL dereference reports from UBSAN at boot Will Deacon @ 2020-05-21 17:37 ` Lorenzo Pieralisi 2020-05-26 20:21 ` Will Deacon 2020-05-22 8:07 ` arm64/acpi: NULL dereference reports from UBSAN at boot Hanjun Guo 1 sibling, 1 reply; 30+ messages in thread From: Lorenzo Pieralisi @ 2020-05-21 17:37 UTC (permalink / raw) To: Will Deacon; +Cc: guohanjun, rjw, linux-arm-kernel, linux-kernel, mark.rutland 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] ================================================================================ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-21 17:37 ` Lorenzo Pieralisi @ 2020-05-26 20:21 ` Will Deacon 2020-05-27 13:41 ` Lorenzo Pieralisi 0 siblings, 1 reply; 30+ messages in thread From: Will Deacon @ 2020-05-26 20:21 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: guohanjun, rjw, linux-arm-kernel, linux-kernel, mark.rutland, ndesaulniers 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-26 20:21 ` Will Deacon @ 2020-05-27 13:41 ` Lorenzo Pieralisi 2020-06-01 7:05 ` Will Deacon 0 siblings, 1 reply; 30+ messages in thread From: Lorenzo Pieralisi @ 2020-05-27 13:41 UTC (permalink / raw) To: Will Deacon Cc: guohanjun, rjw, linux-arm-kernel, linux-kernel, mark.rutland, ndesaulniers 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-27 13:41 ` Lorenzo Pieralisi @ 2020-06-01 7:05 ` Will Deacon 2020-06-01 21:51 ` Nick Desaulniers 0 siblings, 1 reply; 30+ messages in thread From: Will Deacon @ 2020-06-01 7:05 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: guohanjun, rjw, linux-arm-kernel, linux-kernel, mark.rutland, ndesaulniers 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-06-01 7:05 ` Will Deacon @ 2020-06-01 21:51 ` Nick Desaulniers 2020-06-01 21:57 ` Ard Biesheuvel 0 siblings, 1 reply; 30+ messages in thread From: Nick Desaulniers @ 2020-06-01 21:51 UTC (permalink / raw) To: Will Deacon Cc: Lorenzo Pieralisi, guohanjun, Rafael J. Wysocki, Linux ARM, LKML, Mark Rutland, Dmitry Vyukov, Alexander Potapenko, Peter Collingbourne, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-06-01 21:51 ` Nick Desaulniers @ 2020-06-01 21:57 ` Ard Biesheuvel 2020-06-01 22:19 ` Nick Desaulniers 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2020-06-01 21:57 UTC (permalink / raw) To: Nick Desaulniers Cc: Will Deacon, Lorenzo Pieralisi, Hanjun Guo, Rafael J. Wysocki, Linux ARM, LKML, Mark Rutland, Dmitry Vyukov, Alexander Potapenko, Peter Collingbourne 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) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-06-01 21:57 ` Ard Biesheuvel @ 2020-06-01 22:19 ` Nick Desaulniers 2020-06-01 22:28 ` Ard Biesheuvel 0 siblings, 1 reply; 30+ messages in thread From: Nick Desaulniers @ 2020-06-01 22:19 UTC (permalink / raw) To: Ard Biesheuvel Cc: Will Deacon, Lorenzo Pieralisi, Hanjun Guo, Rafael J. Wysocki, Linux ARM, LKML, Mark Rutland, Dmitry Vyukov, Alexander Potapenko, Peter Collingbourne 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-06-01 22:19 ` Nick Desaulniers @ 2020-06-01 22:28 ` Ard Biesheuvel 2020-06-01 23:18 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Nick Desaulniers 0 siblings, 1 reply; 30+ messages in thread From: Ard Biesheuvel @ 2020-06-01 22:28 UTC (permalink / raw) To: Nick Desaulniers Cc: Will Deacon, Lorenzo Pieralisi, Hanjun Guo, Rafael J. Wysocki, Linux ARM, LKML, Mark Rutland, Dmitry Vyukov, Alexander Potapenko, Peter Collingbourne 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-01 22:28 ` Ard Biesheuvel @ 2020-06-01 23:18 ` Nick Desaulniers 2020-06-01 23:37 ` Peter Collingbourne ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Nick Desaulniers @ 2020-06-01 23:18 UTC (permalink / raw) To: Robert Moore, Erik Kaneda, Rafael J. Wysocki, Len Brown Cc: Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, ndesaulniers, pcc, rjw, will, stable, linux-acpi, devel 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-01 23:18 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Nick Desaulniers @ 2020-06-01 23:37 ` Peter Collingbourne 2020-06-01 23:48 ` Nick Desaulniers 2020-06-02 0:02 ` Kaneda, Erik 2020-06-10 23:06 ` Kaneda, Erik 2 siblings, 1 reply; 30+ messages in thread From: Peter Collingbourne @ 2020-06-01 23:37 UTC (permalink / raw) To: Nick Desaulniers Cc: Robert Moore, Erik Kaneda, Rafael J. Wysocki, Len Brown, Ard Biesheuvel, Dmitry Vyukov, Alexander Potapenko, guohanjun, Linux ARM, Linux Kernel Mailing List, lorenzo.pieralisi, Mark Rutland, rjw, Will Deacon, stable, linux-acpi, devel 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 > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-01 23:37 ` Peter Collingbourne @ 2020-06-01 23:48 ` Nick Desaulniers 0 siblings, 0 replies; 30+ messages in thread From: Nick Desaulniers @ 2020-06-01 23:48 UTC (permalink / raw) To: Peter Collingbourne Cc: Robert Moore, Erik Kaneda, Rafael J. Wysocki, Len Brown, Ard Biesheuvel, Dmitry Vyukov, Alexander Potapenko, Hanjun Guo, Linux ARM, Linux Kernel Mailing List, Lorenzo Pieralisi, Mark Rutland, Rafael J. Wysocki, Will Deacon, # 3.4.x, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-01 23:18 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Nick Desaulniers 2020-06-01 23:37 ` Peter Collingbourne @ 2020-06-02 0:02 ` Kaneda, Erik 2020-06-02 18:46 ` Nick Desaulniers 2020-06-10 23:06 ` Kaneda, Erik 2 siblings, 1 reply; 30+ messages in thread From: Kaneda, Erik @ 2020-06-02 0:02 UTC (permalink / raw) To: Nick Desaulniers, Moore, Robert, Wysocki, Rafael J, Len Brown Cc: Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel > -----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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-02 0:02 ` Kaneda, Erik @ 2020-06-02 18:46 ` Nick Desaulniers 2020-06-08 14:51 ` Will Deacon 2020-06-08 23:20 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Kaneda, Erik 0 siblings, 2 replies; 30+ messages in thread From: Nick Desaulniers @ 2020-06-02 18:46 UTC (permalink / raw) To: Kaneda, Erik Cc: Moore, Robert, Wysocki, Rafael J, Len Brown, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-02 18:46 ` Nick Desaulniers @ 2020-06-08 14:51 ` Will Deacon 2020-06-08 20:29 ` Nick Desaulniers 2020-06-08 23:20 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Kaneda, Erik 1 sibling, 1 reply; 30+ messages in thread From: Will Deacon @ 2020-06-08 14:51 UTC (permalink / raw) To: Nick Desaulniers Cc: Kaneda, Erik, Moore, Robert, Wysocki, Rafael J, Len Brown, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, stable, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-08 14:51 ` Will Deacon @ 2020-06-08 20:29 ` Nick Desaulniers 2020-06-08 20:38 ` [PATCH v2] arm64: acpi: fix UBSAN warning Nick Desaulniers 0 siblings, 1 reply; 30+ messages in thread From: Nick Desaulniers @ 2020-06-08 20:29 UTC (permalink / raw) To: Will Deacon, Moore, Robert Cc: Kaneda, Erik, Wysocki, Rafael J, Len Brown, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, stable, linux-acpi, devel 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2] arm64: acpi: fix UBSAN warning 2020-06-08 20:29 ` Nick Desaulniers @ 2020-06-08 20:38 ` Nick Desaulniers 2020-06-09 17:46 ` Lorenzo Pieralisi ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Nick Desaulniers @ 2020-06-08 20:38 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: Nick Desaulniers, stable, Ard Biesheuvel, Enrico Weigelt, Allison Randal, Lorenzo Pieralisi, Jeremy Linton, Thomas Gleixner, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2] arm64: acpi: fix UBSAN warning 2020-06-08 20:38 ` [PATCH v2] arm64: acpi: fix UBSAN warning Nick Desaulniers @ 2020-06-09 17:46 ` Lorenzo Pieralisi 2020-06-09 19:50 ` Jeremy Linton 2020-06-10 11:21 ` Will Deacon 2 siblings, 0 replies; 30+ messages in thread From: Lorenzo Pieralisi @ 2020-06-09 17:46 UTC (permalink / raw) To: Nick Desaulniers Cc: Will Deacon, Catalin Marinas, stable, Ard Biesheuvel, Enrico Weigelt, Allison Randal, Jeremy Linton, Thomas Gleixner, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] arm64: acpi: fix UBSAN warning 2020-06-08 20:38 ` [PATCH v2] arm64: acpi: fix UBSAN warning Nick Desaulniers 2020-06-09 17:46 ` Lorenzo Pieralisi @ 2020-06-09 19:50 ` Jeremy Linton 2020-06-10 11:21 ` Will Deacon 2 siblings, 0 replies; 30+ messages in thread From: Jeremy Linton @ 2020-06-09 19:50 UTC (permalink / raw) To: Nick Desaulniers, Will Deacon, Catalin Marinas Cc: stable, Ard Biesheuvel, Enrico Weigelt, Allison Randal, Lorenzo Pieralisi, Thomas Gleixner, linux-arm-kernel, linux-kernel 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 */ > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] arm64: acpi: fix UBSAN warning 2020-06-08 20:38 ` [PATCH v2] arm64: acpi: fix UBSAN warning Nick Desaulniers 2020-06-09 17:46 ` Lorenzo Pieralisi 2020-06-09 19:50 ` Jeremy Linton @ 2020-06-10 11:21 ` Will Deacon 2 siblings, 0 replies; 30+ messages in thread From: Will Deacon @ 2020-06-10 11:21 UTC (permalink / raw) To: Nick Desaulniers, Catalin Marinas Cc: Will Deacon, linux-kernel, stable, Thomas Gleixner, Enrico Weigelt, Ard Biesheuvel, Allison Randal, Lorenzo Pieralisi, Jeremy Linton, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-02 18:46 ` Nick Desaulniers 2020-06-08 14:51 ` Will Deacon @ 2020-06-08 23:20 ` Kaneda, Erik 1 sibling, 0 replies; 30+ messages in thread From: Kaneda, Erik @ 2020-06-08 23:20 UTC (permalink / raw) To: Nick Desaulniers Cc: Moore, Robert, Wysocki, Rafael J, Len Brown, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel > -----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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-01 23:18 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Nick Desaulniers 2020-06-01 23:37 ` Peter Collingbourne 2020-06-02 0:02 ` Kaneda, Erik @ 2020-06-10 23:06 ` Kaneda, Erik 2020-06-10 23:29 ` Nick Desaulniers 2020-06-10 23:31 ` Jung-uk Kim 2 siblings, 2 replies; 30+ messages in thread From: Kaneda, Erik @ 2020-06-10 23:06 UTC (permalink / raw) To: Nick Desaulniers, Moore, Robert, Wysocki, Rafael J, Len Brown, Jung-uk Kim Cc: Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel +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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-10 23:06 ` Kaneda, Erik @ 2020-06-10 23:29 ` Nick Desaulniers 2020-06-10 23:46 ` Jung-uk Kim 2020-06-10 23:31 ` Jung-uk Kim 1 sibling, 1 reply; 30+ messages in thread From: Nick Desaulniers @ 2020-06-10 23:29 UTC (permalink / raw) To: Kaneda, Erik Cc: Moore, Robert, Wysocki, Rafael J, Len Brown, Jung-uk Kim, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-10 23:29 ` Nick Desaulniers @ 2020-06-10 23:46 ` Jung-uk Kim 2020-06-11 16:45 ` [Devel] " Kaneda, Erik 0 siblings, 1 reply; 30+ messages in thread From: Jung-uk Kim @ 2020-06-10 23:46 UTC (permalink / raw) To: Nick Desaulniers, Kaneda, Erik Cc: Moore, Robert, Wysocki, Rafael J, Len Brown, Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-10 23:46 ` Jung-uk Kim @ 2020-06-11 16:45 ` Kaneda, Erik 2020-06-11 17:06 ` Nick Desaulniers 0 siblings, 1 reply; 30+ messages in thread From: Kaneda, Erik @ 2020-06-11 16:45 UTC (permalink / raw) To: Jung-uk Kim, Nick Desaulniers Cc: Wysocki, Rafael J, Ard Biesheuvel, dvyukov, glider, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel > -----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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-11 16:45 ` [Devel] " Kaneda, Erik @ 2020-06-11 17:06 ` Nick Desaulniers 2020-06-16 21:39 ` Kaneda, Erik 0 siblings, 1 reply; 30+ messages in thread From: Nick Desaulniers @ 2020-06-11 17:06 UTC (permalink / raw) To: Kaneda, Erik Cc: Jung-uk Kim, Wysocki, Rafael J, Ard Biesheuvel, dvyukov, glider, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-11 17:06 ` Nick Desaulniers @ 2020-06-16 21:39 ` Kaneda, Erik 0 siblings, 0 replies; 30+ messages in thread From: Kaneda, Erik @ 2020-06-16 21:39 UTC (permalink / raw) To: Nick Desaulniers Cc: Jung-uk Kim, Wysocki, Rafael J, Ard Biesheuvel, dvyukov, glider, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel > -----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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof 2020-06-10 23:06 ` Kaneda, Erik 2020-06-10 23:29 ` Nick Desaulniers @ 2020-06-10 23:31 ` Jung-uk Kim 1 sibling, 0 replies; 30+ messages in thread From: Jung-uk Kim @ 2020-06-10 23:31 UTC (permalink / raw) To: Kaneda, Erik, Nick Desaulniers, Moore, Robert, Wysocki, Rafael J, Len Brown Cc: Ard Biesheuvel, dvyukov, glider, guohanjun, linux-arm-kernel, linux-kernel, lorenzo.pieralisi, mark.rutland, pcc, rjw, will, stable, linux-acpi, devel 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 > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-21 10:09 arm64/acpi: NULL dereference reports from UBSAN at boot Will Deacon 2020-05-21 17:37 ` Lorenzo Pieralisi @ 2020-05-22 8:07 ` Hanjun Guo 2020-05-22 9:43 ` Hanjun Guo 1 sibling, 1 reply; 30+ messages in thread From: Hanjun Guo @ 2020-05-22 8:07 UTC (permalink / raw) To: Will Deacon, lorenzo.pieralisi Cc: rjw, linux-arm-kernel, linux-kernel, mark.rutland 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] ================================================================================ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: arm64/acpi: NULL dereference reports from UBSAN at boot 2020-05-22 8:07 ` arm64/acpi: NULL dereference reports from UBSAN at boot Hanjun Guo @ 2020-05-22 9:43 ` Hanjun Guo 0 siblings, 0 replies; 30+ messages in thread From: Hanjun Guo @ 2020-05-22 9:43 UTC (permalink / raw) To: Will Deacon, lorenzo.pieralisi Cc: rjw, linux-arm-kernel, linux-kernel, mark.rutland 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-06-16 21:39 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-21 10:09 arm64/acpi: NULL dereference reports from UBSAN at boot Will Deacon 2020-05-21 17:37 ` Lorenzo Pieralisi 2020-05-26 20:21 ` Will Deacon 2020-05-27 13:41 ` Lorenzo Pieralisi 2020-06-01 7:05 ` Will Deacon 2020-06-01 21:51 ` Nick Desaulniers 2020-06-01 21:57 ` Ard Biesheuvel 2020-06-01 22:19 ` Nick Desaulniers 2020-06-01 22:28 ` Ard Biesheuvel 2020-06-01 23:18 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Nick Desaulniers 2020-06-01 23:37 ` Peter Collingbourne 2020-06-01 23:48 ` Nick Desaulniers 2020-06-02 0:02 ` Kaneda, Erik 2020-06-02 18:46 ` Nick Desaulniers 2020-06-08 14:51 ` Will Deacon 2020-06-08 20:29 ` Nick Desaulniers 2020-06-08 20:38 ` [PATCH v2] arm64: acpi: fix UBSAN warning Nick Desaulniers 2020-06-09 17:46 ` Lorenzo Pieralisi 2020-06-09 19:50 ` Jeremy Linton 2020-06-10 11:21 ` Will Deacon 2020-06-08 23:20 ` [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof Kaneda, Erik 2020-06-10 23:06 ` Kaneda, Erik 2020-06-10 23:29 ` Nick Desaulniers 2020-06-10 23:46 ` Jung-uk Kim 2020-06-11 16:45 ` [Devel] " Kaneda, Erik 2020-06-11 17:06 ` Nick Desaulniers 2020-06-16 21:39 ` Kaneda, Erik 2020-06-10 23:31 ` Jung-uk Kim 2020-05-22 8:07 ` arm64/acpi: NULL dereference reports from UBSAN at boot Hanjun Guo 2020-05-22 9:43 ` Hanjun Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).