diff mbox series

KEYS: asymmetric: Make a copy of sig and digest in vmalloced stack

Message ID 20221208164610.867747-1-roberto.sassu@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series KEYS: asymmetric: Make a copy of sig and digest in vmalloced stack | expand

Commit Message

Roberto Sassu Dec. 8, 2022, 4:46 p.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

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.

Check if the signature and digest passed to public_key_verify_signature()
are in the linear mapping area and, for those which are not, make a copy in
the linear mapping area with kmalloc() and adjust the pointer passed to
sg_set_buf(). Reuse the existing kmalloc() and increase the allocation size
as needed.

Minimize the number of copies with the compile-time check of
CONFIG_VMAP_STACK and with the run-time check virt_addr_valid().

Cc: stable@vger.kernel.org # 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@kernel.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 crypto/asymmetric_keys/public_key.c | 39 +++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 5 deletions(-)

Comments

Eric Biggers Dec. 8, 2022, 11:17 p.m. UTC | #1 
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.

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
Roberto Sassu Dec. 9, 2022, 2:15 p.m. UTC | #2 
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
Roberto Sassu Dec. 9, 2022, 2:27 p.m. UTC | #3 
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
diff mbox series

Patch

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);