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

[Ard Biesheuvel]

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