Hi all,
Le 11/03/2021 à 00:10, Alex G a écrit :
On 3/10/21 2:49 PM, Farhan Ali wrote:
On Wed, Mar 10, 2021 at 11:38 AM Alex G. <mr.nuke...@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.
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.
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?
As mentioned above, we (my company and my customer) thought that the fit
node should only be used after the fit signature is verified (and OK).
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).
Regards,
Philippe
Alex
