[PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing

[Farhan Ali]

On Wed, Mar 10, 2021 at 11:38 AM Alex G. <mr.nuke.me at 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 at broadcom.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Alexandru Gagniuc <mr.nuke.me at gmail.com>
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Michal Simek <michal.simek at xilinx.com>
> > Cc: Philippe Reynes <philippe.reynes at softathome.com>
> > Cc: Samuel Holland <samuel at 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 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: <https://lists.denx.de/pipermail/u-boot/attachments/20210310/ec52263e/attachment.bin>