From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philippe REYNES Date: Mon, 22 Mar 2021 15:27:34 +0100 Subject: [PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing In-Reply-To: References: <20210224232531.22899-1-farhan.ali@broadcom.com> <20210309235530.184179-1-farhan.ali@broadcom.com> Message-ID: <389855fe-9401-83b6-350f-eb453ab214ae@softathome.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. > 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