[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
Tom Rini
trini at konsulko.com
Wed May 6 19:02:48 CEST 2020
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 at 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 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.
> >>>>>>>>
> >>>>>>>> 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>
More information about the U-Boot
mailing list