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

[Tom Rini <trini@konsulko.com>]

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>