[PATCH v4 1/2] lib/mpi: Fix buffer overrun when SG is too long

[PATCH v4 1/2] lib/mpi: Fix buffer overrun when SG is too long

[Roberto Sassu]

From: Herbert Xu <herbert@gondor.apana.org.au>

The helper mpi_read_raw_from_sgl sets the number of entries in
the SG list according to nbytes.  However, if the last entry
in the SG list contains more data than nbytes, then it may overrun
the buffer because it only allocates enough memory for nbytes.

Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 lib/mpi/mpicoder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 39c4c6731094..3cb6bd148fa9 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -504,7 +504,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
 
 	while (sg_miter_next(&miter)) {
 		buff = miter.addr;
-		len = miter.length;
+		len = min_t(unsigned, miter.length, nbytes);
+		nbytes -= len;
 
 		for (x = 0; x < len; x++) {
 			a <<= 8;
-- 
2.25.1

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

[Roberto Sassu]

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.

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_set_buf(). 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@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, 22 insertions(+), 17 deletions(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 2f8352e88860..a479e32cb280 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,38 @@ 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_table(&src_sg, 1);
+	sg_set_buf(&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 +440,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:
-- 
2.25.1

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

[Roberto Sassu]

On 12/27/2022 10:46 AM, Roberto Sassu wrote:
> 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.
> 
> 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_set_buf(). 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@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, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 2f8352e88860..a479e32cb280 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,38 @@ 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_table(&src_sg, 1);
> +	sg_set_buf(&src_sg, buf, sig->s_size + sig->digest_size);

Ops, didn't commit the changes. Will send a new version soon.

Roberto

> +	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 +440,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: