[PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/25/24 9:10 AM, Rasmus Villemoes wrote:
> On Thu, Oct 24 2024, Marek Vasut <marex at denx.de> wrote:
> 
>> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
>>> Loading flash.bin using uuu fails when flash.bin does not have the
>>> right size.
>>> When flash.bin is loaded from some storage medium (sd card/emmc),
>>> SPL
>>> just loads some random garbage bytes from beyond what has been
>>> populated when flash.bin was written, but when loaded via uuu, SPL
>>> hangs waiting for the host to send the expected number of bytes. Which
>>> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
>>> alignment to 0x1000 is already done and is necessary in all cases
>>> because that's the exact expected location of the 32 byte IVT
>>> header. But the IVT+CSF blob tacked onto the end must be a total of
>>> CONFIG_CSF_SIZE.
>>> This is exactly the same fix as 89f19f45d650, except that this time
>>> around I don't know how to cleanly get CONFIG_CSF_SIZE.
>>> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M
>>> flash.bin signing)
>>> Signed-off-by: Rasmus Villemoes <ravi at prevas.dk>
>>> ---
>>> Heiko, can you check if this works for you?
>>> And if somebody wants to pick this up and knows how to get at
>>> CONFIG_
>>> values, feel free to fix up and take authorship. But perhaps it's not
>>> really configurable at all; imx8mimage.c has the value 0x2000
>>> hard-coded, so I don't think anything good could ever come from
>>> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
>>> make that knob non-settable.
>>>    tools/binman/etype/nxp_imx8mcst.py | 2 ++
>>>    1 file changed, 2 insertions(+)
>>> diff --git a/tools/binman/etype/nxp_imx8mcst.py
>>> b/tools/binman/etype/nxp_imx8mcst.py
>>> index 8221517b0c4..9a1974cc522 100644
>>> --- a/tools/binman/etype/nxp_imx8mcst.py
>>> +++ b/tools/binman/etype/nxp_imx8mcst.py
>>> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>>>            args = ['-i', cfg_fname, '-o', output_fname]
>>>            if self.cst.run_cmd(*args) is not None:
>>>                outdata = tools.read_file(output_fname)
>>> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
>>> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>>>                return data + outdata
>>>            else:
>>>                # Bintool is missing; just use the input data as the output
>>
>> I have to admit, I never really figured out this binman stuff, but
>> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ?
> 
> No, why? That logic is all about generating the imx-specific header in
> front of SPL.bin, there's no CSF being generated.

This patch is modifying tools/binman/etype/nxp_imx8mcst.py which is the 
Code-Signing-Tool binman etype, the thing used for generating SIGNED 
images during HAB use.

Shouldn't this patch modify tools/binman/etype/nxp_imx8mimage.py which 
is the etype used when generating flash.bin itself ?

My understanding was that the problem Heiko reported was alignment of 
flash.bin , independently of it being signed or not ?

> Or maybe mkimage tacks
> on some dummy bytes, but then that's explicitly ignored by the signing
> step, I think that's what the
> 
>              signbase -= 0x40
>              signsize = struct.unpack('<I', data[24:28])[0] - signbase
>              # Remove mkimage generated padding from the end of data
>              data = data[:signsize]
> 
> is about, so the signing step generates a new IVT and a new (real) CSF
> for the SPL. And none of that matters for the size of the final
> flash.bin, because the FIT image is located far ahead at a fixed 0x58000
> position.
> 
>> And ... shouldn't it somehow use SetImagePos() ?
> 
> Again, why? I'm padding a blob (in this case the CSF data) to a required
> size before tacking it on. Exactly as the existing code does
> 
>              # Align fitImage to 4k
>              signsize = tools.align(len(data), 0x1000)
>              data += tools.get_bytes(0, signsize - len(data))
> 
> before writing out the (padded FIT image + 32 byte IVT) for cst to chew
> on and generate the CSF data.
> 
> This is all about ensuring that the FIT+IVT+CSF blob has (exactly) the size
> computed by board_spl_fit_size_align, which is
> 
>          size = ALIGN(size, 0x1000);
>          size += CONFIG_CSF_SIZE;
> 
> The 0x1000 part is done in the existing code, I'm just making sure that
> the data we append after that is exactly CONFIG_CSF_SIZE. The CSF blob
> itself is what is originally in outdata, and because the IVT must be
> included in the data which is signed, the IVT has already been appended
> after the 0x1000 padded FIT image; that's why the computation of the
> extra bytes ends up being a little complicated. [If we just appended a
> full CONFIG_CSF_SIZE bytes, the board would boot, but then uuu would be
> hanging while waiting to deliver the remaining part of the file, so the
> size should match exactly.]
> 
> I also don't grok the binman stuff, but there's really no separate image
> to set a "position" for, the csf blob (or ivt+csf blob) is not
> represented as its own image in the binman description.
It is probably best to let Simon comment on this.