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

[Philippe Mathieu-Daudé]

On 04/23/2018 12:16 AM, David Gibson wrote:
> On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
>> On 21 April 2018 at 22:16, Philippe Mathieu-Daudé <address@hidden> 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é <address@hidden>
>>> ---
>>>  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).

Since it is, ldl_be_p() seems the clever/cleaner way indeed, but then we
assume we know the underlying type of fdt32_t; while using memcpy we
respect the FDT API.

>> 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").

At the time of the fdt_getprop() call we don't know if the property is
u32/u64.

signature.asc
Description: OpenPGP digital signature