Re: [PATCH 1/2] image-board.c: exempt gd->fdt_blob from fit_check_format() check

[Rasmus Villemoes <ravi@prevas.dk>]

On Fri, May 15 2026, Simon Glass <sjg@chromium.org> wrote:

> Hi Rasmus,
>
> On 2026-05-12T16:16:29, Rasmus Villemoes <ravi@prevas.dk> wrote:
>> image-board.c: exempt gd->fdt_blob from fit_check_format() check
>>
>> Having scripts embedded one way or the other in the U-Boot binary
>> means they are automatically verified/trusted by whatever mechanism
>> verifies U-Boot.
>>
>> Writing those scripts in the built-in environment leads to
>> backslatitis and missing or wrong quoting and is generally not very
>> readable or maintainable.
>>
>> Maintaining scripts in external files allows one
>> to have both syntax highlighting and to some extent apply shellcheck
>> on it (though U-Boot's shell is of course not quite POSIX sh, so some
>> '#shellcheck disable' directives are needed). Getting those into the
>> U-Boot binary is then a matter of having a suitable .dtsi file such as
>>
>> / {
>>         images {
>>                 default = 'boot';
>>                 boot {
>> [...]
>>
>> boot/image-board.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Makes sense, and a nice feature.
>
>> diff --git a/boot/image-board.c b/boot/image-board.c
>> @@ -1037,7 +1037,7 @@ int image_locate_script(void *buf, int size, const char *fit_uname,
>>                       goto exit_image_format;
>>               } else {
>>                       fit_hdr = buf;
>> -                     if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
>> +                     if (fit_hdr != gd->fdt_blob && fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
>>                               puts("Bad FIT image format\n");
>>                               return 1;
>>                       }
>
> Please add a code comment explaining why the control DTB is exempt - a
> future reader running git blame will be puzzled by the pointer
> comparison. Something like 'gd->fdt_blob has already been validated by
> the bootloader and is by definition trusted, so we skip the strict FIT
> format checks (no description/timestamp, presence of unit-addresses,
> ...) for that buffer'.

Yes, I agree this was too terse, it was mostly to demonstrate how little
was actually needed to enable this use case.

> Would it be cleaner to push the trusted-FIT notion into
> fit_check_format() itself (e.g. a sibling fit_check_format_trusted()
> that skips the strict-format / no-@ pieces)? The pointer-equality test
> works but feels out of place in image_locate_script(), and the same
> need will likely show up the next time someone wants to
> source-from-DTB elsewhere, such as an FPGA image. What do you think?

Yes, I agree it's cleaner to do the exemption inside
fit_check_format(). I do want to exclude the timestamp/description
sanity checks, as I don't really like to have to add such properties at
the top level to the control DTB just to satisfy those sanity checks - I
only mentioned that it would be possible to do so.

As for a comment, I'm not sure that's needed as much in v2 now that the
check is

	if (CONFIG_IS_ENABLED(CONTROL_DTB_AS_FIT) && fit == gd_fdt_blob())
		return 0;

and git blame would show the added config option and the reference to
the documentation.

I've added that after the basic fdt_check_header(fit), which really
should never fail for the control DTB, but if it does, it's better that
we don't continue trying to parse it as a FIT.


Rasmus