[PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing

[Philippe REYNES <philippe.reynes@softathome.com>]

Hi Farhan,

Le 30/03/2021 ? 01:10, Farhan Ali a ?crit?:
> Phillipe,
>
> In our implementation?we store?our binaries outside the FIT header, 
> and introduce a gap between the header and the start of binary data 
> (-p and -E option in mkimage). After the FIT has been generated, we 
> sign the FIT header and insert the signature into this gap. The weak 
> function then checks the signature after 'only' the header has been 
> loaded, but before any of the FIT fields have been parsed.
>
> Whatever common implementation we decide on, it is imperative that the 
> signature can be inserted 'AFTER' the complete FIT has been generated. 
> The reason this is so critical?is to allow for off-line signing via 
> customer HSMs.
>
I think we are in line. I just sent a patch that add a stage pre-load to 
the command bootm.
The idea is to add a header to the fit image with:
- a magic : to check that it is a header with a signature
- the size of the image : to know what size should be checked
- the size of the signature
- the signature
All others information are in the u-boot device (header size, public 
key, ...), so the header is minimal.
I think that his "header" is compatible with the patch you have sent.

I? also plan to support multiple cascaded headers. So we could also 
cipher the full fit, or compress the full fit, or any other idea ...


> Regards,
> Farhan
>

Best regards,
Philippe


> On Wed, Mar 24, 2021 at 12:09 AM Simon Glass <sjg@chromium.org 
> <mailto:sjg@chromium.org>> wrote:
>
>     Hi Philippe,
>
>     On Wed, 24 Mar 2021 at 06:16, Philippe REYNES
>     <philippe.reynes@softathome.com
>     <mailto:philippe.reynes@softathome.com>> wrote:
>     >
>     > Hi Simon and Alex,
>     >
>     > Le 23/03/2021 ? 01:56, Simon Glass a ?crit :
>     > > Hi Alex,
>     > >
>     > > On Tue, 23 Mar 2021 at 04:12, Alex G. <mr.nuke.me@gmail.com
>     <mailto:mr.nuke.me@gmail.com>> wrote:
>     > >> On 3/22/21 9:27 AM, Philippe REYNES wrote:
>     > >>> Hi all,
>     > >>>
>     > >>>
>     > >>> Le 11/03/2021 ? 00:10, Alex G a ?crit :
>     > >> [snip]
>     > >>> I reach the same issue, my customers are also worried with
>     the actual
>     > >>> signature check scheme on u-boot.
>     > >>> The fit data/node are parsed before being checked : data
>     should be used
>     > >>> only after being checked, not before.
>     > >>> The code become quite complex for a signature, and the more
>     complex the
>     > >>> code is more risk to have/introduce a bug or security issue.
>     > >> [snip]
>     > >>
>     > >>>>> The reason I used a weak function was to mirror the already
>     > >>>>> upstreamed board_spl_fit_post_load(),
>     > >>>> I see why you'd think it was a good idea.
>     board_spl_fit_pre_load()
>     > >>>> sneaks in a dependency on arch-specific code
>     (CONFIG_IMX_HAB). I don't
>     > >>>> really like the way it's implemented, and I don't know if
>     it would
>     > >>>> work with SPL_LOAD_FIT_FULL or bootm.
>     > >>>>
>     > >>> As I reach the same issue, I was also thinking strongly
>     about adding a
>     > >>> "hook" before the fit image is launched/analyzed. In my mind
>     this "pre
>     > >>> load" function should be able to do some check/update to the
>     fit image,
>     > >>> but also modify the beginning of the fit image (to remove a
>     header for
>     > >>> example). Such function/feature may allow to:
>     > >>> - check a signature for the full fit (without parsing the node)
>     > >>> - cipher the full fit (even the node)
>     > >>> - compress the full fit
>     > >>> - probably that users will find a lot of others ideas .....
>     > >>>
>     > >>> I think that this feature pre load should be implemented in
>     spl and
>     > >>> bootm command.
>     > >>>
>     > >>> I have understood the feedback about a useful
>     implementation/usage of
>     > >>> pre_load.
>     > >>> I propose to sent an example soon (probably based on
>     signature check).
>     > >> So "what" you want to do is verify untrusted metadata before
>     using it.
>     > >> That's a very logical and reasonable thing to do.
>     > >>
>     > >> "How" you are trying to do this is by
>     > >>? ? (1) adding a weak function
>     > >>? ? (2) allowing each board to have a completely different
>     implementation
>     > >>
>     > >> Those are two terrible ideas.
>     > >>
>     > >> I agree that there is a deficiency in the way FIT images are
>     signed. Can
>     > >> we stick the signature between the fdt_header and before
>     dt_struct?
>     > > That seems like a reasonable idea to me. Even better might be
>     to have
>     > > it completely separate, e.g. before the FIT starts, so no
>     parsing at
>     > > all is needed?
>     >
>     >
>     > That's my idea, a header with only the minimum information (like fit
>     > size and signature).
>     > The information about the signature (hash, algo, padding, public
>     key,
>     > ...) may be stored
>     > in the u-boot device tree. So u-boot won't parse the fit image, only
>     > compute the hash
>     > to check the signature.
>     >
>     > >
>     > > Also, which signature? FIT supports multiple signatures which
>     can be
>     > > added at different times. Perhaps this could be for a base
>     signature,
>     > > enough to get through to verifying the 'real' signature.
>     >
>     >
>     > I was thinking that the signature information could be stored in the
>     > u-boot device tree
>     > (or hardcoded in the u-boot configuration). The idea is to have
>     a very
>     > simple header.
>     > I also think that this signature may be used with the signature
>     in the
>     > fit.? It is two
>     > options, so users may eanble both options.
>     >
>     > As we agree on the principle, I will sent a RFC asap.
>
>     You can store the public key (or whatever is used) in the U-Boot
>     devicetree, but the signature presumably has to be attached to the
>     FIT, right?
>
>     Regards,
>     Simon
>