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

Wed May 6 16:27:54 CEST 2020

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 at 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 at 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 at 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 at denx.de>
> >>>>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
> >>>>>>>>>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>>>>>>>>> Cc: Tom Rini <trini at 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>