Re: [PATCH] KEYS: asymmetric: Make a copy of sig and digest in vmalloced stack

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Thu, 2022-12-08 at 15:17 -0800, Eric Biggers wrote:
> On Thu, Dec 08, 2022 at 05:46:10PM +0100, Roberto Sassu wrote:
> > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > index 2f8352e88860..307799ffbc3e 100644
> > --- a/crypto/asymmetric_keys/public_key.c
> > +++ b/crypto/asymmetric_keys/public_key.c
> > @@ -363,7 +363,8 @@ int public_key_verify_signature(const struct public_key *pkey,
> >  	struct scatterlist src_sg[2];
> >  	char alg_name[CRYPTO_MAX_ALG_NAME];
> >  	char *key, *ptr;
> > -	int ret;
> > +	char *sig_s, *digest;
> > +	int ret, verif_bundle_len;
> >  
> >  	pr_devel("==>%s()\n", __func__);
> >  
> > @@ -400,8 +401,21 @@ 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);
> > +	verif_bundle_len = pkey->keylen + sizeof(u32) * 2 + pkey->paramlen;
> > +
> > +	sig_s = sig->s;
> > +	digest = sig->digest;
> > +
> > +	if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > +		if (!virt_addr_valid(sig_s))
> > +			verif_bundle_len += sig->s_size;
> > +
> > +		if (!virt_addr_valid(digest))
> > +			verif_bundle_len += sig->digest_size;
> > +	}
> > +
> > +	/* key points to a buffer which could contain the sig and digest too. */
> > +	key = kmalloc(verif_bundle_len, GFP_KERNEL);
> >  	if (!key)
> >  		goto error_free_req;
> >  
> > @@ -424,9 +438,24 @@ int public_key_verify_signature(const struct public_key *pkey,
> >  			goto error_free_key;
> >  	}
> >  
> > +	if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > +		ptr += pkey->paramlen;
> > +
> > +		if (!virt_addr_valid(sig_s)) {
> > +			sig_s = ptr;
> > +			memcpy(sig_s, sig->s, sig->s_size);
> > +			ptr += sig->s_size;
> > +		}
> > +
> > +		if (!virt_addr_valid(digest)) {
> > +			digest = ptr;
> > +			memcpy(digest, sig->digest, sig->digest_size);
> > +		}
> > +	}
> > +
> >  	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);
> > +	sg_set_buf(&src_sg[0], sig_s, sig->s_size);
> > +	sg_set_buf(&src_sg[1], digest, sig->digest_size);
> >  	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
> >  				   sig->digest_size);
> >  	crypto_init_wait(&cwait);
> 
> We should try to avoid adding error-prone special cases.  How about just doing
> the copy of the signature and digest unconditionally?  That would be much
> simpler.  It would even mean that the scatterlist would only need one element.

Took some time to figure out why Redzone was overwritten.

There must be two separate scatterlists. If you set the first only with
the sum of the key length and digest length, mpi_read_raw_from_sgl()
called by rsa_enc() is going to write before the d pointer in MPI.

		for (x = 0; x < len; x++) {
			a <<= 8;
			a |= *buff++;
			if (((z + x + 1) % BYTES_PER_MPI_LIMB) == 0) {
				val->d[j--] = a;
				a = 0;
			}
		}

Roberto

> Also, the size of buffer needed is only
> 
> 	max(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> 	    sig->s_size + sig->digest_size)
> 
> ... since the signature and digest aren't needed until the key was already used.
> 
> - Eric