[Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access

[Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access

[Philippe Mathieu-Daudé]

This series fixes an unaligned of a 64-bit FDT property in the FIT loader.

Since v2:
- Use the ldst API
- Let ldl_he_p() returns unsigned
- Use the same API for handling 32/64-bit FDT properties

Philippe Mathieu-Daudé (2):
  bswap.h: Fix ldl_he_p() signedness
  loader: Fix 64-bit misaligned member access

 include/qemu/bswap.h | 4 ++--
 hw/core/loader-fit.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness

[Philippe Mathieu-Daudé]

As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
"No signed load operations are provided."
Update lduw_he_p() to return as unsigned.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/bswap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 3f28f661b1..613978f838 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
     memcpy(ptr, &v, sizeof(v));
 }
 
-static inline int ldl_he_p(const void *ptr)
+static inline uint32_t ldl_he_p(const void *ptr)
 {
-    int32_t r;
+    uint32_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
-- 
2.17.0

Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness

[Peter Maydell]

On 23 April 2018 at 17:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."

That phrase is used in the documentation sections for other
kinds of load/store function, but not in the section for ld*_p
and st*_p, which do provide both signed and unsigned flavours.

> Update lduw_he_p() to return as unsigned.

Code is changing a different function to the one named here...

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)

This would make it not match the other ldl*_p functions
(ldl_le_p, ldl_be_p).

The expectation with the ldl functions is that you're
putting it into a variable of the right type and size,
and so we don't need to provide both a signed 32 bit load
and an unsigned 32 bit load.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness

[Philippe Mathieu-Daudé]

On 04/23/2018 01:56 PM, Peter Maydell wrote:
> On 23 April 2018 at 17:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
>> "No signed load operations are provided."
> 
> That phrase is used in the documentation sections for other
> kinds of load/store function, but not in the section for ld*_p
> and st*_p, which do provide both signed and unsigned flavours.
> 
>> Update lduw_he_p() to return as unsigned.
> 
> Code is changing a different function to the one named here...
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/qemu/bswap.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>> index 3f28f661b1..613978f838 100644
>> --- a/include/qemu/bswap.h
>> +++ b/include/qemu/bswap.h
>> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>>      memcpy(ptr, &v, sizeof(v));
>>  }
>>
>> -static inline int ldl_he_p(const void *ptr)
>> +static inline uint32_t ldl_he_p(const void *ptr)
> 
> This would make it not match the other ldl*_p functions
> (ldl_le_p, ldl_be_p).
> 
> The expectation with the ldl functions is that you're
> putting it into a variable of the right type and size,
> and so we don't need to provide both a signed 32 bit load
> and an unsigned 32 bit load.

OK, thank you for the clarification.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness

[Richard Henderson]

On 04/23/2018 06:25 AM, Philippe Mathieu-Daudé wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."
> Update lduw_he_p() to return as unsigned.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>  
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)
>  {
> -    int32_t r;
> +    uint32_t r;

Nack, not without auditing all users.

The documentation is clear about ldl being an oddball,
primarily because we didn't audit all users last time.

In the short term, just cast your user, which is what
we wind up doing elsewhere in the codebase.

r~

Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness

[Paolo Bonzini]

On 23/04/2018 18:25, Philippe Mathieu-Daudé wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."
> Update lduw_he_p() to return as unsigned.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

This is ldl_he_p, not lduw_he_p.

However, this patch should not be necessary; using uint16_t is needed
for shorts because they implicitly extend to int, so a "short -0x1234"
becomes 0xffff1234 when assigned to uint32_t.  Instead, an int can be
assigned to a uint32_t (which is the type of fdt32_to_cpu's argument)
with no change in value.

Paolo

> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>  
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)
>  {
> -    int32_t r;
> +    uint32_t r;
>      memcpy(&r, ptr, sizeof(r));
>      return r;
>  }
> 

[Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access

[Philippe Mathieu-Daudé]

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 this issue by using the ldst (host) API, which does a local copy
on the stack, thus guaranteeing a safe aligned access.

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
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/loader-fit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 0c4a7207f4..628f854636 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -102,10 +102,10 @@ static int fit_image_addr(const void *itb, int img, const char *name,
 
     switch (len) {
     case 4:
-        *addr = fdt32_to_cpu(*(fdt32_t *)prop);
+        *addr = fdt32_to_cpu(ldl_he_p(prop));
         return 0;
     case 8:
-        *addr = fdt64_to_cpu(*(fdt64_t *)prop);
+        *addr = fdt64_to_cpu(ldq_he_p(prop));
         return 0;
     default:
         error_printf("invalid %s address length %d\n", name, len);
-- 
2.17.0

Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access

[Richard Henderson]

On 04/23/2018 06:25 AM, Philippe Mathieu-Daudé wrote:
> Assuming the base of the fdt is aligned, a 32-bit property returns
> a 32-bit aligned pointer...
...
>      case 4:
> -        *addr = fdt32_to_cpu(*(fdt32_t *)prop);
> +        *addr = fdt32_to_cpu(ldl_he_p(prop));
>          return 0;

So why are you changing the 32-bit case?


r~

Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access

[Philippe Mathieu-Daudé]

On 04/23/2018 02:34 PM, Richard Henderson wrote:
> On 04/23/2018 06:25 AM, Philippe Mathieu-Daudé wrote:
>> Assuming the base of the fdt is aligned, a 32-bit property returns
>> a 32-bit aligned pointer...
> ...
>>      case 4:
>> -        *addr = fdt32_to_cpu(*(fdt32_t *)prop);
>> +        *addr = fdt32_to_cpu(ldl_he_p(prop));
>>          return 0;
> 
> So why are you changing the 32-bit case?

First to not assume base fdt is always 32-bit aligned,
second to keep calls similar and avoid someone use *(fdt64_t *) similar
to the 32-bit property. But this is avoidable with a simple comment
about why we have to use ldq_he_p().

Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access

[Paolo Bonzini]

On 23/04/2018 18:25, 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 this issue by using the ldst (host) API, which does a local copy
> on the stack, thus guaranteeing a safe aligned access.
> 
> 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
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/loader-fit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 0c4a7207f4..628f854636 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -102,10 +102,10 @@ static int fit_image_addr(const void *itb, int img, const char *name,
>  
>      switch (len) {
>      case 4:
> -        *addr = fdt32_to_cpu(*(fdt32_t *)prop);
> +        *addr = fdt32_to_cpu(ldl_he_p(prop));
>          return 0;
>      case 8:
> -        *addr = fdt64_to_cpu(*(fdt64_t *)prop);
> +        *addr = fdt64_to_cpu(ldq_he_p(prop));
>          return 0;
>      default:
>          error_printf("invalid %s address length %d\n", name, len);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>