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