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é <f4...@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é <f4...@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).
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