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