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

[Alex G]

On 3/10/21 2:49 PM, Farhan Ali wrote:
> On Wed, Mar 10, 2021 at 11:38 AM Alex G. <mr.nuke.me at gmail.com 
>     This patch describes "how" you're trying to achieve it, but "what" you
>     want to achieve. I'll get later into why I think the "how" is
>     fundamentally flawed.
> 
> The 'what' is basically this: I want to be able to parse the fit header 
> for correctness before
> any image loading takes place. This 'correctness' will be user defined

I'd expect such code to be part of this series. Having a function that a 
"user" might define sounds a lot like a vendor-specific hook with no 
upstream code, hence the skepticism. This series should include a useful 
implementation of board_spl_fit_pre_load().


>   The main use case for us is two folds:
> (1) Customers are worried about our reliance on libfdt for FIT parsing 
> and want to prescan the FIT header to
> check for any future exploits
> (2) We implement a signature on the entire FIT header ( instead of 
> individual nodes ).

Do you believe the current FIT signing scheme is inappropriate for your 
needs? Have you looked at signed configs? Is there a reason why they are 
not appropriate?

There was a potential issue where a bad FIT could place itself anywhere 
in memory. This was fixed in commit 03f1f78a9b ("spl: fit: Prefer a 
malloc()'d buffer for loading images"). Keep in mind that, in this case, 
checking the FIT header would not have guarded against the exploit.


>     Second issue is that spl_simple_fit_read() is intended to bring a FIT
>     image to memory. If you need to make decisions on the content of that
>     image, then spl_simple_fit_read() is the wrong place to do it. A better
>     place might be spl_simple_fit_parse().
> 
> spl_simple_fit_parse()  parses the 'contents' of the fit using standard 
> APIs. We need to check
> the FIT header for correctness BEFORE its contents are parsed, using a 
> user defined 'safe'
> parsing function. The standard FIT loading flow checks for only a few 
> things ( hashes/configuration etc),
> there can be a lot of other USER defined checks which may need to be 
> checked. This callback will achieve this

This patch is calling board_spl_fit_pre_load() after the FIT is read. On 
a FIT with embedded data, you've also loaded all the binaries. It seems 
that checking a header now is a moot point.

If you need to make sure that the FIT wasn't tampered, the signed 
configs were designed exactly for that. You mentioned earlier that you 
want to sign the FIT header. What is the FIT header in this case? Is it 
the FDT of a FIT with external data? Is it struct fdt_header?


> 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.

Alex