[U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks

[Måns Rullgård]

Simon Glass <sjg at chromium.org> writes:

> Hi Rob,
>
> On 24 October 2014 22:51, Rob Herring <robherring2 at gmail.com> wrote:
>> On Sat, Oct 25, 2014 at 1:56 AM, Simon Glass <sjg at chromium.org> wrote:
>>> +Tom
>>>
>>> Hi Rob,
>>>
>>> On 15 October 2014 03:21, Rob Herring <robherring2 at gmail.com> wrote:
>>>> From: Rob Herring <robh at kernel.org>
>>>>
>>>> Commit f18295d3837c282f (fdt_support: fix an endian bug of
>>>> fdt_fixup_memory_banks) changed fdt_fixup_memory_banks cell writing from a
>>>> byte at a time to casting the buffer pointer to a 64-bit pointer. This can
>>>> result in unaligned accesses when there is a mixture of cell sizes of 1
>>>> and 2.
>>>
>>> Should that be 739a01ed8e0?
>>
>> Uhh, yes.
>>
>>>
>>>>
>>>> Cc: Masahiro Yamada <yamada.m at jp.panasonic.com>
>>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>>> ---
>>>>  common/fdt_support.c | 11 +++++++----
>>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>> index 3f64156..9c41f3a 100644
>>>> --- a/common/fdt_support.c
>>>> +++ b/common/fdt_support.c
>>>> @@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,
>>>>         int i;
>>>>         int address_len = get_cells_len(fdt, "#address-cells");
>>>>         int size_len = get_cells_len(fdt, "#size-cells");
>>>> +       u64 cell;
>>>>         char *p = buf;
>>>>
>>>>         for (i = 0; i < n; i++) {
>>>>                 if (address_len == 8)
>>>> -                       *(fdt64_t *)p = cpu_to_fdt64(address[i]);
>>>> +                       cell = cpu_to_fdt64(address[i]);
>>>>                 else
>>>> -                       *(fdt32_t *)p = cpu_to_fdt32(address[i]);
>>>> +                       cell = cpu_to_fdt32(address[i]);
>>>> +               memcpy(p, &cell, address_len);
>>>>                 p += address_len;
>>>>
>>>>                 if (size_len == 8)
>>>> -                       *(fdt64_t *)p = cpu_to_fdt64(size[i]);
>>>> +                       cell = cpu_to_fdt64(size[i]);
>>>>                 else
>>>> -                       *(fdt32_t *)p = cpu_to_fdt32(size[i]);
>>>> +                       cell = cpu_to_fdt32(size[i]);
>>>> +               memcpy(p, &cell, size_len);
>>>
>>> What will this do with 32-bit values? Aren't use assuming that the
>>> first 32-bit word of 'cell' will contain the least significant 32
>>> bits? Is that always true? I'm not quite sure.
>>
>> We've already done the endian conversion, so we're just copying a
>> string of bytes only changing the alignment potentially.
>
> Yes I think you are right. Then I suggest adding a comment here, as
> memcpy() from a native type is unusual.

Better yet, use put_unaligned().

-- 
Måns Rullgård
mans at mansr.com