[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] loader: Fix misaligned member access
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v4] loader: Fix misaligned member access |
Date: |
Wed, 25 Apr 2018 16:29:08 +1000 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
On Tue, Apr 24, 2018 at 07:21:03PM -0300, Philippe Mathieu-Daudé wrote:
> The libfdt does not guarantee than fdt_getprop() returns a pointer
> aligned to the property size.
>
> Assuming the base of the fdt is aligned, a 32-bit property returns
> a 32-bit aligned pointer. This is however not guaranteed for 64-bit
> properties, where 64-bit loads might trigger unaligned access.
>
> Fix the 64-bit access using the ldst (host) API, which uses a local
> copy on the stack, thus guaranteeing a safe aligned access.
>
> This fixes the following ASan warning:
>
> $ 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
> Suggested-by: Peter Maydell <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
Reviewed-by: David Gibson <address@hidden>
I'll look at adding a hlper for this to libfdt, but no promises on timeframe.
> ---
> v4: do not change the 32-bit access, use ldq_he_p() for the 64-bit access
> v3: do not use memcpy(), incorrectly change ldl_he_p()
> v2: do not change the 32-bit access, use memcpy(), add comments (David Gibson)
> v1: use memcpy()
>
> hw/core/loader-fit.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 0c4a7207f4..ed4140061b 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -102,10 +102,17 @@ static int fit_image_addr(const void *itb, int img,
> const char *name,
>
> switch (len) {
> case 4:
> + /* Assuming the base of the fdt is aligned, then fdt_getprop()
> + * returns 32-bit aligned properties, so this load is guaranteed
> + * to be 32-bit aligned.
> + */
> *addr = fdt32_to_cpu(*(fdt32_t *)prop);
> return 0;
> case 8:
> - *addr = fdt64_to_cpu(*(fdt64_t *)prop);
> + /* Since the property is not guaranteed to be 64-bit aligned,
> + * use ldq_he_p()'s stack to avoid an unaligned load.
> + */
> + *addr = fdt64_to_cpu(ldq_he_p(prop));
> return 0;
> default:
> error_printf("invalid %s address length %d\n", name, len);
--
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
signature.asc
Description: PGP signature