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

[Peter Maydell]

On 23 April 2018 at 16:49, Philippe Mathieu-Daudé  wrote:
> I think I never noticed ldq_he_p(), good to know.
>
> $ git grep -E '(ld|st)._he_'
> net/checksum.c:130:stw_he_p(>th_sum, 0);
> net/checksum.c:151:stw_he_p(>uh_sum, 0);
> util/bufferiszero.c:47:uint64_t t = ldq_he_p(buf);
> util/bufferiszero.c:61:t |= ldq_he_p(buf + len - 8);
>
> Not many users...

Well, it's fairly rare to have something that you want
to load in host endianness that isn't also aligned as
much as the host requires for it. We only want it here
because libfdt doesn't provide a ldq_fdt or something
like it ought to.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 04/23/2018 12:32 PM, Peter Maydell wrote:
> On 23 April 2018 at 15:26, Philippe Mathieu-Daudé  wrote:
>>> On 04/23/2018 11:04 AM, Peter Maydell wrote:
 On 23 April 2018 at 14:57, Philippe Mathieu-Daudé  wrote:
> On 04/23/2018 12:16 AM, David Gibson wrote:
>> On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
>>> 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.

  *addr = fdt32_to_cpu(ldl_p(prop));

 is better than a raw memcpy still.
>>
>> ldl_p() is target-specific, I'd prefer loader code to be target agnostic.
>>
>> Since FDT is big-endian, are you OK I use, as you suggested,
>>
>> *addr = ldq_be_p(prop);
>>
>> (with a comment about FDT being BE)?
> 
> Oops, yes, forgot that ldq_p is the target-endian version.
> ldq_he_p() is the "load in host endianness" function, so
>*addr = fdt64_to_cpu(ldq_he_p(prop));

I think I never noticed ldq_he_p(), good to know.

$ git grep -E '(ld|st)._he_'
net/checksum.c:130:stw_he_p(>th_sum, 0);
net/checksum.c:151:stw_he_p(>uh_sum, 0);
util/bufferiszero.c:47:uint64_t t = ldq_he_p(buf);
util/bufferiszero.c:61:t |= ldq_he_p(buf + len - 8);

Not many users...

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

[Peter Maydell]

On 23 April 2018 at 15:26, Philippe Mathieu-Daudé  wrote:
>> On 04/23/2018 11:04 AM, Peter Maydell wrote:
>>> On 23 April 2018 at 14:57, Philippe Mathieu-Daudé  wrote:
 On 04/23/2018 12:16 AM, David Gibson wrote:
> On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
>> 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.
>>>
>>>  *addr = fdt32_to_cpu(ldl_p(prop));
>>>
>>> is better than a raw memcpy still.
>
> ldl_p() is target-specific, I'd prefer loader code to be target agnostic.
>
> Since FDT is big-endian, are you OK I use, as you suggested,
>
> *addr = ldq_be_p(prop);
>
> (with a comment about FDT being BE)?

Oops, yes, forgot that ldq_p is the target-endian version.
ldq_he_p() is the "load in host endianness" function, so
   *addr = fdt64_to_cpu(ldq_he_p(prop));

Sorry for the confusion.

-- PMM

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

[Philippe Mathieu-Daudé]

> On 04/23/2018 11:04 AM, Peter Maydell wrote:
>> On 23 April 2018 at 14:57, Philippe Mathieu-Daudé  wrote:
>>> On 04/23/2018 12:16 AM, David Gibson wrote:
 On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
> 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.
>>
>>  *addr = fdt32_to_cpu(ldl_p(prop));
>>
>> is better than a raw memcpy still.

ldl_p() is target-specific, I'd prefer loader code to be target agnostic.

Since FDT is big-endian, are you OK I use, as you suggested,

*addr = ldq_be_p(prop);

(with a comment about FDT being BE)?

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

[Philippe Mathieu-Daudé]

On 04/23/2018 11:04 AM, Peter Maydell wrote:
> On 23 April 2018 at 14:57, Philippe Mathieu-Daudé  wrote:
>> On 04/23/2018 12:16 AM, David Gibson wrote:
>>> On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
 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.
> 
>  *addr = fdt32_to_cpu(ldl_p(prop));
> 
> is better than a raw memcpy still.

Oops I was in a console sending a v2, I'll go for a v3.

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

[Peter Maydell]

On 23 April 2018 at 14:57, Philippe Mathieu-Daudé  wrote:
> On 04/23/2018 12:16 AM, David Gibson wrote:
>> On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
>>> 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.

 *addr = fdt32_to_cpu(ldl_p(prop));

is better than a raw memcpy still.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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é  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é 
>>> ---
>>>  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, );
>>> @@ -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(, 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

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

[David Gibson]

On Sun, Apr 22, 2018 at 11:41:20AM +0100, Peter Maydell wrote:
> On 21 April 2018 at 22:16, Philippe Mathieu-Daudé  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é 
> > ---
> >  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, );
> > @@ -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(, 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).
> 
> 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").

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

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

[Peter Maydell]

On 21 April 2018 at 22:16, Philippe Mathieu-Daudé  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é 
> ---
>  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, );
> @@ -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(, 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