* [Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access @ 2018-04-23 16:25 Philippe Mathieu-Daudé 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access Philippe Mathieu-Daudé 0 siblings, 2 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-04-23 16:25 UTC (permalink / raw) To: David Gibson, Peter Maydell Cc: Philippe Mathieu-Daudé, qemu-devel, Paul Burton, Marc-André Lureau 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness 2018-04-23 16:25 [Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access Philippe Mathieu-Daudé @ 2018-04-23 16:25 ` Philippe Mathieu-Daudé 2018-04-23 16:56 ` Peter Maydell ` (2 more replies) 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access Philippe Mathieu-Daudé 1 sibling, 3 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-04-23 16:25 UTC (permalink / raw) To: David Gibson, Peter Maydell Cc: Philippe Mathieu-Daudé, qemu-devel, Richard Henderson 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé @ 2018-04-23 16:56 ` Peter Maydell 2018-04-24 22:18 ` Philippe Mathieu-Daudé 2018-04-23 17:32 ` Richard Henderson 2018-05-25 14:06 ` Paolo Bonzini 2 siblings, 1 reply; 10+ messages in thread From: Peter Maydell @ 2018-04-23 16:56 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: David Gibson, QEMU Developers, Richard Henderson 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness 2018-04-23 16:56 ` Peter Maydell @ 2018-04-24 22:18 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-04-24 22:18 UTC (permalink / raw) To: Peter Maydell; +Cc: Richard Henderson, QEMU Developers, David Gibson 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé 2018-04-23 16:56 ` Peter Maydell @ 2018-04-23 17:32 ` Richard Henderson 2018-05-25 14:06 ` Paolo Bonzini 2 siblings, 0 replies; 10+ messages in thread From: Richard Henderson @ 2018-04-23 17:32 UTC (permalink / raw) To: Philippe Mathieu-Daudé, David Gibson, Peter Maydell; +Cc: qemu-devel 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~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé 2018-04-23 16:56 ` Peter Maydell 2018-04-23 17:32 ` Richard Henderson @ 2018-05-25 14:06 ` Paolo Bonzini 2 siblings, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2018-05-25 14:06 UTC (permalink / raw) To: Philippe Mathieu-Daudé, David Gibson, Peter Maydell Cc: Richard Henderson, qemu-devel 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; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access 2018-04-23 16:25 [Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access Philippe Mathieu-Daudé 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé @ 2018-04-23 16:25 ` Philippe Mathieu-Daudé 2018-04-23 17:34 ` Richard Henderson 2018-05-25 14:07 ` Paolo Bonzini 1 sibling, 2 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-04-23 16:25 UTC (permalink / raw) To: David Gibson, Peter Maydell Cc: Philippe Mathieu-Daudé, qemu-devel, Paul Burton, Marc-André Lureau 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access Philippe Mathieu-Daudé @ 2018-04-23 17:34 ` Richard Henderson 2018-04-23 17:38 ` Philippe Mathieu-Daudé 2018-05-25 14:07 ` Paolo Bonzini 1 sibling, 1 reply; 10+ messages in thread From: Richard Henderson @ 2018-04-23 17:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé, David Gibson, Peter Maydell Cc: Marc-André Lureau, Paul Burton, qemu-devel 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~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access 2018-04-23 17:34 ` Richard Henderson @ 2018-04-23 17:38 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-04-23 17:38 UTC (permalink / raw) To: Richard Henderson, David Gibson, Peter Maydell Cc: Marc-André Lureau, Paul Burton, qemu-devel 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(). ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access Philippe Mathieu-Daudé 2018-04-23 17:34 ` Richard Henderson @ 2018-05-25 14:07 ` Paolo Bonzini 1 sibling, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2018-05-25 14:07 UTC (permalink / raw) To: Philippe Mathieu-Daudé, David Gibson, Peter Maydell Cc: Marc-André Lureau, Paul Burton, qemu-devel 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-25 14:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-23 16:25 [Qemu-devel] [PATCH v3 0/2] loader: Fix misaligned member access Philippe Mathieu-Daudé 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 1/2] bswap.h: Fix ldl_he_p() signedness Philippe Mathieu-Daudé 2018-04-23 16:56 ` Peter Maydell 2018-04-24 22:18 ` Philippe Mathieu-Daudé 2018-04-23 17:32 ` Richard Henderson 2018-05-25 14:06 ` Paolo Bonzini 2018-04-23 16:25 ` [Qemu-devel] [PATCH v3 2/2] loader: Fix 64-bit misaligned member access Philippe Mathieu-Daudé 2018-04-23 17:34 ` Richard Henderson 2018-04-23 17:38 ` Philippe Mathieu-Daudé 2018-05-25 14:07 ` Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.