Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature()

[Roberto Sassu]

On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote:
> 
> On 12/27/22 09:27, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > 
> > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > mapping") checks that both the signature and the digest reside in the
> > linear mapping area.
> > 
> > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced
> > stack support") made it possible to move the stack in the vmalloc area,
> > which is not contiguous, and thus not suitable for sg_set_buf() which needs
> > adjacent pages.
> > 
> > Always make a copy of the signature and digest in the same buffer used to
> > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > to conditionally doing the copy if necessary, to keep the code simple. The
> > buffer allocated with kmalloc() is in the linear mapping area.
> > 
> > Cc: stable@xxxxxxxxxxxxxxx # 4.9.x
> > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > Suggested-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > Reviewed-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> 
> I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue.
> 
> Tested-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>

Thanks, Stefan.

I did multiple attempts to have the patch included, but I didn't have
any luck with the maintainers (David, Jarkko).

It would be awesome if any maintainer picks it.

Thanks!

Roberto

> ---
> >   crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > index 2f8352e88860..49a3f7c01149 100644
> > --- a/crypto/asymmetric_keys/public_key.c
> > +++ b/crypto/asymmetric_keys/public_key.c
> > @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   	struct crypto_wait cwait;
> >   	struct crypto_akcipher *tfm;
> >   	struct akcipher_request *req;
> > -	struct scatterlist src_sg[2];
> > +	struct scatterlist src_sg;
> >   	char alg_name[CRYPTO_MAX_ALG_NAME];
> > -	char *key, *ptr;
> > +	char *buf, *ptr;
> > +	size_t buf_len;
> >   	int ret;
> >   
> >   	pr_devel("==>%s()\n", __func__);
> > @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   	if (!req)
> >   		goto error_free_tfm;
> >   
> > -	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> > -		      GFP_KERNEL);
> > -	if (!key)
> > +	buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> > +			sig->s_size + sig->digest_size);
> > +
> > +	buf = kmalloc(buf_len, GFP_KERNEL);
> > +	if (!buf)
> >   		goto error_free_req;
> >   
> > -	memcpy(key, pkey->key, pkey->keylen);
> > -	ptr = key + pkey->keylen;
> > +	memcpy(buf, pkey->key, pkey->keylen);
> > +	ptr = buf + pkey->keylen;
> >   	ptr = pkey_pack_u32(ptr, pkey->algo);
> >   	ptr = pkey_pack_u32(ptr, pkey->paramlen);
> >   	memcpy(ptr, pkey->params, pkey->paramlen);
> >   
> >   	if (pkey->key_is_private)
> > -		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
> > +		ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen);
> >   	else
> > -		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
> > +		ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen);
> >   	if (ret)
> > -		goto error_free_key;
> > +		goto error_free_buf;
> >   
> >   	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
> >   		ret = cert_sig_digest_update(sig, tfm);
> >   		if (ret)
> > -			goto error_free_key;
> > +			goto error_free_buf;
> >   	}
> >   
> > -	sg_init_table(src_sg, 2);
> > -	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
> > -	sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
> > -	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
> > +	memcpy(buf, sig->s, sig->s_size);
> > +	memcpy(buf + sig->s_size, sig->digest, sig->digest_size);
> > +
> > +	sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size);
> > +	akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size,
> >   				   sig->digest_size);
> >   	crypto_init_wait(&cwait);
> >   	akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> > @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   				      crypto_req_done, &cwait);
> >   	ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait);
> >   
> > -error_free_key:
> > -	kfree(key);
> > +error_free_buf:
> > +	kfree(buf);
> >   error_free_req:
> >   	akcipher_request_free(req);
> >   error_free_tfm: