linux-kernel - Re: [PATCH] ACPICA: fix UBSAN warning using __builtin

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]
Message-ID: <CAKwvOdnh6Zh+P9SM_qFiy-9u7Y21fn=byTJtG4fTTRJqqU9bcQ@mail.gmail.com>
Date:   Wed, 10 Jun 2020 16:29:02 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     "Kaneda, Erik" <erik.kaneda@...el.com>
Cc:     "Moore, Robert" <robert.moore@...el.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        Len Brown <lenb@...nel.org>, Jung-uk Kim <jkim@...ebsd.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>,
        "glider@...gle.com" <glider@...gle.com>,
        "guohanjun@...wei.com" <guohanjun@...wei.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "pcc@...gle.com" <pcc@...gle.com>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "will@...nel.org" <will@...nel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "devel@...ica.org" <devel@...ica.org>
Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof

On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <erik.kaneda@...el.com> wrote:
>
> +JKim (for FreeBSD's perspective)
>
> > -----Original Message-----
> > From: Nick Desaulniers <ndesaulniers@...gle.com>
> > Sent: Monday, June 1, 2020 4:18 PM
> > To: Moore, Robert <robert.moore@...el.com>; Kaneda, Erik
> > <erik.kaneda@...el.com>; Wysocki, Rafael J <rafael.j.wysocki@...el.com>;
> > Len Brown <lenb@...nel.org>
> > Cc: Ard Biesheuvel <ardb@...nel.org>; dvyukov@...gle.com;
> > glider@...gle.com; guohanjun@...wei.com; linux-arm-
> > kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> > lorenzo.pieralisi@....com; mark.rutland@....com;
> > ndesaulniers@...gle.com; pcc@...gle.com; rjw@...ysocki.net;
> > will@...nel.org; stable@...r.kernel.org; linux-acpi@...r.kernel.org;
> > devel@...ica.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@...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>
> > ---
> >  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