Re: [Qemu-devel] [PATCH] loader: Fix misaligned member access

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3326 bytes --]

On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
> On 21 April 2018 at 22:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > This fixes the following ASan warning:
> >
> >   $ mips64el-softmmu/qemu-system-mips64el -M boston -kernel vmlinux.gz.itb -nographic
> >   hw/core/loader-fit.c:108:17: runtime error: load of misaligned address 0x7f95cd7e4264 for type 'fdt64_t', which requires 8 byte alignment
> >   0x7f95cd7e4264: note: pointer points here
> >     00 00 00 3e ff ff ff ff  80 7d 2a c0 00 00 00 01  68 61 73 68 40 30 00 00  00 00 00 03 00 00 00 14
> >                 ^
> >
> > Reported-by: AddressSanitizer
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/core/loader-fit.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> > index 0c4a7207f4..1a69697f89 100644
> > --- a/hw/core/loader-fit.c
> > +++ b/hw/core/loader-fit.c
> > @@ -93,6 +93,8 @@ static int fit_image_addr(const void *itb, int img, const char *name,
> >                            hwaddr *addr)
> >  {
> >      const void *prop;
> > +    fdt32_t v32;
> > +    fdt64_t v64;
> >      int len;
> >
> >      prop = fdt_getprop(itb, img, name, &len);
> > @@ -102,10 +104,12 @@ static int fit_image_addr(const void *itb, int img, const char *name,
> >
> >      switch (len) {
> >      case 4:
> > -        *addr = fdt32_to_cpu(*(fdt32_t *)prop);
> > +        memcpy(&v32, prop, sizeof(v32));
> > +        *addr = fdt32_to_cpu(v32);

So, assuming the base of the fdt is aligned (which I'd expect), then
properties should also be 32-bit aligned, so this shouldn't be
necessary.  They may not be 64-bit aligned, so the equivalent for
length 8 *is* required.

(Really old fdt versions did attempt to 64-bit align larger
properties, but it caused a bunch of other complications)

> If we need to do an unaligned load, then ldl_p() is the
> right way to do it. (We could also just do
>  *addr = ldl_be_p(prop) but we maybe don't want to
> bake in knowledge that FDT is big-endian).
> 
> This does make me suspicious that maybe we're not using
> the fdt APIs right here though. Since 'prop' is the return
> value of fdt_getprop(), shouldn't libfdt be providing
> APIs for "give me the fdt32 here" that don't require
> the libfdt user to either implement its own unaligned
> access helpers or invoke C undefined behaviour? David,
> any suggestions?

It's pretty much correct usage.  Properties in fdt - as in Open
Firmware - are defined to be bytestrings.  libfdt is concerned with
extracting those bytestrings from the tree encoding, and parsing
what's inside the bytestring is mostly out of scope for it.

Mostly.. because we do have some helpers for common cases -
e.g. fdt_setprop_u32().  We don't currently have an fdt_getprop_u32()
or fdt_getprop_u64().  I'm not in principle opposed to adding them,
but the right interface isn't immediately obvious to me: we can't just
return a u32/u64, because then we have no way of reporting errors
(like "no such property").

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]