linux-kernel - Re: [PATCH v2] arm64: acpi: fix UBSAN warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 9 Jun 2020 14:50:36 -0500
From:   Jeremy Linton <jeremy.linton@....com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>
Cc:     stable@...r.kernel.org, Ard Biesheuvel <ardb@...nel.org>,
        Enrico Weigelt <info@...ux.net>,
        Allison Randal <allison@...utok.net>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: acpi: fix UBSAN warning

Hi,

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

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

But either way, this change looks fine too.

Reviewed-by: Jeremy Linton <jeremy.linton@....com>

Thanks,

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