Re: [PATCH v3 15/25] mbedtls: add X509 cert parser porting layer

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]

On Tue, Jun 04, 2024 at 07:53:54PM +0300, Ilias Apalodimas wrote:
> On Tue, 4 Jun 2024 at 19:05, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Hi Ilias,
> >
> > On Fri, 31 May 2024 at 07:42, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >>
> >> On Tue, 28 May 2024 at 17:15, Raymond Mao <raymond.mao@linaro.org> wrote:
> >> >
> >> > Add porting layer for X509 cert parser on top of MbedTLS X509
> >> > library.
> >> >
> >> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> >> > ---
> >> > Changes in v2
> >> > - Move the porting layer to MbedTLS dir.
> >> > Changes in v3
> >> > - None.
> >> >
> >> >  lib/mbedtls/Makefile           |   1 +
> >> >  lib/mbedtls/x509_cert_parser.c | 497 +++++++++++++++++++++++++++++++++
> >> >  2 files changed, 498 insertions(+)
> >> >  create mode 100644 lib/mbedtls/x509_cert_parser.c
> >> >
> >
> > [snip]
> >>
> >> > diff --git a/lib/mbedtls/x509_cert_parser.c b/lib/mbedtls/x509_cert_parser.c
> >> > new file mode 100644
> >> > index 00000000000..b0867d31047
> >> > --- /dev/null
> >> > +++ b/lib/mbedtls/x509_cert_parser.c
> >>
> > [snip]
> >>
> >> > +static int x509_set_cert_flags(struct x509_certificate *cert)
> >> > +{
> >> > +       struct public_key_signature *sig = cert->sig;
> >> > +
> >> > +       if (!sig || !cert->pub) {
> >> > +               pr_err("Signature or public key is not initialized\n");
> >> > +               return -ENOPKG;
> >> > +       }
> >> > +
> >> > +       if (!cert->pub->pkey_algo)
> >> > +               cert->unsupported_key = true;
> >> > +
> >> > +       if (!sig->pkey_algo)
> >> > +               cert->unsupported_sig = true;
> >> > +
> >> > +       if (!sig->hash_algo)
> >> > +               cert->unsupported_sig = true;
> >> > +
> >> > +       /* TODO: is_hash_blacklisted()? */
> >>
> >> Is this supported by our current implementation?
> >>
> > This is not supported currently either. I just copied the TODO mark
> > from legacy lib.
> >
> > [snip]
> >>
> >> > +               }
> >> > +               goto out;
> >> > +       }
> >> > +
> >> > +       pr_devel("Cert Self-signature verified");
> >> > +       cert->self_signed = true;
> >> > +
> >> > +out:
> >> > +       return ret;
> >> > +
> >> > +not_self_signed:
> >> > +       return 0;
> >> > +}
> >>
> >> the whole function looks like a copy of lib/crypto/x509_public_key.c.
> >> Can you move all the c/p ones to a common file that the existing and
> >> mbedTLS implementations can use?
> >>
> > Per a previous discussion with Tom, eventually we tend to keep only one
> > crypto lib, that is the reason I prefer to copy/optimize a few existing
> > functions into MbedTLS implementation instead of creating another
> > common file.
> 
> Regardless of the implementation, the common functions should reside
> in a common file which will be used regardless of mbedTLS or the
> existing stack.
> We do not want to fix bugs twice

And please keep in mind we already have _two_ implementations at times
today, and it will stay that way even when mbedTLS replaces legacy
options.  The ARM HW SHA256 option for example is going to likely be
used over mbedTLS SHA256.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]