[PATCH 6/8] lib: crypto: export and enhance pkcs7_verify_one()

[AKASHI Takahiro]

Heinrich,

On Wed, Jun 10, 2020 at 11:08:18PM +0200, Heinrich Schuchardt wrote:
> On 6/9/20 7:13 AM, AKASHI Takahiro wrote:
> > The function, pkcs7_verify_one(), will be utilized to rework signature
> > verification logic aiming to support intermediate certificates in
> > "chain of trust."
> >
> > To do that, its function interface is expanded, adding an extra argument
> > which is expected to return the last certificate in trusted chain.
> > Then, this last one must further be verified with signature database, db
> > and/or dbx.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  include/crypto/pkcs7.h    |  9 ++++++++-
> >  lib/crypto/pkcs7_verify.c | 36 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
> > index 8f5c8a7ee3b9..ca35df29f6fb 100644
> > --- a/include/crypto/pkcs7.h
> > +++ b/include/crypto/pkcs7.h
> > @@ -27,7 +27,14 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
> >  				  const void **_data, size_t *_datalen,
> >  				  size_t *_headerlen);
> >
> > -#ifndef __UBOOT__
> > +#ifdef __UBOOT__
> > +struct pkcs7_signed_info;
> > +struct x509_certificate;
> > +
> > +int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > +		     struct pkcs7_signed_info *sinfo,
> > +		     struct x509_certificate **signer);
> > +#else
> >  /*
> >   * pkcs7_trust.c
> >   */
> > diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
> > index 8b5cd7a443ae..220ce9f77cf0 100644
> > --- a/lib/crypto/pkcs7_verify.c
> > +++ b/lib/crypto/pkcs7_verify.c
> > @@ -295,8 +295,14 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,
> >  /*
> >   * Verify the internal certificate chain as best we can.
> Hello Takahiro,
> 
> please, add a function description. See
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

Sure

> >   */
> > +#ifdef __UBOOT__
> > +static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> > +				  struct pkcs7_signed_info *sinfo,
> > +				  struct x509_certificate **signer)
> > +#else
> >  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  				  struct pkcs7_signed_info *sinfo)
> 
> I would prefer if you could change this to a single signature with the
> argument 'signer' which is required below anyway.

I don't get your point here.

> Please, remove all #ifdef __UBOOT__.
> 
> Functions that are not always used can be marked as __maybe_unused if
> this is required to avoid compiler warnings.

The purpose of __UBOOT__ is to distinguish my changes from the original
code that was imported from linux. I believe that it would make
it much easier to catch up and apply the following commits in linux.
Since there is no explicit custodian for lib/crypto, it is crucial
to keep such a maintenance effort as simple as possible.

I adopted this style of import also in other files under lib/crypto.

Thanks,
-Takahiro Akashi

> > +#endif
> >  {
> >  	struct public_key_signature *sig;
> >  	struct x509_certificate *x509 = sinfo->signer, *p;
> > @@ -305,6 +311,8 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >
> >  	kenter("");
> >
> > +	*signer = NULL;
> > +
> >  	for (p = pkcs7->certs; p; p = p->next)
> >  		p->seen = false;
> >
> > @@ -322,6 +330,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  			for (p = sinfo->signer; p != x509; p = p->signer)
> >  				p->blacklisted = true;
> >  			pr_debug("- blacklisted\n");
> > +#ifdef __UBOOT__
> > +			*signer = x509;
> > +#endif
> >  			return 0;
> >  		}
> >
> > @@ -347,6 +358,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  				goto unsupported_crypto_in_x509;
> >  			x509->signer = x509;
> >  			pr_debug("- self-signed\n");
> > +#ifdef __UBOOT__
> > +			*signer = x509;
> > +#endif
> >  			return 0;
> >  		}
> a>
> > @@ -377,6 +391,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >
> >  		/* We didn't find the root of this chain */
> >  		pr_debug("- top\n");
> > +#ifdef __UBOOT__
> > +		*signer = x509;
> > +#endif
> >  		return 0;
> >
> >  	found_issuer_check_skid:
> > @@ -394,6 +411,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  		if (p->seen) {
> >  			pr_warn("Sig %u: X.509 chain contains loop\n",
> >  				sinfo->index);
> > +#ifdef __UBOOT__
> > +			*signer = p;
> > +#endif
> >  			return 0;
> >  		}
> >  		ret = public_key_verify_signature(p->pub, x509->sig);
> > @@ -402,6 +422,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  		x509->signer = p;
> >  		if (x509 == p) {
> >  			pr_debug("- self-signed\n");
> > +#ifdef __UBOOT__
> > +			*signer = p;
> > +#endif
> >  			return 0;
> >  		}
> >  		x509 = p;
> > @@ -423,11 +446,14 @@ unsupported_crypto_in_x509:
> >  /*
> >   * Verify one signed information block from a PKCS#7 message.
> 
> A proper function description is missing here.
> 
> >   */
> > -#ifndef __UBOOT__
> > -static
> > -#endif
> > +#ifdef __UBOOT__
> >  int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > -		     struct pkcs7_signed_info *sinfo)
> > +		     struct pkcs7_signed_info *sinfo,
> > +		     struct x509_certificate **signer)
> > +#else
> > +static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > +			    struct pkcs7_signed_info *sinfo)
> 
> Please, use a single function signature here too.
> 
> Best regards
> 
> Heinrich
> 
> > +#endif
> >  {
> >  	int ret;
> >
> > @@ -471,7 +497,7 @@ int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> >  	pr_devel("Verified signature %u\n", sinfo->index);
> >
> >  	/* Verify the internal certificate chain */
> > -	return pkcs7_verify_sig_chain(pkcs7, sinfo);
> > +	return pkcs7_verify_sig_chain(pkcs7, sinfo, signer);
> >  }
> >
> >  #ifndef __UBOOT__
> >
>