On Wed, Mar 10, 2021 at 11:38 AM Alex G. mr.nuke.me@gmail.com wrote:
On 3/9/21 5:55 PM, Farhan Ali wrote:
This change adds a callback for preprocessing the FIT header before it is parsed. There are 3 main reasons for this callback:
(1) If a vulnerability is discovered in the FIT parsing/loading code, or libfdt, this callback allows users to scan the FIT header for specific exploit signatures and prevent flashing/booting of the image
(2) If users want to implement a single signature which covers the entire FIT header, which is then appended to the end of the header, then this callback can be used to authenticate that signature.
(3) If users want to check the FIT header contents against specific metadata stored outside the FIT header, then this callback allows them to do that.
Hi Fahran,
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.
Hi Alex,
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 The 'how' is this: A weak function which is invoked right after the fit HEADER ONLY is read.
There should be at least a use case implemented in this series. When
someone notices an empty function that isn't used, the first instinct would be to submit a patch to remove it. But more importantly, seeing an actual example of "what" you are trying to achieve will help others suggest a better way on "how" to achieve it.
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 ). This speeds up the signing process for production/development builds. We want to be able validate this signature before the FIT parsing starts. Signature is stored elsewhere, outside the FIT header.
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
The third issue is that whatever the end goal is, you're trying to
achieve it with weak functions. Weak functions aren't always bad. There are a limited number of cases where they work very well for the purpose -- I've even used them before. But to introduce a weak function, a really strong argument is needed. Maybe you have it, but that argument needs to be made clear.
The reason I used a weak function was to mirror the already
upstreamed board_spl_fit_post_load(), I thought that the justifications for that function would also apply to this new board_spl_fit_pre_load(). If board_spl_fit_pre_load() is fundamentally different in some aspect, then I am happy to look for an alternative implementation.
Regards,
Farhan
Let's assume board 'c' implements this. Then later someone with board 'd' implements this at the SOC level. Does board 'c' get the old implementation, or the new? Things become ambiguous in everything but the simplest of cases.
A more elegant way would be to have a proper interface to hook into the FIT processing. That could be done by a function pointer, ops structure, or perhaps new UCLASS (Simon?).
Alex
Signed-off-by: Farhan Ali farhan.ali@broadcom.com Cc: Simon Glass sjg@chromium.org Cc: Alexandru Gagniuc mr.nuke.me@gmail.com Cc: Marek Vasut marex@denx.de Cc: Michal Simek michal.simek@xilinx.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Samuel Holland samuel@sholland.org
Changes for v2: - Callback now returns a value - Added a log message on failure
common/spl/spl_fit.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 75c8ff0..01aee1c 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -43,6 +43,14 @@ __weak ulong board_spl_fit_size_align(ulong size) return size; }
+__weak int board_spl_fit_pre_load(struct spl_load_info *load_info,
const void *fit,ulong start_sector,ulong loaded_sector_count)+{
return 0;+}
- static int find_node_from_desc(const void *fit, int node, const char
*str)
{ int child; @@ -527,6 +535,7 @@ static int spl_simple_fit_read(struct spl_fit_info
*ctx,
unsigned long count, size; int sectors; void *buf;
int rc = 0; /* * For FIT with external data, figure out where the external images@@ -552,7 +561,18 @@ static int spl_simple_fit_read(struct spl_fit_info
*ctx,
debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu,size=0x%lx\n",
sector, sectors, buf, count, size);
return (count == 0) ? -EIO : 0;
if (count) {/* preprocess loaded fit header before parsing and loadingbinaries */
rc = board_spl_fit_pre_load(info, fit_header, sector,sectors);
if (rc) {debug("%s: fit header pre processing failed.rc=%d\n",
__func__, rc);}} else {rc = -EIO;}return rc;}
static int spl_simple_fit_parse(struct spl_fit_info *ctx)