[PATCH v3] mkimage: Default to 8-byte alignment for DTBs added via -b argument

[Tom Rini]

On Tue, Jan 27, 2026 at 11:28:03AM -0500, Sean Anderson wrote:
> On 1/27/26 11:06, Tom Rini wrote:
> > On Tue, Jan 27, 2026 at 01:36:59AM +0100, Marek Vasut wrote:
> >> On 1/26/26 11:58 PM, Tom Rini wrote:
> >>
> >> > > > Some validation of "len" has to be done before it is fed into strcmp() here.
> >> > >
> >> > > Well if you are concerned about lacking a nul-terminator then we will
> >> > > read at most 8 bytes beyond the string.
> >>
> >> Which is wrong, and trivial to fix -- check the "len" that is already
> >> provided to you by fdt_getprop() and make sure it is exactly length of
> >> FIT_TYPE_PROP before doing the strcmp() . Then you should be safe.
> >>
> >> > > The other thing that can go wrong is if we get something like
> >> > > "flat_dt\0extra data" we will match.
> >>
> >> This I did not consider. So maybe what needs to be done is compare the
> >> length and then memcmp() instead of strcmp().
> >>
> >> > > Surprisingly the closest we have to this construct is fdt_string_eq_. I
> >> > > suppose this is because most strings in U-Boot are guaranteed to be
> >> > > nul-terminated.
> >>
> >> fdt_string_eq_() seems like the best fit, yes , nice.
> >
> > Bad news, fdt_string_eq_() is an internal call in libfdt. Good news,
> > fdt_getprop() makes use of it via its eventual call chain. Which I think
> > gets back to what I said earlier, libfdt is doing the input sanitization
> > for us, or so many other places are more useful to exploit?
> >
> 
> No, the check in fdt_getprop is only for the name, not the value (which
> could be arbitrary binary data including many nul bytes).
> 
> I think the assumption in most of U-Boot is that the provided FDT is
> trusted (or at least not maliciously ill-formed).  functions like
> image_get_checksum_algo read up to 6 bytes beyond full_name if full_name
> == "". It's possible to read off the end of the DTB if this is the very
> last string in the strings block and there is no trailing padding. If
> we're really concerned about this (e.g. to avoid false-positives with
> ASAN), then we should just add (say) 16 bytes to the end of every DTB
> when we malloc it.

Yes, we rely on the sanity checking in libfdt, which I think the kernel
also does.

And no, I'm not sure if we care enough about all of these corner cases,
but if we do then I'm not sure this right here and now is where to
start. It should start with upstream libfdt to see whatever cases aren't
handled, and then what cases fall on the callers to deal with wrt
security implications.

Because I really, really, really, do not want to block fixing booting on
some large number of boards because now we're going to depend first on a
security audit here.

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