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

[Roberto Sassu]

On Fri, 2022-12-09 at 15:15 +0100, Roberto Sassu wrote:
> 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()

Of signature length and digest length.

Roberto

> 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