[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

There is no reason to tail-pad fitImage with external data to 4-bytes,
while fitImage without external data does not have any such padding and
is often unaligned. DT spec also does not mandate any such padding.

Moreover, the tail-pad fills the last few bytes with uninitialized data,
which could lead to a potential information leak.

$ echo -n xy > /tmp/data ; \
	./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
	hexdump -vC /tmp/fitImage | tail -n 3

before:
00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
                   ^^       ^^ ^^
after:
00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
00000270  7a 65 00 78 79                                    |ze.xy|

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: Rebase on u-boot/master, update commit message
---
 tools/fit_image.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 4aeabbcfe9..b859e7c0c0 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -434,7 +434,7 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname)
 	int image_number;
 	int align_size;
 
-	align_size = params->bl_len ? params->bl_len : 4;
+	align_size = params->bl_len ? params->bl_len : 1;
 	fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
 	if (fd < 0)
 		return -EIO;
@@ -492,7 +492,6 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname)
 	fdt_pack(fdt);
 
 	new_size = fdt_totalsize(fdt);
-	new_size = ALIGN(new_size, align_size);
 	fdt_set_totalsize(fdt, new_size);
 	debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
 	debug("External data size %x\n", buf_ptr);
-- 
2.25.1

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Simon Glass]

On Fri, 1 May 2020 at 09:40, Marek Vasut <marex@denx.de> wrote:
>
> There is no reason to tail-pad fitImage with external data to 4-bytes,
> while fitImage without external data does not have any such padding and
> is often unaligned. DT spec also does not mandate any such padding.
>
> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> which could lead to a potential information leak.
>
> $ echo -n xy > /tmp/data ; \
>         ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>         hexdump -vC /tmp/fitImage | tail -n 3
>
> before:
> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>                    ^^       ^^ ^^
> after:
> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> 00000270  7a 65 00 78 79                                    |ze.xy|
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: Rebase on u-boot/master, update commit message
> ---
>  tools/fit_image.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:

> There is no reason to tail-pad fitImage with external data to 4-bytes,
> while fitImage without external data does not have any such padding and
> is often unaligned. DT spec also does not mandate any such padding.
> 
> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> which could lead to a potential information leak.
> 
> $ echo -n xy > /tmp/data ; \
> 	./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> 	hexdump -vC /tmp/fitImage | tail -n 3
> 
> before:
> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>                    ^^       ^^ ^^
> after:
> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> 00000270  7a 65 00 78 79                                    |ze.xy|
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200504/3c4576ea/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Alex Kiernan]

On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>
> > There is no reason to tail-pad fitImage with external data to 4-bytes,
> > while fitImage without external data does not have any such padding and
> > is often unaligned. DT spec also does not mandate any such padding.
> >
> > Moreover, the tail-pad fills the last few bytes with uninitialized data,
> > which could lead to a potential information leak.
> >
> > $ echo -n xy > /tmp/data ; \
> >       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >       hexdump -vC /tmp/fitImage | tail -n 3
> >
> > before:
> > 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >                    ^^       ^^ ^^
> > after:
> > 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > 00000270  7a 65 00 78 79                                    |ze.xy|
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Tom Rini <trini@konsulko.com>
>
> Applied to u-boot/master, thanks!
>

This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
from eMMC I get nothing at all on the console, if I boot over ymodem
it stalls at 420k, before continuing to 460k. My guess is there's some
error going to the console at the 420k mark, but obviously it's lost
in the ymodem... I have two DTBs in the FIT image, 420k would about
align to the point between them.

-- 
Alex Kiernan

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/5/20 3:22 PM, Alex Kiernan wrote:
> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>
>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>
>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>> while fitImage without external data does not have any such padding and
>>> is often unaligned. DT spec also does not mandate any such padding.
>>>
>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>> which could lead to a potential information leak.
>>>
>>> $ echo -n xy > /tmp/data ; \
>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>
>>> before:
>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>                    ^^       ^^ ^^
>>> after:
>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Cc: Tom Rini <trini@konsulko.com>
>>
>> Applied to u-boot/master, thanks!
>>
> 
> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> from eMMC I get nothing at all on the console, if I boot over ymodem
> it stalls at 420k, before continuing to 460k. My guess is there's some
> error going to the console at the 420k mark, but obviously it's lost
> in the ymodem... I have two DTBs in the FIT image, 420k would about
> align to the point between them.

My bet would be on some padding / unaligned access problem that this
patch uncovered. Can you take a look ?

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Alex Kiernan]

On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>
> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> > On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>
> >>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>> while fitImage without external data does not have any such padding and
> >>> is often unaligned. DT spec also does not mandate any such padding.
> >>>
> >>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>> which could lead to a potential information leak.
> >>>
> >>> $ echo -n xy > /tmp/data ; \
> >>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>
> >>> before:
> >>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>                    ^^       ^^ ^^
> >>> after:
> >>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> Cc: Tom Rini <trini@konsulko.com>
> >>
> >> Applied to u-boot/master, thanks!
> >>
> >
> > This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> > CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> > from eMMC I get nothing at all on the console, if I boot over ymodem
> > it stalls at 420k, before continuing to 460k. My guess is there's some
> > error going to the console at the 420k mark, but obviously it's lost
> > in the ymodem... I have two DTBs in the FIT image, 420k would about
> > align to the point between them.
>
> My bet would be on some padding / unaligned access problem that this
> patch uncovered. Can you take a look ?

Seems plausible. With this change my external data starts at 0x483 and
everything after it is non-aligned:

/dts-v1/;
// magic: 0xd00dfeed
// totalsize: 0x483 (1155)
// off_dt_struct: 0x38
// off_dt_strings: 0x3f8
// off_mem_rsvmap: 0x28
// version: 17
// last_comp_version: 2
// boot_cpuid_phys: 0x0
// size_dt_strings: 0x8b
// size_dt_struct: 0x3c0


-- 
Alex Kiernan

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/5/20 6:37 PM, Alex Kiernan wrote:
> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>
>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>> while fitImage without external data does not have any such padding and
>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>
>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>> which could lead to a potential information leak.
>>>>>
>>>>> $ echo -n xy > /tmp/data ; \
>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>
>>>>> before:
>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>                    ^^       ^^ ^^
>>>>> after:
>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>
>>>> Applied to u-boot/master, thanks!
>>>>
>>>
>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>> error going to the console at the 420k mark, but obviously it's lost
>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>> align to the point between them.
>>
>> My bet would be on some padding / unaligned access problem that this
>> patch uncovered. Can you take a look ?
> 
> Seems plausible. With this change my external data starts at 0x483 and
> everything after it is non-aligned:

Should the beginning of external data be aligned ?

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>
> >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>
> >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>> while fitImage without external data does not have any such padding and
> >>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>
> >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>> which could lead to a potential information leak.
> >>>>>
> >>>>> $ echo -n xy > /tmp/data ; \
> >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>
> >>>>> before:
> >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>                    ^^       ^^ ^^
> >>>>> after:
> >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>
> >>>> Applied to u-boot/master, thanks!
> >>>>
> >>>
> >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>> error going to the console at the 420k mark, but obviously it's lost
> >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>> align to the point between them.
> >>
> >> My bet would be on some padding / unaligned access problem that this
> >> patch uncovered. Can you take a look ?
> > 
> > Seems plausible. With this change my external data starts at 0x483 and
> > everything after it is non-aligned:
> 
> Should the beginning of external data be aligned ?

If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
problem go away?  If so, that's not a fix outright, it means we need to
dig back in to the libfdt thread and find the "make this work without
killing performance everywhere all the time" option.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200505/024e1f66/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/5/20 7:50 PM, Tom Rini wrote:
> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>
>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>
>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>
>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>> which could lead to a potential information leak.
>>>>>>>
>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>
>>>>>>> before:
>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>                    ^^       ^^ ^^
>>>>>>> after:
>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>
>>>>>> Applied to u-boot/master, thanks!
>>>>>>
>>>>>
>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>> align to the point between them.
>>>>
>>>> My bet would be on some padding / unaligned access problem that this
>>>> patch uncovered. Can you take a look ?
>>>
>>> Seems plausible. With this change my external data starts at 0x483 and
>>> everything after it is non-aligned:
>>
>> Should the beginning of external data be aligned ?
> 
> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
> problem go away?  If so, that's not a fix outright, it means we need to
> dig back in to the libfdt thread and find the "make this work without
> killing performance everywhere all the time" option.

Still, my question is, should the beginning of external data be aligned
? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for
arm64/rv64 ?

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
> On 5/5/20 7:50 PM, Tom Rini wrote:
> > On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>
> >>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>
> >>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>
> >>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>> which could lead to a potential information leak.
> >>>>>>>
> >>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>
> >>>>>>> before:
> >>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>                    ^^       ^^ ^^
> >>>>>>> after:
> >>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>
> >>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>
> >>>>>> Applied to u-boot/master, thanks!
> >>>>>>
> >>>>>
> >>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>> align to the point between them.
> >>>>
> >>>> My bet would be on some padding / unaligned access problem that this
> >>>> patch uncovered. Can you take a look ?
> >>>
> >>> Seems plausible. With this change my external data starts at 0x483 and
> >>> everything after it is non-aligned:
> >>
> >> Should the beginning of external data be aligned ?
> > 
> > If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
> > problem go away?  If so, that's not a fix outright, it means we need to
> > dig back in to the libfdt thread and find the "make this work without
> > killing performance everywhere all the time" option.
> 
> Still, my question is, should the beginning of external data be aligned
> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for
> arm64/rv64 ?

Is "external data" the kernel in this case?  If so, I swear Linux
mandates 8 byte alignment for arm32 as well.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200505/aff188ed/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/5/20 7:55 PM, Tom Rini wrote:
> On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
>> On 5/5/20 7:50 PM, Tom Rini wrote:
>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>
>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>
>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>
>>>>>>>>> before:
>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>> after:
>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>
>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>
>>>>>>>
>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>> align to the point between them.
>>>>>>
>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>> patch uncovered. Can you take a look ?
>>>>>
>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>> everything after it is non-aligned:
>>>>
>>>> Should the beginning of external data be aligned ?
>>>
>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
>>> problem go away?  If so, that's not a fix outright, it means we need to
>>> dig back in to the libfdt thread and find the "make this work without
>>> killing performance everywhere all the time" option.
>>
>> Still, my question is, should the beginning of external data be aligned
>> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for
>> arm64/rv64 ?
> 
> Is "external data" the kernel in this case?  If so, I swear Linux
> mandates 8 byte alignment for arm32 as well.

External data can be anything, and if it is supposed to be 8 bytes, we
already failed at that since forever.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote:
> On 5/5/20 7:55 PM, Tom Rini wrote:
> > On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
> >> On 5/5/20 7:50 PM, Tom Rini wrote:
> >>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>
> >>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>
> >>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>
> >>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>
> >>>>>>>>> before:
> >>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>> after:
> >>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>
> >>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>
> >>>>>>>
> >>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>> align to the point between them.
> >>>>>>
> >>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>> patch uncovered. Can you take a look ?
> >>>>>
> >>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>> everything after it is non-aligned:
> >>>>
> >>>> Should the beginning of external data be aligned ?
> >>>
> >>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
> >>> problem go away?  If so, that's not a fix outright, it means we need to
> >>> dig back in to the libfdt thread and find the "make this work without
> >>> killing performance everywhere all the time" option.
> >>
> >> Still, my question is, should the beginning of external data be aligned
> >> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for
> >> arm64/rv64 ?
> > 
> > Is "external data" the kernel in this case?  If so, I swear Linux
> > mandates 8 byte alignment for arm32 as well.
> 
> External data can be anything, and if it is supposed to be 8 bytes, we
> already failed at that since forever.

I would be entirely unsurprised at things working through a combination
of luck and co-incidence in our previous padding working out.  So, what
typically is "external data" in this context?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200505/7f581422/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/5/20 8:06 PM, Tom Rini wrote:
> On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote:
>> On 5/5/20 7:55 PM, Tom Rini wrote:
>>> On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
>>>> On 5/5/20 7:50 PM, Tom Rini wrote:
>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>
>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>
>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>
>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>
>>>>>>>>>>> before:
>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>> after:
>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>
>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>> align to the point between them.
>>>>>>>>
>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>
>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>> everything after it is non-aligned:
>>>>>>
>>>>>> Should the beginning of external data be aligned ?
>>>>>
>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
>>>>> problem go away?  If so, that's not a fix outright, it means we need to
>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>> killing performance everywhere all the time" option.
>>>>
>>>> Still, my question is, should the beginning of external data be aligned
>>>> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for
>>>> arm64/rv64 ?
>>>
>>> Is "external data" the kernel in this case?  If so, I swear Linux
>>> mandates 8 byte alignment for arm32 as well.
>>
>> External data can be anything, and if it is supposed to be 8 bytes, we
>> already failed at that since forever.
> 
> I would be entirely unsurprised at things working through a combination
> of luck and co-incidence in our previous padding working out.  So, what
> typically is "external data" in this context?

Anything you can bundle into the fitImage, -E just puts the content at
the end of the fitImage and places a reference into the fitImage itself.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Simon Glass]

Hi Tom,

On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> > On 5/5/20 6:37 PM, Alex Kiernan wrote:
> > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> > >>
> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> > >>>>
> > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> > >>>>
> > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> > >>>>> while fitImage without external data does not have any such padding and
> > >>>>> is often unaligned. DT spec also does not mandate any such padding.
> > >>>>>
> > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> > >>>>> which could lead to a potential information leak.
> > >>>>>
> > >>>>> $ echo -n xy > /tmp/data ; \
> > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> > >>>>>
> > >>>>> before:
> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> > >>>>>                    ^^       ^^ ^^
> > >>>>> after:
> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> > >>>>>
> > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >>>>> Cc: Tom Rini <trini@konsulko.com>
> > >>>>
> > >>>> Applied to u-boot/master, thanks!
> > >>>>
> > >>>
> > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
> > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
> > >>> error going to the console at the 420k mark, but obviously it's lost
> > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> > >>> align to the point between them.
> > >>
> > >> My bet would be on some padding / unaligned access problem that this
> > >> patch uncovered. Can you take a look ?
> > >
> > > Seems plausible. With this change my external data starts at 0x483 and
> > > everything after it is non-aligned:
> >
> > Should the beginning of external data be aligned ?
>
> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
> problem go away?  If so, that's not a fix outright, it means we need to
> dig back in to the libfdt thread and find the "make this work without
> killing performance everywhere all the time" option.

If it is a device tree, it must be 32-bit aligned.

But Marek's patch affects the FIT image itself, so I am not sure what
would go after that.

Regards,
Simon

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Michael Walle]

Hi all,

Am 2020-05-05 20:41, schrieb Simon Glass:
> Hi Tom,
> 
> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>> 
>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>> > On 5/5/20 6:37 PM, Alex Kiernan wrote:
>> > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>> > >>
>> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>> > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>> > >>>>
>> > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>> > >>>>
>> > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>> > >>>>> while fitImage without external data does not have any such padding and
>> > >>>>> is often unaligned. DT spec also does not mandate any such padding.
>> > >>>>>
>> > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>> > >>>>> which could lead to a potential information leak.
>> > >>>>>
>> > >>>>> $ echo -n xy > /tmp/data ; \
>> > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>> > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>> > >>>>>
>> > >>>>> before:
>> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>> > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>> > >>>>>                    ^^       ^^ ^^
>> > >>>>> after:
>> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>> > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>> > >>>>>
>> > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>> > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> > >>>>> Cc: Tom Rini <trini@konsulko.com>
>> > >>>>
>> > >>>> Applied to u-boot/master, thanks!
>> > >>>>
>> > >>>
>> > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>> > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>> > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
>> > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
>> > >>> error going to the console at the 420k mark, but obviously it's lost
>> > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>> > >>> align to the point between them.
>> > >>
>> > >> My bet would be on some padding / unaligned access problem that this
>> > >> patch uncovered. Can you take a look ?
>> > >
>> > > Seems plausible. With this change my external data starts at 0x483 and
>> > > everything after it is non-aligned:
>> >
>> > Should the beginning of external data be aligned ?
>> 
>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does 
>> the
>> problem go away?  If so, that's not a fix outright, it means we need 
>> to
>> dig back in to the libfdt thread and find the "make this work without
>> killing performance everywhere all the time" option.
> 
> If it is a device tree, it must be 32-bit aligned.

This commit actually breaks my board too (which I was just about to send
upstream, but realized it was broken).

Said board uses SPL and main U-Boot. SPL runs fine and main u-boot 
doesn't
output anything. The only difference which I found is that fit-dtb.blob 
is
2 bytes shorter. And the content is shifted by one byte although
data-offset is the same. Strange. In the non-working case, the inner
FDT magic isn't 4 byte aligned.

You can find the two fit-dtb.blobs here:

https://walle.cc/u-boot/fit-dtb.blob.working
https://walle.cc/u-boot/fit-dtb.blob.non-working


Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
reverted it the wrong way, there is actually a conflict).

I'll dig deeper into that tomorrow, but maybe you have some pointers 
where
to look.

For reference you can find the current patch here:
https://github.com/mwalle/u-boot/tree/sl28-upstream

> 
> But Marek's patch affects the FIT image itself, so I am not sure what
> would go after that.
> 
> Regards,
> Simon

-- 
-michael

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Michael Walle]

Am 2020-05-05 23:17, schrieb Michael Walle:
> Hi all,
> 
> Am 2020-05-05 20:41, schrieb Simon Glass:
>> Hi Tom,
>> 
>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>> 
>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>> > On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>> > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>> > >>
>>> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>> > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>> > >>>>
>>> > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>> > >>>>
>>> > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>> > >>>>> while fitImage without external data does not have any such padding and
>>> > >>>>> is often unaligned. DT spec also does not mandate any such padding.
>>> > >>>>>
>>> > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>> > >>>>> which could lead to a potential information leak.
>>> > >>>>>
>>> > >>>>> $ echo -n xy > /tmp/data ; \
>>> > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>> > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>> > >>>>>
>>> > >>>>> before:
>>> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>> > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>> > >>>>>                    ^^       ^^ ^^
>>> > >>>>> after:
>>> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>> > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>> > >>>>>
>>> > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> > >>>>> Cc: Tom Rini <trini@konsulko.com>
>>> > >>>>
>>> > >>>> Applied to u-boot/master, thanks!
>>> > >>>>
>>> > >>>
>>> > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>> > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>> > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>> > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>> > >>> error going to the console at the 420k mark, but obviously it's lost
>>> > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>> > >>> align to the point between them.
>>> > >>
>>> > >> My bet would be on some padding / unaligned access problem that this
>>> > >> patch uncovered. Can you take a look ?
>>> > >
>>> > > Seems plausible. With this change my external data starts at 0x483 and
>>> > > everything after it is non-aligned:
>>> >
>>> > Should the beginning of external data be aligned ?
>>> 
>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does 
>>> the
>>> problem go away?  If so, that's not a fix outright, it means we need 
>>> to
>>> dig back in to the libfdt thread and find the "make this work without
>>> killing performance everywhere all the time" option.
>> 
>> If it is a device tree, it must be 32-bit aligned.
> 
> This commit actually breaks my board too (which I was just about to 
> send
> upstream, but realized it was broken).
> 
> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot 
> doesn't
> output anything. The only difference which I found is that fit-dtb.blob 
> is
> 2 bytes shorter. And the content is shifted by one byte although
> data-offset is the same. Strange. In the non-working case, the inner
> FDT magic isn't 4 byte aligned.

Here are the steps to reproduce it on the uboot master branch:

$ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig 
fit-dtb.blob tools
$ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob
$ file fdt-1
fdt-1: data
$ git revert 20a154f95bfe0a3b5bfba90bea7f001c58217536
$ git clean -fdx
$ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig 
fit-dtb.blob tools
$ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob
$ file fdt-1
fdt-1: Device Tree Blob version 17, size=34978, boot CPU=0, string block 
size=1746, DT structure block size=33176

-michael

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Alex Kiernan]

On Tue, May 5, 2020 at 10:17 PM Michael Walle <michael@walle.cc> wrote:
>
> Hi all,
>
> Am 2020-05-05 20:41, schrieb Simon Glass:
> > Hi Tom,
> >
> > On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >> > On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >> > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >> > >>
> >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >> > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >> > >>>>
> >> > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >> > >>>>
> >> > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >> > >>>>> while fitImage without external data does not have any such padding and
> >> > >>>>> is often unaligned. DT spec also does not mandate any such padding.
> >> > >>>>>
> >> > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >> > >>>>> which could lead to a potential information leak.
> >> > >>>>>
> >> > >>>>> $ echo -n xy > /tmp/data ; \
> >> > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >> > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >> > >>>>>
> >> > >>>>> before:
> >> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >> > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >> > >>>>>                    ^^       ^^ ^^
> >> > >>>>> after:
> >> > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >> > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >> > >>>>>
> >> > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >> > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> > >>>>> Cc: Tom Rini <trini@konsulko.com>
> >> > >>>>
> >> > >>>> Applied to u-boot/master, thanks!
> >> > >>>>
> >> > >>>
> >> > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >> > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >> > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >> > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >> > >>> error going to the console at the 420k mark, but obviously it's lost
> >> > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >> > >>> align to the point between them.
> >> > >>
> >> > >> My bet would be on some padding / unaligned access problem that this
> >> > >> patch uncovered. Can you take a look ?
> >> > >
> >> > > Seems plausible. With this change my external data starts at 0x483 and
> >> > > everything after it is non-aligned:
> >> >
> >> > Should the beginning of external data be aligned ?
> >>
> >> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >> the
> >> problem go away?  If so, that's not a fix outright, it means we need
> >> to
> >> dig back in to the libfdt thread and find the "make this work without
> >> killing performance everywhere all the time" option.
> >
> > If it is a device tree, it must be 32-bit aligned.
>
> This commit actually breaks my board too (which I was just about to send
> upstream, but realized it was broken).
>
> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot
> doesn't
> output anything. The only difference which I found is that fit-dtb.blob
> is
> 2 bytes shorter. And the content is shifted by one byte although
> data-offset is the same. Strange. In the non-working case, the inner
> FDT magic isn't 4 byte aligned.
>
> You can find the two fit-dtb.blobs here:
>
> https://walle.cc/u-boot/fit-dtb.blob.working
> https://walle.cc/u-boot/fit-dtb.blob.non-working
>
>
> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> reverted it the wrong way, there is actually a conflict).
>

I just did the same thing by manually extracting the patches and I
agree, reverting that doesn't fix the problem.

> I'll dig deeper into that tomorrow, but maybe you have some pointers
> where
> to look.
>
> For reference you can find the current patch here:
> https://github.com/mwalle/u-boot/tree/sl28-upstream
>
> >
> > But Marek's patch affects the FIT image itself, so I am not sure what
> > would go after that.
> >
> > Regards,
> > Simon
>
> --
> -michael



-- 
Alex Kiernan

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> Hi all,
> 
> Am 2020-05-05 20:41, schrieb Simon Glass:
> > Hi Tom,
> > 
> > On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> > > 
> > > On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> > > > On 5/5/20 6:37 PM, Alex Kiernan wrote:
> > > > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> > > > >>
> > > > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> > > > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> > > > >>>>
> > > > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> > > > >>>>
> > > > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> > > > >>>>> while fitImage without external data does not have any such padding and
> > > > >>>>> is often unaligned. DT spec also does not mandate any such padding.
> > > > >>>>>
> > > > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> > > > >>>>> which could lead to a potential information leak.
> > > > >>>>>
> > > > >>>>> $ echo -n xy > /tmp/data ; \
> > > > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> > > > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> > > > >>>>>
> > > > >>>>> before:
> > > > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > > > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> > > > >>>>>                    ^^       ^^ ^^
> > > > >>>>> after:
> > > > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > > > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> > > > >>>>>
> > > > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> > > > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > >>>>> Cc: Tom Rini <trini@konsulko.com>
> > > > >>>>
> > > > >>>> Applied to u-boot/master, thanks!
> > > > >>>>
> > > > >>>
> > > > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> > > > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> > > > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
> > > > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
> > > > >>> error going to the console at the 420k mark, but obviously it's lost
> > > > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> > > > >>> align to the point between them.
> > > > >>
> > > > >> My bet would be on some padding / unaligned access problem that this
> > > > >> patch uncovered. Can you take a look ?
> > > > >
> > > > > Seems plausible. With this change my external data starts at 0x483 and
> > > > > everything after it is non-aligned:
> > > >
> > > > Should the beginning of external data be aligned ?
> > > 
> > > If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> > > the
> > > problem go away?  If so, that's not a fix outright, it means we need
> > > to
> > > dig back in to the libfdt thread and find the "make this work without
> > > killing performance everywhere all the time" option.
> > 
> > If it is a device tree, it must be 32-bit aligned.
> 
> This commit actually breaks my board too (which I was just about to send
> upstream, but realized it was broken).
> 
> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> output anything. The only difference which I found is that fit-dtb.blob is
> 2 bytes shorter. And the content is shifted by one byte although
> data-offset is the same. Strange. In the non-working case, the inner
> FDT magic isn't 4 byte aligned.
> 
> You can find the two fit-dtb.blobs here:
> 
> https://walle.cc/u-boot/fit-dtb.blob.working
> https://walle.cc/u-boot/fit-dtb.blob.non-working
> 
> 
> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> reverted it the wrong way, there is actually a conflict).
> 
> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> to look.
> 
> For reference you can find the current patch here:
> https://github.com/mwalle/u-boot/tree/sl28-upstream

I think we have a few things to fix here.  Marek's patch is breaking
things and needs to be reverted.  But it's showing a few underlying
problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't
  leak random data.
- We need to 8-byte alignment on the external data.  That's the
  requirement for Linux for device trees on both 32 and 64bit arm.
  Atish, does RISC-V require more than that?  I don't see it in Linux's
  Documentation/riscv/boot-image-header.rst (and there's no booting.rst
  file like arm/arm64).

Thanks all!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/2c77c532/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 3:48 PM, Tom Rini wrote:
> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>> Hi all,
>>
>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>> Hi Tom,
>>>
>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>
>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>
>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>
>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>
>>>>>>>>>> before:
>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>> after:
>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>
>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>
>>>>>>>>
>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>> align to the point between them.
>>>>>>>
>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>> patch uncovered. Can you take a look ?
>>>>>>
>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>> everything after it is non-aligned:
>>>>>
>>>>> Should the beginning of external data be aligned ?
>>>>
>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>> the
>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>> to
>>>> dig back in to the libfdt thread and find the "make this work without
>>>> killing performance everywhere all the time" option.
>>>
>>> If it is a device tree, it must be 32-bit aligned.
>>
>> This commit actually breaks my board too (which I was just about to send
>> upstream, but realized it was broken).
>>
>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>> output anything. The only difference which I found is that fit-dtb.blob is
>> 2 bytes shorter. And the content is shifted by one byte although
>> data-offset is the same. Strange. In the non-working case, the inner
>> FDT magic isn't 4 byte aligned.
>>
>> You can find the two fit-dtb.blobs here:
>>
>> https://walle.cc/u-boot/fit-dtb.blob.working
>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>
>>
>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>> reverted it the wrong way, there is actually a conflict).
>>
>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>> to look.
>>
>> For reference you can find the current patch here:
>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> 
> I think we have a few things to fix here.  Marek's patch is breaking
> things and needs to be reverted.  But it's showing a few underlying
> problems that need to be fixed too:
> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>   leak random data.
> - We need to 8-byte alignment on the external data.  That's the
>   requirement for Linux for device trees on both 32 and 64bit arm.
>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>   file like arm/arm64).

Why 8-byte alignment ? The external data are copied into the target
location, so why do they need to be padded in any way?

If the external data are used in place, then it's the same problem as
arm64 fitImage with fdt_high=-1, which fails because the DT is aligned
to 4 bytes and arm64 expects it at 8byte offset.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> On 5/6/20 3:48 PM, Tom Rini wrote:
> > On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >> Hi all,
> >>
> >> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>> Hi Tom,
> >>>
> >>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>
> >>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>
> >>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>
> >>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>
> >>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>
> >>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>
> >>>>>>>>>> before:
> >>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>> after:
> >>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>
> >>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>> align to the point between them.
> >>>>>>>
> >>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>> patch uncovered. Can you take a look ?
> >>>>>>
> >>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>> everything after it is non-aligned:
> >>>>>
> >>>>> Should the beginning of external data be aligned ?
> >>>>
> >>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>> the
> >>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>> to
> >>>> dig back in to the libfdt thread and find the "make this work without
> >>>> killing performance everywhere all the time" option.
> >>>
> >>> If it is a device tree, it must be 32-bit aligned.
> >>
> >> This commit actually breaks my board too (which I was just about to send
> >> upstream, but realized it was broken).
> >>
> >> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >> output anything. The only difference which I found is that fit-dtb.blob is
> >> 2 bytes shorter. And the content is shifted by one byte although
> >> data-offset is the same. Strange. In the non-working case, the inner
> >> FDT magic isn't 4 byte aligned.
> >>
> >> You can find the two fit-dtb.blobs here:
> >>
> >> https://walle.cc/u-boot/fit-dtb.blob.working
> >> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>
> >>
> >> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >> reverted it the wrong way, there is actually a conflict).
> >>
> >> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >> to look.
> >>
> >> For reference you can find the current patch here:
> >> https://github.com/mwalle/u-boot/tree/sl28-upstream
> > 
> > I think we have a few things to fix here.  Marek's patch is breaking
> > things and needs to be reverted.  But it's showing a few underlying
> > problems that need to be fixed too:
> > - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >   leak random data.
> > - We need to 8-byte alignment on the external data.  That's the
> >   requirement for Linux for device trees on both 32 and 64bit arm.
> >   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >   file like arm/arm64).
> 
> Why 8-byte alignment ? The external data are copied into the target
> location, so why do they need to be padded in any way?

The start of the external data needs the alignment, to be clearer.

> If the external data are used in place, then it's the same problem as
> arm64 fitImage with fdt_high=-1, which fails because the DT is aligned
> to 4 bytes and arm64 expects it at 8byte offset.

Except we're talking about cases where we can't relocate the data or it
doesn't make any sense to.

That said, if you think the answer is that we need to ensure that when
we use external data we first align it, please show us how that looks.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/b2ba3ab9/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 4:27 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>> Hi all,
>>>>
>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>> Hi Tom,
>>>>>
>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>
>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>
>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>
>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>
>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>
>>>>>>>>>>>> before:
>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>> after:
>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>
>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>> align to the point between them.
>>>>>>>>>
>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>
>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>> everything after it is non-aligned:
>>>>>>>
>>>>>>> Should the beginning of external data be aligned ?
>>>>>>
>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>> the
>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>> to
>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>> killing performance everywhere all the time" option.
>>>>>
>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>
>>>> This commit actually breaks my board too (which I was just about to send
>>>> upstream, but realized it was broken).
>>>>
>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>> FDT magic isn't 4 byte aligned.
>>>>
>>>> You can find the two fit-dtb.blobs here:
>>>>
>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>
>>>>
>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>> reverted it the wrong way, there is actually a conflict).
>>>>
>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>> to look.
>>>>
>>>> For reference you can find the current patch here:
>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>
>>> I think we have a few things to fix here.  Marek's patch is breaking
>>> things and needs to be reverted.  But it's showing a few underlying
>>> problems that need to be fixed too:
>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>   leak random data.
>>> - We need to 8-byte alignment on the external data.  That's the
>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>   file like arm/arm64).
>>
>> Why 8-byte alignment ? The external data are copied into the target
>> location, so why do they need to be padded in any way?
> 
> The start of the external data needs the alignment, to be clearer.

Why ?

>> If the external data are used in place, then it's the same problem as
>> arm64 fitImage with fdt_high=-1, which fails because the DT is aligned
>> to 4 bytes and arm64 expects it at 8byte offset.
> 
> Except we're talking about cases where we can't relocate the data or it
> doesn't make any sense to.

Which cases? This really needs to be spelled out.

> That said, if you think the answer is that we need to ensure that when
> we use external data we first align it, please show us how that looks.

I would like to understand the problem space first.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
> On 5/6/20 4:27 PM, Tom Rini wrote:
> > On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> >> On 5/6/20 3:48 PM, Tom Rini wrote:
> >>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >>>> Hi all,
> >>>>
> >>>> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>>>> Hi Tom,
> >>>>>
> >>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>>>
> >>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>
> >>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>>>
> >>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>>>
> >>>>>>>>>>>> before:
> >>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>>>> after:
> >>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>
> >>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>>>> align to the point between them.
> >>>>>>>>>
> >>>>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>>>> patch uncovered. Can you take a look ?
> >>>>>>>>
> >>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>>>> everything after it is non-aligned:
> >>>>>>>
> >>>>>>> Should the beginning of external data be aligned ?
> >>>>>>
> >>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>>>> the
> >>>>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>>>> to
> >>>>>> dig back in to the libfdt thread and find the "make this work without
> >>>>>> killing performance everywhere all the time" option.
> >>>>>
> >>>>> If it is a device tree, it must be 32-bit aligned.
> >>>>
> >>>> This commit actually breaks my board too (which I was just about to send
> >>>> upstream, but realized it was broken).
> >>>>
> >>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >>>> output anything. The only difference which I found is that fit-dtb.blob is
> >>>> 2 bytes shorter. And the content is shifted by one byte although
> >>>> data-offset is the same. Strange. In the non-working case, the inner
> >>>> FDT magic isn't 4 byte aligned.
> >>>>
> >>>> You can find the two fit-dtb.blobs here:
> >>>>
> >>>> https://walle.cc/u-boot/fit-dtb.blob.working
> >>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>>>
> >>>>
> >>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >>>> reverted it the wrong way, there is actually a conflict).
> >>>>
> >>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >>>> to look.
> >>>>
> >>>> For reference you can find the current patch here:
> >>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> >>>
> >>> I think we have a few things to fix here.  Marek's patch is breaking
> >>> things and needs to be reverted.  But it's showing a few underlying
> >>> problems that need to be fixed too:
> >>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >>>   leak random data.
> >>> - We need to 8-byte alignment on the external data.  That's the
> >>>   requirement for Linux for device trees on both 32 and 64bit arm.
> >>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >>>   file like arm/arm64).
> >>
> >> Why 8-byte alignment ? The external data are copied into the target
> >> location, so why do they need to be padded in any way?
> > 
> > The start of the external data needs the alignment, to be clearer.
> 
> Why ?

Given that things which end up in external data have alignment
requirements, we need to align and meet those requirements.  And I noted
why 8 above.

> >> If the external data are used in place, then it's the same problem as
> >> arm64 fitImage with fdt_high=-1, which fails because the DT is aligned
> >> to 4 bytes and arm64 expects it at 8byte offset.
> > 
> > Except we're talking about cases where we can't relocate the data or it
> > doesn't make any sense to.
> 
> Which cases? This really needs to be spelled out.
> 
> > That said, if you think the answer is that we need to ensure that when
> > we use external data we first align it, please show us how that looks.
> 
> I would like to understand the problem space first.

Then please re-read this thread and come up with an alternative solution
to the problem you're trying to solve, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/ff63b84f/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 4:37 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
>> On 5/6/20 4:27 PM, Tom Rini wrote:
>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> before:
>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>>>> after:
>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>>>> align to the point between them.
>>>>>>>>>>>
>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>>>
>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>>>> everything after it is non-aligned:
>>>>>>>>>
>>>>>>>>> Should the beginning of external data be aligned ?
>>>>>>>>
>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>>>> the
>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>>>> to
>>>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>>>> killing performance everywhere all the time" option.
>>>>>>>
>>>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>>>
>>>>>> This commit actually breaks my board too (which I was just about to send
>>>>>> upstream, but realized it was broken).
>>>>>>
>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>>>> FDT magic isn't 4 byte aligned.
>>>>>>
>>>>>> You can find the two fit-dtb.blobs here:
>>>>>>
>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>>>
>>>>>>
>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>>>> reverted it the wrong way, there is actually a conflict).
>>>>>>
>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>>>> to look.
>>>>>>
>>>>>> For reference you can find the current patch here:
>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>>>
>>>>> I think we have a few things to fix here.  Marek's patch is breaking
>>>>> things and needs to be reverted.  But it's showing a few underlying
>>>>> problems that need to be fixed too:
>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>>>   leak random data.
>>>>> - We need to 8-byte alignment on the external data.  That's the
>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>>>   file like arm/arm64).
>>>>
>>>> Why 8-byte alignment ? The external data are copied into the target
>>>> location, so why do they need to be padded in any way?
>>>
>>> The start of the external data needs the alignment, to be clearer.
>>
>> Why ?
> 
> Given that things which end up in external data have alignment
> requirements, we need to align and meet those requirements.  And I noted
> why 8 above.

If you end up with external data, then you need to move those blobs into
their target location anyway. That's what you specify in the load = <>
property in the .its .

>>>> If the external data are used in place, then it's the same problem as
>>>> arm64 fitImage with fdt_high=-1, which fails because the DT is aligned
>>>> to 4 bytes and arm64 expects it at 8byte offset.
>>>
>>> Except we're talking about cases where we can't relocate the data or it
>>> doesn't make any sense to.
>>
>> Which cases? This really needs to be spelled out.
>>
>>> That said, if you think the answer is that we need to ensure that when
>>> we use external data we first align it, please show us how that looks.
>>
>> I would like to understand the problem space first.
> 
> Then please re-read this thread and come up with an alternative solution
> to the problem you're trying to solve, thanks!

Thank you for the detailed explanation.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Alex Kiernan]

On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
>
> On 5/6/20 4:37 PM, Tom Rini wrote:
> > On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
> >> On 5/6/20 4:27 PM, Tom Rini wrote:
> >>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> >>>> On 5/6/20 3:48 PM, Tom Rini wrote:
> >>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>>>>>> Hi Tom,
> >>>>>>>
> >>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>
> >>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> before:
> >>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>>>>>> after:
> >>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>>>>>> align to the point between them.
> >>>>>>>>>>>
> >>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>>>>>> patch uncovered. Can you take a look ?
> >>>>>>>>>>
> >>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>>>>>> everything after it is non-aligned:
> >>>>>>>>>
> >>>>>>>>> Should the beginning of external data be aligned ?
> >>>>>>>>
> >>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>>>>>> the
> >>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>>>>>> to
> >>>>>>>> dig back in to the libfdt thread and find the "make this work without
> >>>>>>>> killing performance everywhere all the time" option.
> >>>>>>>
> >>>>>>> If it is a device tree, it must be 32-bit aligned.
> >>>>>>
> >>>>>> This commit actually breaks my board too (which I was just about to send
> >>>>>> upstream, but realized it was broken).
> >>>>>>
> >>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >>>>>> output anything. The only difference which I found is that fit-dtb.blob is
> >>>>>> 2 bytes shorter. And the content is shifted by one byte although
> >>>>>> data-offset is the same. Strange. In the non-working case, the inner
> >>>>>> FDT magic isn't 4 byte aligned.
> >>>>>>
> >>>>>> You can find the two fit-dtb.blobs here:
> >>>>>>
> >>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
> >>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>>>>>
> >>>>>>
> >>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >>>>>> reverted it the wrong way, there is actually a conflict).
> >>>>>>
> >>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >>>>>> to look.
> >>>>>>
> >>>>>> For reference you can find the current patch here:
> >>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> >>>>>
> >>>>> I think we have a few things to fix here.  Marek's patch is breaking
> >>>>> things and needs to be reverted.  But it's showing a few underlying
> >>>>> problems that need to be fixed too:
> >>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >>>>>   leak random data.
> >>>>> - We need to 8-byte alignment on the external data.  That's the
> >>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
> >>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >>>>>   file like arm/arm64).
> >>>>
> >>>> Why 8-byte alignment ? The external data are copied into the target
> >>>> location, so why do they need to be padded in any way?
> >>>
> >>> The start of the external data needs the alignment, to be clearer.
> >>
> >> Why ?
> >
> > Given that things which end up in external data have alignment
> > requirements, we need to align and meet those requirements.  And I noted
> > why 8 above.
>
> If you end up with external data, then you need to move those blobs into
> their target location anyway. That's what you specify in the load = <>
> property in the .its .
>

Just reading common/spl/spl_fit.c, I think that'll try and parse in
situ, rather than relocating it?


--
Alex Kiernan

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 5:43 PM, Alex Kiernan wrote:
> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/6/20 4:37 PM, Tom Rini wrote:
>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>>>>>> Hi Tom,
>>>>>>>>>
>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> before:
>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>>>>>> after:
>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>>>>>> align to the point between them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>>>>>
>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>>>>>> everything after it is non-aligned:
>>>>>>>>>>>
>>>>>>>>>>> Should the beginning of external data be aligned ?
>>>>>>>>>>
>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>>>>>> the
>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>>>>>> to
>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>>>>>> killing performance everywhere all the time" option.
>>>>>>>>>
>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>>>>>
>>>>>>>> This commit actually breaks my board too (which I was just about to send
>>>>>>>> upstream, but realized it was broken).
>>>>>>>>
>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>>>>>> FDT magic isn't 4 byte aligned.
>>>>>>>>
>>>>>>>> You can find the two fit-dtb.blobs here:
>>>>>>>>
>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>>>>>
>>>>>>>>
>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>>>>>> reverted it the wrong way, there is actually a conflict).
>>>>>>>>
>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>>>>>> to look.
>>>>>>>>
>>>>>>>> For reference you can find the current patch here:
>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>>>>>
>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
>>>>>>> things and needs to be reverted.  But it's showing a few underlying
>>>>>>> problems that need to be fixed too:
>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>>>>>   leak random data.
>>>>>>> - We need to 8-byte alignment on the external data.  That's the
>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>>>>>   file like arm/arm64).
>>>>>>
>>>>>> Why 8-byte alignment ? The external data are copied into the target
>>>>>> location, so why do they need to be padded in any way?
>>>>>
>>>>> The start of the external data needs the alignment, to be clearer.
>>>>
>>>> Why ?
>>>
>>> Given that things which end up in external data have alignment
>>> requirements, we need to align and meet those requirements.  And I noted
>>> why 8 above.
>>
>> If you end up with external data, then you need to move those blobs into
>> their target location anyway. That's what you specify in the load = <>
>> property in the .its .
>>
> 
> Just reading common/spl/spl_fit.c, I think that'll try and parse in
> situ, rather than relocating it?

And is that correct or is that the same problem as we have on arm64 with
fitImage and fdt_high=-1 ? I think it's the later.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
> On 5/6/20 5:43 PM, Alex Kiernan wrote:
> > On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 5/6/20 4:37 PM, Tom Rini wrote:
> >>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
> >>>> On 5/6/20 4:27 PM, Tom Rini wrote:
> >>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> >>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
> >>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >>>>>>>> Hi all,
> >>>>>>>>
> >>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>>>>>>>> Hi Tom,
> >>>>>>>>>
> >>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> before:
> >>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>>>>>>>> after:
> >>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>>>>>>>> align to the point between them.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>>>>>>>> patch uncovered. Can you take a look ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>>>>>>>> everything after it is non-aligned:
> >>>>>>>>>>>
> >>>>>>>>>>> Should the beginning of external data be aligned ?
> >>>>>>>>>>
> >>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>>>>>>>> the
> >>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>>>>>>>> to
> >>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
> >>>>>>>>>> killing performance everywhere all the time" option.
> >>>>>>>>>
> >>>>>>>>> If it is a device tree, it must be 32-bit aligned.
> >>>>>>>>
> >>>>>>>> This commit actually breaks my board too (which I was just about to send
> >>>>>>>> upstream, but realized it was broken).
> >>>>>>>>
> >>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
> >>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
> >>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
> >>>>>>>> FDT magic isn't 4 byte aligned.
> >>>>>>>>
> >>>>>>>> You can find the two fit-dtb.blobs here:
> >>>>>>>>
> >>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
> >>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >>>>>>>> reverted it the wrong way, there is actually a conflict).
> >>>>>>>>
> >>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >>>>>>>> to look.
> >>>>>>>>
> >>>>>>>> For reference you can find the current patch here:
> >>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> >>>>>>>
> >>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
> >>>>>>> things and needs to be reverted.  But it's showing a few underlying
> >>>>>>> problems that need to be fixed too:
> >>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >>>>>>>   leak random data.
> >>>>>>> - We need to 8-byte alignment on the external data.  That's the
> >>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
> >>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >>>>>>>   file like arm/arm64).
> >>>>>>
> >>>>>> Why 8-byte alignment ? The external data are copied into the target
> >>>>>> location, so why do they need to be padded in any way?
> >>>>>
> >>>>> The start of the external data needs the alignment, to be clearer.
> >>>>
> >>>> Why ?
> >>>
> >>> Given that things which end up in external data have alignment
> >>> requirements, we need to align and meet those requirements.  And I noted
> >>> why 8 above.
> >>
> >> If you end up with external data, then you need to move those blobs into
> >> their target location anyway. That's what you specify in the load = <>
> >> property in the .its .
> >>
> > 
> > Just reading common/spl/spl_fit.c, I think that'll try and parse in
> > situ, rather than relocating it?
> 
> And is that correct or is that the same problem as we have on arm64 with
> fitImage and fdt_high=-1 ? I think it's the later.

I'm not sure that it is.  Can we easily/safely memmove the data to be
aligned?  Is that really a better option in this case than ensuring
alignment within the file?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/2b0a2c13/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 6:04 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
>> On 5/6/20 5:43 PM, Alex Kiernan wrote:
>>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 5/6/20 4:37 PM, Tom Rini wrote:
>>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
>>>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
>>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>>>>>>>> Hi Tom,
>>>>>>>>>>>
>>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> before:
>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>>>>>>>> after:
>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>>>>>>>> align to the point between them.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>>>>>>>> everything after it is non-aligned:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Should the beginning of external data be aligned ?
>>>>>>>>>>>>
>>>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>>>>>>>> the
>>>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>>>>>>>> to
>>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>>>>>>>> killing performance everywhere all the time" option.
>>>>>>>>>>>
>>>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>>>>>>>
>>>>>>>>>> This commit actually breaks my board too (which I was just about to send
>>>>>>>>>> upstream, but realized it was broken).
>>>>>>>>>>
>>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>>>>>>>> FDT magic isn't 4 byte aligned.
>>>>>>>>>>
>>>>>>>>>> You can find the two fit-dtb.blobs here:
>>>>>>>>>>
>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>>>>>>>> reverted it the wrong way, there is actually a conflict).
>>>>>>>>>>
>>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>>>>>>>> to look.
>>>>>>>>>>
>>>>>>>>>> For reference you can find the current patch here:
>>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>>>>>>>
>>>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
>>>>>>>>> things and needs to be reverted.  But it's showing a few underlying
>>>>>>>>> problems that need to be fixed too:
>>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>>>>>>>   leak random data.
>>>>>>>>> - We need to 8-byte alignment on the external data.  That's the
>>>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>>>>>>>   file like arm/arm64).
>>>>>>>>
>>>>>>>> Why 8-byte alignment ? The external data are copied into the target
>>>>>>>> location, so why do they need to be padded in any way?
>>>>>>>
>>>>>>> The start of the external data needs the alignment, to be clearer.
>>>>>>
>>>>>> Why ?
>>>>>
>>>>> Given that things which end up in external data have alignment
>>>>> requirements, we need to align and meet those requirements.  And I noted
>>>>> why 8 above.
>>>>
>>>> If you end up with external data, then you need to move those blobs into
>>>> their target location anyway. That's what you specify in the load = <>
>>>> property in the .its .
>>>>
>>>
>>> Just reading common/spl/spl_fit.c, I think that'll try and parse in
>>> situ, rather than relocating it?
>>
>> And is that correct or is that the same problem as we have on arm64 with
>> fitImage and fdt_high=-1 ? I think it's the later.
> 
> I'm not sure that it is.  Can we easily/safely memmove the data to be
> aligned?  Is that really a better option in this case than ensuring
> alignment within the file?

Can't we use the new mkimage -B option to enforce the alignment IFF and
only IFF it is required ? Then we can enforce it separately for 32bit
and 64bit platforms to 4 and 8 bytes respectively even.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
> On 5/6/20 6:04 PM, Tom Rini wrote:
> > On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
> >> On 5/6/20 5:43 PM, Alex Kiernan wrote:
> >>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 5/6/20 4:37 PM, Tom Rini wrote:
> >>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
> >>>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
> >>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> >>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
> >>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >>>>>>>>>> Hi all,
> >>>>>>>>>>
> >>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>>>>>>>>>> Hi Tom,
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> before:
> >>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>>>>>>>>>> after:
> >>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>>>>>>>>>> align to the point between them.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>>>>>>>>>> patch uncovered. Can you take a look ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>>>>>>>>>> everything after it is non-aligned:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Should the beginning of external data be aligned ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>>>>>>>>>> the
> >>>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>>>>>>>>>> to
> >>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
> >>>>>>>>>>>> killing performance everywhere all the time" option.
> >>>>>>>>>>>
> >>>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
> >>>>>>>>>>
> >>>>>>>>>> This commit actually breaks my board too (which I was just about to send
> >>>>>>>>>> upstream, but realized it was broken).
> >>>>>>>>>>
> >>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >>>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
> >>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
> >>>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
> >>>>>>>>>> FDT magic isn't 4 byte aligned.
> >>>>>>>>>>
> >>>>>>>>>> You can find the two fit-dtb.blobs here:
> >>>>>>>>>>
> >>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
> >>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >>>>>>>>>> reverted it the wrong way, there is actually a conflict).
> >>>>>>>>>>
> >>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >>>>>>>>>> to look.
> >>>>>>>>>>
> >>>>>>>>>> For reference you can find the current patch here:
> >>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> >>>>>>>>>
> >>>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
> >>>>>>>>> things and needs to be reverted.  But it's showing a few underlying
> >>>>>>>>> problems that need to be fixed too:
> >>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >>>>>>>>>   leak random data.
> >>>>>>>>> - We need to 8-byte alignment on the external data.  That's the
> >>>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
> >>>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >>>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >>>>>>>>>   file like arm/arm64).
> >>>>>>>>
> >>>>>>>> Why 8-byte alignment ? The external data are copied into the target
> >>>>>>>> location, so why do they need to be padded in any way?
> >>>>>>>
> >>>>>>> The start of the external data needs the alignment, to be clearer.
> >>>>>>
> >>>>>> Why ?
> >>>>>
> >>>>> Given that things which end up in external data have alignment
> >>>>> requirements, we need to align and meet those requirements.  And I noted
> >>>>> why 8 above.
> >>>>
> >>>> If you end up with external data, then you need to move those blobs into
> >>>> their target location anyway. That's what you specify in the load = <>
> >>>> property in the .its .
> >>>>
> >>>
> >>> Just reading common/spl/spl_fit.c, I think that'll try and parse in
> >>> situ, rather than relocating it?
> >>
> >> And is that correct or is that the same problem as we have on arm64 with
> >> fitImage and fdt_high=-1 ? I think it's the later.
> > 
> > I'm not sure that it is.  Can we easily/safely memmove the data to be
> > aligned?  Is that really a better option in this case than ensuring
> > alignment within the file?
> 
> Can't we use the new mkimage -B option to enforce the alignment IFF and
> only IFF it is required ? 

Perhaps.  But..

> Then we can enforce it separately for 32bit
> and 64bit platforms to 4 and 8 bytes respectively even.

It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
you only do 4 byte alignment but the documented requirement is 8, for
arm32.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/f7b6ab27/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 6:33 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
>> On 5/6/20 6:04 PM, Tom Rini wrote:
>>> On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
>>>> On 5/6/20 5:43 PM, Alex Kiernan wrote:
>>>>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 5/6/20 4:37 PM, Tom Rini wrote:
>>>>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
>>>>>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
>>>>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>>>>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>>>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>>>>>>>>>> Hi Tom,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> before:
>>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>>>>>>>>>> after:
>>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>>>>>>>>>> align to the point between them.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>>>>>>>>>> everything after it is non-aligned:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Should the beginning of external data be aligned ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>>>>>>>>>> killing performance everywhere all the time" option.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>>>>>>>>>
>>>>>>>>>>>> This commit actually breaks my board too (which I was just about to send
>>>>>>>>>>>> upstream, but realized it was broken).
>>>>>>>>>>>>
>>>>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>>>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>>>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>>>>>>>>>> FDT magic isn't 4 byte aligned.
>>>>>>>>>>>>
>>>>>>>>>>>> You can find the two fit-dtb.blobs here:
>>>>>>>>>>>>
>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>>>>>>>>>> reverted it the wrong way, there is actually a conflict).
>>>>>>>>>>>>
>>>>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>>>>>>>>>> to look.
>>>>>>>>>>>>
>>>>>>>>>>>> For reference you can find the current patch here:
>>>>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>>>>>>>>>
>>>>>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
>>>>>>>>>>> things and needs to be reverted.  But it's showing a few underlying
>>>>>>>>>>> problems that need to be fixed too:
>>>>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>>>>>>>>>   leak random data.
>>>>>>>>>>> - We need to 8-byte alignment on the external data.  That's the
>>>>>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>>>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>>>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>>>>>>>>>   file like arm/arm64).
>>>>>>>>>>
>>>>>>>>>> Why 8-byte alignment ? The external data are copied into the target
>>>>>>>>>> location, so why do they need to be padded in any way?
>>>>>>>>>
>>>>>>>>> The start of the external data needs the alignment, to be clearer.
>>>>>>>>
>>>>>>>> Why ?
>>>>>>>
>>>>>>> Given that things which end up in external data have alignment
>>>>>>> requirements, we need to align and meet those requirements.  And I noted
>>>>>>> why 8 above.
>>>>>>
>>>>>> If you end up with external data, then you need to move those blobs into
>>>>>> their target location anyway. That's what you specify in the load = <>
>>>>>> property in the .its .
>>>>>>
>>>>>
>>>>> Just reading common/spl/spl_fit.c, I think that'll try and parse in
>>>>> situ, rather than relocating it?
>>>>
>>>> And is that correct or is that the same problem as we have on arm64 with
>>>> fitImage and fdt_high=-1 ? I think it's the later.
>>>
>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>> aligned?  Is that really a better option in this case than ensuring
>>> alignment within the file?
>>
>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>> only IFF it is required ? 
> 
> Perhaps.  But..
> 
>> Then we can enforce it separately for 32bit
>> and 64bit platforms to 4 and 8 bytes respectively even.
> 
> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
> you only do 4 byte alignment but the documented requirement is 8, for
> arm32.

With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
for arm64 for example.

And what you usually parse in-place would be the DT then.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Wed, May 06, 2020 at 06:35:52PM +0200, Marek Vasut wrote:
> On 5/6/20 6:33 PM, Tom Rini wrote:
> > On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
> >> On 5/6/20 6:04 PM, Tom Rini wrote:
> >>> On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
> >>>> On 5/6/20 5:43 PM, Alex Kiernan wrote:
> >>>>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 5/6/20 4:37 PM, Tom Rini wrote:
> >>>>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
> >>>>>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
> >>>>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
> >>>>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
> >>>>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
> >>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
> >>>>>>>>>>>>> Hi Tom,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
> >>>>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> >>>>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> >>>>>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
> >>>>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> >>>>>>>>>>>>>>>>>>>> which could lead to a potential information leak.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
> >>>>>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> >>>>>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> before:
> >>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> >>>>>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
> >>>>>>>>>>>>>>>>>>>> after:
> >>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> >>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> >>>>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> >>>>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
> >>>>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
> >>>>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
> >>>>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> >>>>>>>>>>>>>>>>>> align to the point between them.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
> >>>>>>>>>>>>>>>>> patch uncovered. Can you take a look ?
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
> >>>>>>>>>>>>>>>> everything after it is non-aligned:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Should the beginning of external data be aligned ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
> >>>>>>>>>>>>>> the
> >>>>>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
> >>>>>>>>>>>>>> to
> >>>>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
> >>>>>>>>>>>>>> killing performance everywhere all the time" option.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This commit actually breaks my board too (which I was just about to send
> >>>>>>>>>>>> upstream, but realized it was broken).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
> >>>>>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
> >>>>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
> >>>>>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
> >>>>>>>>>>>> FDT magic isn't 4 byte aligned.
> >>>>>>>>>>>>
> >>>>>>>>>>>> You can find the two fit-dtb.blobs here:
> >>>>>>>>>>>>
> >>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
> >>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
> >>>>>>>>>>>> reverted it the wrong way, there is actually a conflict).
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
> >>>>>>>>>>>> to look.
> >>>>>>>>>>>>
> >>>>>>>>>>>> For reference you can find the current patch here:
> >>>>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
> >>>>>>>>>>>
> >>>>>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
> >>>>>>>>>>> things and needs to be reverted.  But it's showing a few underlying
> >>>>>>>>>>> problems that need to be fixed too:
> >>>>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
> >>>>>>>>>>>   leak random data.
> >>>>>>>>>>> - We need to 8-byte alignment on the external data.  That's the
> >>>>>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
> >>>>>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
> >>>>>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
> >>>>>>>>>>>   file like arm/arm64).
> >>>>>>>>>>
> >>>>>>>>>> Why 8-byte alignment ? The external data are copied into the target
> >>>>>>>>>> location, so why do they need to be padded in any way?
> >>>>>>>>>
> >>>>>>>>> The start of the external data needs the alignment, to be clearer.
> >>>>>>>>
> >>>>>>>> Why ?
> >>>>>>>
> >>>>>>> Given that things which end up in external data have alignment
> >>>>>>> requirements, we need to align and meet those requirements.  And I noted
> >>>>>>> why 8 above.
> >>>>>>
> >>>>>> If you end up with external data, then you need to move those blobs into
> >>>>>> their target location anyway. That's what you specify in the load = <>
> >>>>>> property in the .its .
> >>>>>>
> >>>>>
> >>>>> Just reading common/spl/spl_fit.c, I think that'll try and parse in
> >>>>> situ, rather than relocating it?
> >>>>
> >>>> And is that correct or is that the same problem as we have on arm64 with
> >>>> fitImage and fdt_high=-1 ? I think it's the later.
> >>>
> >>> I'm not sure that it is.  Can we easily/safely memmove the data to be
> >>> aligned?  Is that really a better option in this case than ensuring
> >>> alignment within the file?
> >>
> >> Can't we use the new mkimage -B option to enforce the alignment IFF and
> >> only IFF it is required ? 
> > 
> > Perhaps.  But..
> > 
> >> Then we can enforce it separately for 32bit
> >> and 64bit platforms to 4 and 8 bytes respectively even.
> > 
> > It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
> > you only do 4 byte alignment but the documented requirement is 8, for
> > arm32.
> 
> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
> for arm64 for example.

For arm64 you have to move it to where text_offset says it needs to be.
For arm32 the common (always, practically?) case is you're firing off
the zImage which does what's needed.  But..

> And what you usually parse in-place would be the DT then.

Yes, the practical case is that it's a DT and that needs 8 byte
alignment.  And we should just get back to aligning that correctly.
Going back to the v1 thread, it turns out the answer to "why do we even
have this padding?" is "we need the DT to be aligned".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200506/8a3f4328/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/6/20 7:02 PM, Tom Rini wrote:
> On Wed, May 06, 2020 at 06:35:52PM +0200, Marek Vasut wrote:
>> On 5/6/20 6:33 PM, Tom Rini wrote:
>>> On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
>>>> On 5/6/20 6:04 PM, Tom Rini wrote:
>>>>> On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
>>>>>> On 5/6/20 5:43 PM, Alex Kiernan wrote:
>>>>>>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 5/6/20 4:37 PM, Tom Rini wrote:
>>>>>>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
>>>>>>>>>> On 5/6/20 4:27 PM, Tom Rini wrote:
>>>>>>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
>>>>>>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote:
>>>>>>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass:
>>>>>>>>>>>>>>> Hi Tom,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote:
>>>>>>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
>>>>>>>>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and
>>>>>>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
>>>>>>>>>>>>>>>>>>>>>> which could lead to a potential information leak.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \
>>>>>>>>>>>>>>>>>>>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
>>>>>>>>>>>>>>>>>>>>>>       hexdump -vC /tmp/fitImage | tail -n 3
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> before:
>>>>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
>>>>>>>>>>>>>>>>>>>>>>                    ^^       ^^ ^^
>>>>>>>>>>>>>>>>>>>>>> after:
>>>>>>>>>>>>>>>>>>>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
>>>>>>>>>>>>>>>>>>>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
>>>>>>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
>>>>>>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem
>>>>>>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some
>>>>>>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost
>>>>>>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about
>>>>>>>>>>>>>>>>>>>> align to the point between them.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this
>>>>>>>>>>>>>>>>>>> patch uncovered. Can you take a look ?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and
>>>>>>>>>>>>>>>>>> everything after it is non-aligned:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Should the beginning of external data be aligned ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> problem go away?  If so, that's not a fix outright, it means we need
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work without
>>>>>>>>>>>>>>>> killing performance everywhere all the time" option.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If it is a device tree, it must be 32-bit aligned.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This commit actually breaks my board too (which I was just about to send
>>>>>>>>>>>>>> upstream, but realized it was broken).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't
>>>>>>>>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is
>>>>>>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although
>>>>>>>>>>>>>> data-offset is the same. Strange. In the non-working case, the inner
>>>>>>>>>>>>>> FDT magic isn't 4 byte aligned.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> You can find the two fit-dtb.blobs here:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working
>>>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might
>>>>>>>>>>>>>> reverted it the wrong way, there is actually a conflict).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where
>>>>>>>>>>>>>> to look.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> For reference you can find the current patch here:
>>>>>>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think we have a few things to fix here.  Marek's patch is breaking
>>>>>>>>>>>>> things and needs to be reverted.  But it's showing a few underlying
>>>>>>>>>>>>> problems that need to be fixed too:
>>>>>>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't
>>>>>>>>>>>>>   leak random data.
>>>>>>>>>>>>> - We need to 8-byte alignment on the external data.  That's the
>>>>>>>>>>>>>   requirement for Linux for device trees on both 32 and 64bit arm.
>>>>>>>>>>>>>   Atish, does RISC-V require more than that?  I don't see it in Linux's
>>>>>>>>>>>>>   Documentation/riscv/boot-image-header.rst (and there's no booting.rst
>>>>>>>>>>>>>   file like arm/arm64).
>>>>>>>>>>>>
>>>>>>>>>>>> Why 8-byte alignment ? The external data are copied into the target
>>>>>>>>>>>> location, so why do they need to be padded in any way?
>>>>>>>>>>>
>>>>>>>>>>> The start of the external data needs the alignment, to be clearer.
>>>>>>>>>>
>>>>>>>>>> Why ?
>>>>>>>>>
>>>>>>>>> Given that things which end up in external data have alignment
>>>>>>>>> requirements, we need to align and meet those requirements.  And I noted
>>>>>>>>> why 8 above.
>>>>>>>>
>>>>>>>> If you end up with external data, then you need to move those blobs into
>>>>>>>> their target location anyway. That's what you specify in the load = <>
>>>>>>>> property in the .its .
>>>>>>>>
>>>>>>>
>>>>>>> Just reading common/spl/spl_fit.c, I think that'll try and parse in
>>>>>>> situ, rather than relocating it?
>>>>>>
>>>>>> And is that correct or is that the same problem as we have on arm64 with
>>>>>> fitImage and fdt_high=-1 ? I think it's the later.
>>>>>
>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>> alignment within the file?
>>>>
>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>> only IFF it is required ? 
>>>
>>> Perhaps.  But..
>>>
>>>> Then we can enforce it separately for 32bit
>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>
>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>> you only do 4 byte alignment but the documented requirement is 8, for
>>> arm32.
>>
>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>> for arm64 for example.
> 
> For arm64 you have to move it to where text_offset says it needs to be.

The alignment for compressed image is 2 MiB though, right ?

> For arm32 the common (always, practically?) case is you're firing off
> the zImage which does what's needed.  But..
> 
>> And what you usually parse in-place would be the DT then.
> 
> Yes, the practical case is that it's a DT and that needs 8 byte
> alignment.

4-byte

> And we should just get back to aligning that correctly.
> Going back to the v1 thread, it turns out the answer to "why do we even
> have this padding?" is "we need the DT to be aligned".

OK, let's assume you enforce 8-byte alignment on the external data. But
then, there is fitImage with embedded data (mkimage called without -E),
and the DT inside that fitImage will be aligned to 4 bytes. Then what?

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Samuel Holland]

On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>> alignment within the file?
>>>>
>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>> only IFF it is required ? 
>>>
>>> Perhaps.  But..
>>>
>>>> Then we can enforce it separately for 32bit
>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>
>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>> you only do 4 byte alignment but the documented requirement is 8, for
>>> arm32.
>>
>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>> for arm64 for example.
> 
> For arm64 you have to move it to where text_offset says it needs to be.
> For arm32 the common (always, practically?) case is you're firing off
> the zImage which does what's needed.  But..
> 
>> And what you usually parse in-place would be the DT then.
> 
> Yes, the practical case is that it's a DT and that needs 8 byte
> alignment.  And we should just get back to aligning that correctly.
> Going back to the v1 thread, it turns out the answer to "why do we even
> have this padding?" is "we need the DT to be aligned".

This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
alignment of external data after the FIT. In spl_load_simple_fit():

 /*
  * For FIT with external data, figure out where the external images
  * start. This is the base for the data-offset properties in each
  * image.
  */
 size = fdt_totalsize(fit);
 size = (size + 3) & ~3;
 size = board_spl_fit_size_align(size);
 base_offset = (size + 3) & ~3;

And then later on in spl_load_fit_image():

 if (!fit_image_get_data_position(fit, node, &offset)) {
         external_data = true;
 } else if (!fit_image_get_data_offset(fit, node, &offset)) {
         offset += base_offset;
         external_data = true;
 }

In my case, after this change, the FIT binary was 0x453 bytes long. But SPL
rounded it up to 0x454, so the external data offsets were off by one, and the
first byte of every loadable was cut off in RAM:

 Trying to boot from MMC1
 fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x454
 firmware: 'uboot'
 External data: dst=4a000000, offset=454, size=81208
 src = 4a000054, dest = 4a000000
 4a000000: 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00 00

If I remove both "(size + 3) & ~3" lines, I can boot successfully:

 Trying to boot from MMC1

 fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x453
 firmware: 'uboot'

 External data: dst=4a000000, offset=453, size=81208
 src = 4a000053, dest = 4a000000

 4a000000: 1f 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00

In other words, it's not just FDTs that are affected by this change, but _all_
external data loaded from SPL.

So if you change the alignment to anything but 4 (be it 1 or 8 or something
else), you must also update the assumptions made by SPL.

Regards,
Samuel

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/7/20 10:46 PM, Samuel Holland wrote:
> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>>> alignment within the file?
>>>>>
>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>>> only IFF it is required ? 
>>>>
>>>> Perhaps.  But..
>>>>
>>>>> Then we can enforce it separately for 32bit
>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>>
>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>>> you only do 4 byte alignment but the documented requirement is 8, for
>>>> arm32.
>>>
>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>>> for arm64 for example.
>>
>> For arm64 you have to move it to where text_offset says it needs to be.
>> For arm32 the common (always, practically?) case is you're firing off
>> the zImage which does what's needed.  But..
>>
>>> And what you usually parse in-place would be the DT then.
>>
>> Yes, the practical case is that it's a DT and that needs 8 byte
>> alignment.  And we should just get back to aligning that correctly.
>> Going back to the v1 thread, it turns out the answer to "why do we even
>> have this padding?" is "we need the DT to be aligned".
> 
> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
> alignment of external data after the FIT. In spl_load_simple_fit():
> 
>  /*
>   * For FIT with external data, figure out where the external images
>   * start. This is the base for the data-offset properties in each
>   * image.
>   */
>  size = fdt_totalsize(fit);
>  size = (size + 3) & ~3;
>  size = board_spl_fit_size_align(size);
>  base_offset = (size + 3) & ~3;

Somehow this doesn't match the 8-byte alignment Tom was suggesting.
And that only leads me to believe that we can either make assumptions
about alignment, which would very likely fail one way or the other OR we
can say that for SPL as a special case, we enforce some alignment.

But that in turn fails for fitImage with embedded data, where the
embedded data are always aligned to 4 bytes, because that's how DTC
aligns properties.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
> On 5/7/20 10:46 PM, Samuel Holland wrote:
> > On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
> >>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
> >>>>>> aligned?  Is that really a better option in this case than ensuring
> >>>>>> alignment within the file?
> >>>>>
> >>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
> >>>>> only IFF it is required ? 
> >>>>
> >>>> Perhaps.  But..
> >>>>
> >>>>> Then we can enforce it separately for 32bit
> >>>>> and 64bit platforms to 4 and 8 bytes respectively even.
> >>>>
> >>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
> >>>> you only do 4 byte alignment but the documented requirement is 8, for
> >>>> arm32.
> >>>
> >>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
> >>> for arm64 for example.
> >>
> >> For arm64 you have to move it to where text_offset says it needs to be.
> >> For arm32 the common (always, practically?) case is you're firing off
> >> the zImage which does what's needed.  But..
> >>
> >>> And what you usually parse in-place would be the DT then.
> >>
> >> Yes, the practical case is that it's a DT and that needs 8 byte
> >> alignment.  And we should just get back to aligning that correctly.
> >> Going back to the v1 thread, it turns out the answer to "why do we even
> >> have this padding?" is "we need the DT to be aligned".
> > 
> > This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
> > haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
> > alignment of external data after the FIT. In spl_load_simple_fit():
> > 
> >  /*
> >   * For FIT with external data, figure out where the external images
> >   * start. This is the base for the data-offset properties in each
> >   * image.
> >   */
> >  size = fdt_totalsize(fit);
> >  size = (size + 3) & ~3;
> >  size = board_spl_fit_size_align(size);
> >  base_offset = (size + 3) & ~3;
> 
> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
> And that only leads me to believe that we can either make assumptions
> about alignment, which would very likely fail one way or the other OR we
> can say that for SPL as a special case, we enforce some alignment.

It's likely the case that on arm32 as there's no natural alignment
problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
DTB.  Which is why we should probably move the alignment here to 8 bytes
instead of 4.

> But that in turn fails for fitImage with embedded data, where the
> embedded data are always aligned to 4 bytes, because that's how DTC
> aligns properties.

I think the answer is that the use-case you're talking about is simply
going to require data to be relocated.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/c7013ac6/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/8/20 8:47 PM, Tom Rini wrote:
> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
>> On 5/7/20 10:46 PM, Samuel Holland wrote:
>>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>>>>> alignment within the file?
>>>>>>>
>>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>>>>> only IFF it is required ? 
>>>>>>
>>>>>> Perhaps.  But..
>>>>>>
>>>>>>> Then we can enforce it separately for 32bit
>>>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>>>>
>>>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>>>>> you only do 4 byte alignment but the documented requirement is 8, for
>>>>>> arm32.
>>>>>
>>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>>>>> for arm64 for example.
>>>>
>>>> For arm64 you have to move it to where text_offset says it needs to be.
>>>> For arm32 the common (always, practically?) case is you're firing off
>>>> the zImage which does what's needed.  But..
>>>>
>>>>> And what you usually parse in-place would be the DT then.
>>>>
>>>> Yes, the practical case is that it's a DT and that needs 8 byte
>>>> alignment.  And we should just get back to aligning that correctly.
>>>> Going back to the v1 thread, it turns out the answer to "why do we even
>>>> have this padding?" is "we need the DT to be aligned".
>>>
>>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
>>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
>>> alignment of external data after the FIT. In spl_load_simple_fit():
>>>
>>>  /*
>>>   * For FIT with external data, figure out where the external images
>>>   * start. This is the base for the data-offset properties in each
>>>   * image.
>>>   */
>>>  size = fdt_totalsize(fit);
>>>  size = (size + 3) & ~3;
>>>  size = board_spl_fit_size_align(size);
>>>  base_offset = (size + 3) & ~3;
>>
>> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
>> And that only leads me to believe that we can either make assumptions
>> about alignment, which would very likely fail one way or the other OR we
>> can say that for SPL as a special case, we enforce some alignment.
> 
> It's likely the case that on arm32 as there's no natural alignment
> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
> DTB.  Which is why we should probably move the alignment here to 8 bytes
> instead of 4.
> 
>> But that in turn fails for fitImage with embedded data, where the
>> embedded data are always aligned to 4 bytes, because that's how DTC
>> aligns properties.
> 
> I think the answer is that the use-case you're talking about is simply
> going to require data to be relocated.

I have a feeling that no matter how much you try to pad when generating
fitImage from U-Boot, there will always be a case where that will fail.
I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail

So that relocation logic or at least warning or something should be in
there, no matter what.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
> On 5/8/20 8:47 PM, Tom Rini wrote:
> > On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
> >> On 5/7/20 10:46 PM, Samuel Holland wrote:
> >>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
> >>>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
> >>>>>>>> aligned?  Is that really a better option in this case than ensuring
> >>>>>>>> alignment within the file?
> >>>>>>>
> >>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
> >>>>>>> only IFF it is required ? 
> >>>>>>
> >>>>>> Perhaps.  But..
> >>>>>>
> >>>>>>> Then we can enforce it separately for 32bit
> >>>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
> >>>>>>
> >>>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
> >>>>>> you only do 4 byte alignment but the documented requirement is 8, for
> >>>>>> arm32.
> >>>>>
> >>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
> >>>>> for arm64 for example.
> >>>>
> >>>> For arm64 you have to move it to where text_offset says it needs to be.
> >>>> For arm32 the common (always, practically?) case is you're firing off
> >>>> the zImage which does what's needed.  But..
> >>>>
> >>>>> And what you usually parse in-place would be the DT then.
> >>>>
> >>>> Yes, the practical case is that it's a DT and that needs 8 byte
> >>>> alignment.  And we should just get back to aligning that correctly.
> >>>> Going back to the v1 thread, it turns out the answer to "why do we even
> >>>> have this padding?" is "we need the DT to be aligned".
> >>>
> >>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
> >>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
> >>> alignment of external data after the FIT. In spl_load_simple_fit():
> >>>
> >>>  /*
> >>>   * For FIT with external data, figure out where the external images
> >>>   * start. This is the base for the data-offset properties in each
> >>>   * image.
> >>>   */
> >>>  size = fdt_totalsize(fit);
> >>>  size = (size + 3) & ~3;
> >>>  size = board_spl_fit_size_align(size);
> >>>  base_offset = (size + 3) & ~3;
> >>
> >> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
> >> And that only leads me to believe that we can either make assumptions
> >> about alignment, which would very likely fail one way or the other OR we
> >> can say that for SPL as a special case, we enforce some alignment.
> > 
> > It's likely the case that on arm32 as there's no natural alignment
> > problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
> > and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
> > DTB.  Which is why we should probably move the alignment here to 8 bytes
> > instead of 4.
> > 
> >> But that in turn fails for fitImage with embedded data, where the
> >> embedded data are always aligned to 4 bytes, because that's how DTC
> >> aligns properties.
> > 
> > I think the answer is that the use-case you're talking about is simply
> > going to require data to be relocated.
> 
> I have a feeling that no matter how much you try to pad when generating
> fitImage from U-Boot, there will always be a case where that will fail.
> I listed at least two:
> - fitImage with embedded data, 4byte alignment due to DTC
> - Older fitImages, 4byte alignment, fails on arm64
> - Someone can generate signed fitImage with older mkimage => fail
> 
> So that relocation logic or at least warning or something should be in
> there, no matter what.

There's two distinct areas here, and they keep being conflated.

The case of SPL and a FIT image for U-Boot+DTB.  We've always aligned
this to 4 bytes and it's worked.  I think if someone looked at the ARM
ARM for aarch64 you could reason out that "4-but-not-8-byte aligned
pointers are slow but work" as why this wasn't a hard fail on aarch64.
We should adjust our current alignment up to cover that and move on.

The case of FIT images and "kernel_noload" / fdt_high=-1 /
initrd_high=-1 and aarch64.  If you load a FIT image in to memory and
try and use it as-is, it will not work.  It's not even possible in the
general case as you would have to inspect the kernel, see what the
text_offset is and build a FIT image that took that in to account, to
not have to move the Image around.  The device tree will almost
certainly be misaligned and still need to be relocated.  This is why a
while back I sent out an email asking every maintainer of a board that
disabled device tree relocation to stop that.  Perhaps a run-time patch
to scream about this rather than note it as we do today would help (see
common/image-fdt.c::boot_relocate_fdt()).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/3fd9e3f8/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/8/20 9:21 PM, Tom Rini wrote:
> On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
>> On 5/8/20 8:47 PM, Tom Rini wrote:
>>> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
>>>> On 5/7/20 10:46 PM, Samuel Holland wrote:
>>>>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>>>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>>>>>>> alignment within the file?
>>>>>>>>>
>>>>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>>>>>>> only IFF it is required ? 
>>>>>>>>
>>>>>>>> Perhaps.  But..
>>>>>>>>
>>>>>>>>> Then we can enforce it separately for 32bit
>>>>>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>>>>>>
>>>>>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>>>>>>> you only do 4 byte alignment but the documented requirement is 8, for
>>>>>>>> arm32.
>>>>>>>
>>>>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>>>>>>> for arm64 for example.
>>>>>>
>>>>>> For arm64 you have to move it to where text_offset says it needs to be.
>>>>>> For arm32 the common (always, practically?) case is you're firing off
>>>>>> the zImage which does what's needed.  But..
>>>>>>
>>>>>>> And what you usually parse in-place would be the DT then.
>>>>>>
>>>>>> Yes, the practical case is that it's a DT and that needs 8 byte
>>>>>> alignment.  And we should just get back to aligning that correctly.
>>>>>> Going back to the v1 thread, it turns out the answer to "why do we even
>>>>>> have this padding?" is "we need the DT to be aligned".
>>>>>
>>>>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
>>>>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
>>>>> alignment of external data after the FIT. In spl_load_simple_fit():
>>>>>
>>>>>  /*
>>>>>   * For FIT with external data, figure out where the external images
>>>>>   * start. This is the base for the data-offset properties in each
>>>>>   * image.
>>>>>   */
>>>>>  size = fdt_totalsize(fit);
>>>>>  size = (size + 3) & ~3;
>>>>>  size = board_spl_fit_size_align(size);
>>>>>  base_offset = (size + 3) & ~3;
>>>>
>>>> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
>>>> And that only leads me to believe that we can either make assumptions
>>>> about alignment, which would very likely fail one way or the other OR we
>>>> can say that for SPL as a special case, we enforce some alignment.
>>>
>>> It's likely the case that on arm32 as there's no natural alignment
>>> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
>>> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
>>> DTB.  Which is why we should probably move the alignment here to 8 bytes
>>> instead of 4.
>>>
>>>> But that in turn fails for fitImage with embedded data, where the
>>>> embedded data are always aligned to 4 bytes, because that's how DTC
>>>> aligns properties.
>>>
>>> I think the answer is that the use-case you're talking about is simply
>>> going to require data to be relocated.
>>
>> I have a feeling that no matter how much you try to pad when generating
>> fitImage from U-Boot, there will always be a case where that will fail.
>> I listed at least two:
>> - fitImage with embedded data, 4byte alignment due to DTC
>> - Older fitImages, 4byte alignment, fails on arm64
>> - Someone can generate signed fitImage with older mkimage => fail
>>
>> So that relocation logic or at least warning or something should be in
>> there, no matter what.
> 
> There's two distinct areas here, and they keep being conflated.
> 
> The case of SPL and a FIT image for U-Boot+DTB.  We've always aligned
> this to 4 bytes and it's worked.  I think if someone looked at the ARM
> ARM for aarch64 you could reason out that "4-but-not-8-byte aligned
> pointers are slow but work" as why this wasn't a hard fail on aarch64.

But we had hard-fault on arm64, see
[PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses

> We should adjust our current alignment up to cover that and move on.

Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?
You will fail here either way, since if you build the fitImage with
embedded data, the embedded data will be aligned to 4 bytes, because DT
properties are aligned to 4 bytes.

> The case of FIT images and "kernel_noload" / fdt_high=-1 /
> initrd_high=-1 and aarch64.  If you load a FIT image in to memory and
> try and use it as-is, it will not work.  It's not even possible in the
> general case as you would have to inspect the kernel, see what the
> text_offset is and build a FIT image that took that in to account, to
> not have to move the Image around.  The device tree will almost
> certainly be misaligned and still need to be relocated.  This is why a
> while back I sent out an email asking every maintainer of a board that
> disabled device tree relocation to stop that.  Perhaps a run-time patch
> to scream about this rather than note it as we do today would help (see
> common/image-fdt.c::boot_relocate_fdt()).

I have a feeling we should do the relocation either way. And if there is
some special limited case (like the SPL), we should warn about it and
push mkimage with e.g. -B 8 flag to enforce the alignment.

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Tom Rini]

On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote:
> On 5/8/20 9:21 PM, Tom Rini wrote:
> > On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
> >> On 5/8/20 8:47 PM, Tom Rini wrote:
> >>> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
> >>>> On 5/7/20 10:46 PM, Samuel Holland wrote:
> >>>>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
> >>>>>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
> >>>>>>>>>> aligned?  Is that really a better option in this case than ensuring
> >>>>>>>>>> alignment within the file?
> >>>>>>>>>
> >>>>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
> >>>>>>>>> only IFF it is required ? 
> >>>>>>>>
> >>>>>>>> Perhaps.  But..
> >>>>>>>>
> >>>>>>>>> Then we can enforce it separately for 32bit
> >>>>>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
> >>>>>>>>
> >>>>>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
> >>>>>>>> you only do 4 byte alignment but the documented requirement is 8, for
> >>>>>>>> arm32.
> >>>>>>>
> >>>>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
> >>>>>>> for arm64 for example.
> >>>>>>
> >>>>>> For arm64 you have to move it to where text_offset says it needs to be.
> >>>>>> For arm32 the common (always, practically?) case is you're firing off
> >>>>>> the zImage which does what's needed.  But..
> >>>>>>
> >>>>>>> And what you usually parse in-place would be the DT then.
> >>>>>>
> >>>>>> Yes, the practical case is that it's a DT and that needs 8 byte
> >>>>>> alignment.  And we should just get back to aligning that correctly.
> >>>>>> Going back to the v1 thread, it turns out the answer to "why do we even
> >>>>>> have this padding?" is "we need the DT to be aligned".
> >>>>>
> >>>>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
> >>>>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
> >>>>> alignment of external data after the FIT. In spl_load_simple_fit():
> >>>>>
> >>>>>  /*
> >>>>>   * For FIT with external data, figure out where the external images
> >>>>>   * start. This is the base for the data-offset properties in each
> >>>>>   * image.
> >>>>>   */
> >>>>>  size = fdt_totalsize(fit);
> >>>>>  size = (size + 3) & ~3;
> >>>>>  size = board_spl_fit_size_align(size);
> >>>>>  base_offset = (size + 3) & ~3;
> >>>>
> >>>> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
> >>>> And that only leads me to believe that we can either make assumptions
> >>>> about alignment, which would very likely fail one way or the other OR we
> >>>> can say that for SPL as a special case, we enforce some alignment.
> >>>
> >>> It's likely the case that on arm32 as there's no natural alignment
> >>> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
> >>> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
> >>> DTB.  Which is why we should probably move the alignment here to 8 bytes
> >>> instead of 4.
> >>>
> >>>> But that in turn fails for fitImage with embedded data, where the
> >>>> embedded data are always aligned to 4 bytes, because that's how DTC
> >>>> aligns properties.
> >>>
> >>> I think the answer is that the use-case you're talking about is simply
> >>> going to require data to be relocated.
> >>
> >> I have a feeling that no matter how much you try to pad when generating
> >> fitImage from U-Boot, there will always be a case where that will fail.
> >> I listed at least two:
> >> - fitImage with embedded data, 4byte alignment due to DTC
> >> - Older fitImages, 4byte alignment, fails on arm64
> >> - Someone can generate signed fitImage with older mkimage => fail
> >>
> >> So that relocation logic or at least warning or something should be in
> >> there, no matter what.
> > 
> > There's two distinct areas here, and they keep being conflated.
> > 
> > The case of SPL and a FIT image for U-Boot+DTB.  We've always aligned
> > this to 4 bytes and it's worked.  I think if someone looked at the ARM
> > ARM for aarch64 you could reason out that "4-but-not-8-byte aligned
> > pointers are slow but work" as why this wasn't a hard fail on aarch64.
> 
> But we had hard-fault on arm64, see
> [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses

You're mixing the issues again.  That's an example of "device tree
properties are only 4 byte aligned" and we can't do what we were doing.
I'm not even sure reverting e8c2d25845c7 would have helped in that case.
It's also not the case we're talking about with respect to padding the
start of embedded data.

> > We should adjust our current alignment up to cover that and move on.
> 
> Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?

8 bytes is the defined requirement for everything today that defines a
required _start_ alignment.

> You will fail here either way, since if you build the fitImage with
> embedded data, the embedded data will be aligned to 4 bytes, because DT
> properties are aligned to 4 bytes.

Yes, there's the difference between "the device tree itself must be
aligned to 8 bytes" and "device tree internally is 4 byte aligned".  The
latter means that some operations are simply not possible and a feature
of the format.

> > The case of FIT images and "kernel_noload" / fdt_high=-1 /
> > initrd_high=-1 and aarch64.  If you load a FIT image in to memory and
> > try and use it as-is, it will not work.  It's not even possible in the
> > general case as you would have to inspect the kernel, see what the
> > text_offset is and build a FIT image that took that in to account, to
> > not have to move the Image around.  The device tree will almost
> > certainly be misaligned and still need to be relocated.  This is why a
> > while back I sent out an email asking every maintainer of a board that
> > disabled device tree relocation to stop that.  Perhaps a run-time patch
> > to scream about this rather than note it as we do today would help (see
> > common/image-fdt.c::boot_relocate_fdt()).
> 
> I have a feeling we should do the relocation either way. And if there is
> some special limited case (like the SPL), we should warn about it and
> push mkimage with e.g. -B 8 flag to enforce the alignment.

Please send patches so we can see what that looks like, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200511/e0bc77eb/attachment.sig>

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Marek Vasut]

On 5/11/20 9:36 PM, Tom Rini wrote:
> On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote:
>> On 5/8/20 9:21 PM, Tom Rini wrote:
>>> On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
>>>> On 5/8/20 8:47 PM, Tom Rini wrote:
>>>>> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
>>>>>> On 5/7/20 10:46 PM, Samuel Holland wrote:
>>>>>>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>>>>>>>>>> I'm not sure that it is.  Can we easily/safely memmove the data to be
>>>>>>>>>>>> aligned?  Is that really a better option in this case than ensuring
>>>>>>>>>>>> alignment within the file?
>>>>>>>>>>>
>>>>>>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and
>>>>>>>>>>> only IFF it is required ? 
>>>>>>>>>>
>>>>>>>>>> Perhaps.  But..
>>>>>>>>>>
>>>>>>>>>>> Then we can enforce it separately for 32bit
>>>>>>>>>>> and 64bit platforms to 4 and 8 bytes respectively even.
>>>>>>>>>>
>>>>>>>>>> It's 8 bytes for both.  It's possible that Linux doesn't hard fail if
>>>>>>>>>> you only do 4 byte alignment but the documented requirement is 8, for
>>>>>>>>>> arm32.
>>>>>>>>>
>>>>>>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB
>>>>>>>>> for arm64 for example.
>>>>>>>>
>>>>>>>> For arm64 you have to move it to where text_offset says it needs to be.
>>>>>>>> For arm32 the common (always, practically?) case is you're firing off
>>>>>>>> the zImage which does what's needed.  But..
>>>>>>>>
>>>>>>>>> And what you usually parse in-place would be the DT then.
>>>>>>>>
>>>>>>>> Yes, the practical case is that it's a DT and that needs 8 byte
>>>>>>>> alignment.  And we should just get back to aligning that correctly.
>>>>>>>> Going back to the v1 thread, it turns out the answer to "why do we even
>>>>>>>> have this padding?" is "we need the DT to be aligned".
>>>>>>>
>>>>>>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I
>>>>>>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte
>>>>>>> alignment of external data after the FIT. In spl_load_simple_fit():
>>>>>>>
>>>>>>>  /*
>>>>>>>   * For FIT with external data, figure out where the external images
>>>>>>>   * start. This is the base for the data-offset properties in each
>>>>>>>   * image.
>>>>>>>   */
>>>>>>>  size = fdt_totalsize(fit);
>>>>>>>  size = (size + 3) & ~3;
>>>>>>>  size = board_spl_fit_size_align(size);
>>>>>>>  base_offset = (size + 3) & ~3;
>>>>>>
>>>>>> Somehow this doesn't match the 8-byte alignment Tom was suggesting.
>>>>>> And that only leads me to believe that we can either make assumptions
>>>>>> about alignment, which would very likely fail one way or the other OR we
>>>>>> can say that for SPL as a special case, we enforce some alignment.
>>>>>
>>>>> It's likely the case that on arm32 as there's no natural alignment
>>>>> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure
>>>>> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the
>>>>> DTB.  Which is why we should probably move the alignment here to 8 bytes
>>>>> instead of 4.
>>>>>
>>>>>> But that in turn fails for fitImage with embedded data, where the
>>>>>> embedded data are always aligned to 4 bytes, because that's how DTC
>>>>>> aligns properties.
>>>>>
>>>>> I think the answer is that the use-case you're talking about is simply
>>>>> going to require data to be relocated.
>>>>
>>>> I have a feeling that no matter how much you try to pad when generating
>>>> fitImage from U-Boot, there will always be a case where that will fail.
>>>> I listed at least two:
>>>> - fitImage with embedded data, 4byte alignment due to DTC
>>>> - Older fitImages, 4byte alignment, fails on arm64
>>>> - Someone can generate signed fitImage with older mkimage => fail
>>>>
>>>> So that relocation logic or at least warning or something should be in
>>>> there, no matter what.
>>>
>>> There's two distinct areas here, and they keep being conflated.
>>>
>>> The case of SPL and a FIT image for U-Boot+DTB.  We've always aligned
>>> this to 4 bytes and it's worked.  I think if someone looked at the ARM
>>> ARM for aarch64 you could reason out that "4-but-not-8-byte aligned
>>> pointers are slow but work" as why this wasn't a hard fail on aarch64.
>>
>> But we had hard-fault on arm64, see
>> [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses
> 
> You're mixing the issues again.  That's an example of "device tree
> properties are only 4 byte aligned" and we can't do what we were doing.
> I'm not even sure reverting e8c2d25845c7 would have helped in that case.
> It's also not the case we're talking about with respect to padding the
> start of embedded data.

No, I am not mixing any issues again. These issues are all
interconnected, which is why this is a valid example of the problems we
have with the padding.

>>> We should adjust our current alignment up to cover that and move on.
>>
>> Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?
> 
> 8 bytes is the defined requirement for everything today that defines a
> required _start_ alignment.

Except DT properties are 4 byte aligned, so that's not true.

>> You will fail here either way, since if you build the fitImage with
>> embedded data, the embedded data will be aligned to 4 bytes, because DT
>> properties are aligned to 4 bytes.
> 
> Yes, there's the difference between "the device tree itself must be
> aligned to 8 bytes" and "device tree internally is 4 byte aligned".  The
> latter means that some operations are simply not possible and a feature
> of the format.

So fitImage with embedded data should not be possible at all ?

>>> The case of FIT images and "kernel_noload" / fdt_high=-1 /
>>> initrd_high=-1 and aarch64.  If you load a FIT image in to memory and
>>> try and use it as-is, it will not work.  It's not even possible in the
>>> general case as you would have to inspect the kernel, see what the
>>> text_offset is and build a FIT image that took that in to account, to
>>> not have to move the Image around.  The device tree will almost
>>> certainly be misaligned and still need to be relocated.  This is why a
>>> while back I sent out an email asking every maintainer of a board that
>>> disabled device tree relocation to stop that.  Perhaps a run-time patch
>>> to scream about this rather than note it as we do today would help (see
>>> common/image-fdt.c::boot_relocate_fdt()).
>>
>> I have a feeling we should do the relocation either way. And if there is
>> some special limited case (like the SPL), we should warn about it and
>> push mkimage with e.g. -B 8 flag to enforce the alignment.
> 
> Please send patches so we can see what that looks like, thanks!

Maybe we should remove support for embedded data from fitImage and then
enforce padding ?

[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data

[Alex Kiernan]

On Tue, May 5, 2020 at 7:41 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 5 May 2020 at 11:50, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
> > > On 5/5/20 6:37 PM, Alex Kiernan wrote:
> > > > On Tue, May 5, 2020 at 2:28 PM Marek Vasut <marex@denx.de> wrote:
> > > >>
> > > >> On 5/5/20 3:22 PM, Alex Kiernan wrote:
> > > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini <trini@konsulko.com> wrote:
> > > >>>>
> > > >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> > > >>>>
> > > >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes,
> > > >>>>> while fitImage without external data does not have any such padding and
> > > >>>>> is often unaligned. DT spec also does not mandate any such padding.
> > > >>>>>
> > > >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data,
> > > >>>>> which could lead to a potential information leak.
> > > >>>>>
> > > >>>>> $ echo -n xy > /tmp/data ; \
> > > >>>>>       ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
> > > >>>>>       hexdump -vC /tmp/fitImage | tail -n 3
> > > >>>>>
> > > >>>>> before:
> > > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > > >>>>> 00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
> > > >>>>>                    ^^       ^^ ^^
> > > >>>>> after:
> > > >>>>> 00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69  |a-offset.data-si|
> > > >>>>> 00000270  7a 65 00 78 79                                    |ze.xy|
> > > >>>>>
> > > >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> > > >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> > > >>>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > >>>>> Cc: Tom Rini <trini@konsulko.com>
> > > >>>>
> > > >>>> Applied to u-boot/master, thanks!
> > > >>>>
> > > >>>
> > > >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot,
> > > >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it
> > > >>> from eMMC I get nothing at all on the console, if I boot over ymodem
> > > >>> it stalls at 420k, before continuing to 460k. My guess is there's some
> > > >>> error going to the console at the 420k mark, but obviously it's lost
> > > >>> in the ymodem... I have two DTBs in the FIT image, 420k would about
> > > >>> align to the point between them.
> > > >>
> > > >> My bet would be on some padding / unaligned access problem that this
> > > >> patch uncovered. Can you take a look ?
> > > >
> > > > Seems plausible. With this change my external data starts at 0x483 and
> > > > everything after it is non-aligned:
> > >
> > > Should the beginning of external data be aligned ?
> >
> > If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the
> > problem go away?  If so, that's not a fix outright, it means we need to
> > dig back in to the libfdt thread and find the "make this work without
> > killing performance everywhere all the time" option.
>
> If it is a device tree, it must be 32-bit aligned.
>
> But Marek's patch affects the FIT image itself, so I am not sure what
> would go after that.
>

Just reading the commit log, the example in there shows the padding
after the FIT image being dropped:

    $ echo -n xy > /tmp/data ; \
            ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \
            hexdump -vC /tmp/fitImage | tail -n 3

    before:
    00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69
|a-offset.data-si|
    00000270  7a 65 00 00 78 79 64 64                           |ze..xydd|
                       ^^       ^^ ^^
    after:
    00000260  61 2d 6f 66 66 73 65 74  00 64 61 74 61 2d 73 69
|a-offset.data-si|
    00000270  7a 65 00 78 79                                    |ze.xy|

Though I can't reproduce that result with or without the associated commit.

--
Alex Kiernan