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

[Peter Maydell <peter.maydell@linaro.org>]

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);

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?

(Similarly with the fdt64 access and ldq_p.)

thanks
-- PMM