Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Akashi-san,

On Tue, Jan 18, 2022 at 09:38:22PM +0900, AKASHI Takahiro wrote:
> Hi Ilias,
> 
> On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote:
> > Right now the code explicitly limits us to sha1,256 hashes with RSA2048
> > encryption.  But the limitation is artificial since U-Boot supports
> > a wider range of algorithms.
> > 
> > The internal image_get_[checksum|crypto]_algo() functions expect an
> > argument in the format of <checksum>,<crypto>.  So let's remove the size
> > checking and create the needed string on the fly in order to support
> > more hash/signing combinations.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  lib/crypto/public_key.c | 27 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> > index df6033cdb499..b783c63f5a51 100644
> > --- a/lib/crypto/public_key.c
> > +++ b/lib/crypto/public_key.c
> > @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey,
> >  				const struct public_key_signature *sig)
> >  {
> >  	struct image_sign_info info;
> > +	char algo[256];
> >  	int ret;
> >  
> >  	pr_devel("==>%s()\n", __func__);
> > @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey,
> >  		return -EINVAL;
> >  
> >  	memset(&info, '\0', sizeof(info));
> > +	memset(algo, 0, sizeof(algo));
> >  	info.padding = image_get_padding_algo("pkcs-1.5");
> >  	/*
> >  	 * Note: image_get_[checksum|crypto]_algo takes a string
> >  	 * argument like "<checksum>,<crypto>"
> >  	 * TODO: support other hash algorithms
> >  	 */
> 
> If this patch is applied, the TODO comment above will make no sense :)

We are still only handle SHA,  but there's a printable error now, so i'll
get rid of the comment. 

> 
> > -	if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {
> > -		pr_warn("Encryption is not RSA2048: %s%d\n",
> > -			sig->pkey_algo, sig->s_size * 8);
> > -		return -ENOPKG;
> > -	}
> > -	if (!strcmp(sig->hash_algo, "sha1")) {
> > -		info.checksum = image_get_checksum_algo("sha1,rsa2048");
> > -		info.name = "sha1,rsa2048";
> > -	} else if (!strcmp(sig->hash_algo, "sha256")) {
> > -		info.checksum = image_get_checksum_algo("sha256,rsa2048");
> > -		info.name = "sha256,rsa2048";
> > -	} else {
> > -		pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
> > +	if (strcmp(sig->pkey_algo, "rsa")) {
> > +		pr_err("Encryption is not RSA: %s\n", sig->pkey_algo);
> >  		return -ENOPKG;
> >  	}
> > +	ret = snprintf(algo, sizeof(algo), "%s,%s%d", sig->hash_algo,
> > +		       sig->pkey_algo, sig->s_size * 8);
> 
> I'm not sure that this naming rule, in particular the latter part, will
> always hold in the future while all the existing algo's observe it.
> (Maybe we need some note somewhere?)

The if a few lines below will shield us and return -EINVAL.  How about
adding an error message there? 

Cheers
/Ilias
> 
> -Takahiro Akashi
> 
> > +
> > +	if (ret >= sizeof(algo))
> > +		return -EINVAL;
> > +
> > +	info.checksum = image_get_checksum_algo((const char *)algo);
> > +	info.name = (const char *)algo;
> >  	info.crypto = image_get_crypto_algo(info.name);
> > -	if (IS_ERR(info.checksum) || IS_ERR(info.crypto))
> > +	if (!info.checksum || !info.crypto)
> >  		return -ENOPKG;
> >  
> >  	info.key = pkey->key;
> > -- 
> > 2.30.2
> >