From mboxrd@z Thu Jan 1 00:00:00 1970 From: Farhan Ali Date: Wed, 10 Mar 2021 12:49:07 -0800 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: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Mar 10, 2021 at 11:38 AM Alex G. 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 > > Cc: Simon Glass > > Cc: Alexandru Gagniuc > > Cc: Marek Vasut > > Cc: Michal Simek > > Cc: Philippe Reynes > > Cc: Samuel Holland > > > > --- > > 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 loading > binaries */ > > + 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) > > > -------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/pkcs7-signature Size: 4203 bytes Desc: S/MIME Cryptographic Signature URL: