diff mbox series

[v2,08/19] crypto: rsassa-pkcs1 - Avoid copying hash prefix

Message ID 819ed9cd21975ad4d6683d46f4147659ca043f8b.1725972335.git.lukas@wunner.de (mailing list archive)
State New
Headers show
Series Migrate to sig_alg and templatize ecdsa | expand

Commit Message

Lukas Wunner Sept. 10, 2024, 2:30 p.m. UTC
When constructing the EMSA-PKCS1-v1_5 padding for the sign operation,
a buffer for the padding is allocated and the Full Hash Prefix is copied
into it.  The padding is then passed to the RSA decrypt operation as an
sglist entry which is succeeded by a second sglist entry for the hash.

Actually copying the hash prefix around is completely unnecessary.
It can simply be referenced from a third sglist entry which sits
in-between the padding and the digest.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 crypto/rsassa-pkcs1.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Jarkko Sakkinen Sept. 11, 2024, 1 p.m. UTC | #1
On Tue Sep 10, 2024 at 5:30 PM EEST, Lukas Wunner wrote:
> When constructing the EMSA-PKCS1-v1_5 padding for the sign operation,
> a buffer for the padding is allocated and the Full Hash Prefix is copied
> into it.  The padding is then passed to the RSA decrypt operation as an
> sglist entry which is succeeded by a second sglist entry for the hash.
>
> Actually copying the hash prefix around is completely unnecessary.
> It can simply be referenced from a third sglist entry which sits
> in-between the padding and the digest.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  crypto/rsassa-pkcs1.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
> index 8f42a5712806..b291ec0944a2 100644
> --- a/crypto/rsassa-pkcs1.c
> +++ b/crypto/rsassa-pkcs1.c
> @@ -153,7 +153,7 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
>  	struct rsassa_pkcs1_ctx *ctx = crypto_sig_ctx(tfm);
>  	unsigned int child_reqsize = crypto_akcipher_reqsize(ctx->child);
>  	struct akcipher_request *child_req __free(kfree_sensitive) = NULL;
> -	struct scatterlist in_sg[2], out_sg;
> +	struct scatterlist in_sg[3], out_sg;
>  	struct crypto_wait cwait;
>  	unsigned int pad_len;
>  	unsigned int ps_end;
> @@ -173,24 +173,26 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
>  	if (slen + hash_prefix->size > ctx->key_size - 11)
>  		return -EOVERFLOW;
>  
> -	child_req = kmalloc(sizeof(*child_req) + child_reqsize +
> -			    ctx->key_size - 1 - slen, GFP_KERNEL);
> +	pad_len = ctx->key_size - slen - hash_prefix->size - 1;
> +
> +	child_req = kmalloc(sizeof(*child_req) + child_reqsize + pad_len,
> +			    GFP_KERNEL);
>  	if (!child_req)
>  		return -ENOMEM;
>  
>  	/* RFC 8017 sec 8.2.1 step 1 - EMSA-PKCS1-v1_5 encoding generation */
>  	in_buf = (u8 *)(child_req + 1) + child_reqsize;
> -	ps_end = ctx->key_size - hash_prefix->size - slen - 2;
> +	ps_end = pad_len - 1;
>  	in_buf[0] = 0x01;
>  	memset(in_buf + 1, 0xff, ps_end - 1);
>  	in_buf[ps_end] = 0x00;
> -	memcpy(in_buf + ps_end + 1, hash_prefix->data, hash_prefix->size);
>  
>  	/* RFC 8017 sec 8.2.1 step 2 - RSA signature */
>  	crypto_init_wait(&cwait);
> -	sg_init_table(in_sg, 2);
> -	sg_set_buf(&in_sg[0], in_buf, ctx->key_size - 1 - slen);
> -	sg_set_buf(&in_sg[1], src, slen);
> +	sg_init_table(in_sg, 3);
> +	sg_set_buf(&in_sg[0], in_buf, pad_len);
> +	sg_set_buf(&in_sg[1], hash_prefix->data, hash_prefix->size);
> +	sg_set_buf(&in_sg[2], src, slen);
>  	sg_init_one(&out_sg, dst, dlen);
>  	akcipher_request_set_tfm(child_req, ctx->child);
>  	akcipher_request_set_crypt(child_req, in_sg, &out_sg,

LGTM. Not giving any tags because given the size of the patch set this
could change in content and/or order (in the series). Wondering if this
could be as a clean up like more in the head of the series?

BR, Jarkko
diff mbox series

Patch

diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
index 8f42a5712806..b291ec0944a2 100644
--- a/crypto/rsassa-pkcs1.c
+++ b/crypto/rsassa-pkcs1.c
@@ -153,7 +153,7 @@  static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
 	struct rsassa_pkcs1_ctx *ctx = crypto_sig_ctx(tfm);
 	unsigned int child_reqsize = crypto_akcipher_reqsize(ctx->child);
 	struct akcipher_request *child_req __free(kfree_sensitive) = NULL;
-	struct scatterlist in_sg[2], out_sg;
+	struct scatterlist in_sg[3], out_sg;
 	struct crypto_wait cwait;
 	unsigned int pad_len;
 	unsigned int ps_end;
@@ -173,24 +173,26 @@  static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
 	if (slen + hash_prefix->size > ctx->key_size - 11)
 		return -EOVERFLOW;
 
-	child_req = kmalloc(sizeof(*child_req) + child_reqsize +
-			    ctx->key_size - 1 - slen, GFP_KERNEL);
+	pad_len = ctx->key_size - slen - hash_prefix->size - 1;
+
+	child_req = kmalloc(sizeof(*child_req) + child_reqsize + pad_len,
+			    GFP_KERNEL);
 	if (!child_req)
 		return -ENOMEM;
 
 	/* RFC 8017 sec 8.2.1 step 1 - EMSA-PKCS1-v1_5 encoding generation */
 	in_buf = (u8 *)(child_req + 1) + child_reqsize;
-	ps_end = ctx->key_size - hash_prefix->size - slen - 2;
+	ps_end = pad_len - 1;
 	in_buf[0] = 0x01;
 	memset(in_buf + 1, 0xff, ps_end - 1);
 	in_buf[ps_end] = 0x00;
-	memcpy(in_buf + ps_end + 1, hash_prefix->data, hash_prefix->size);
 
 	/* RFC 8017 sec 8.2.1 step 2 - RSA signature */
 	crypto_init_wait(&cwait);
-	sg_init_table(in_sg, 2);
-	sg_set_buf(&in_sg[0], in_buf, ctx->key_size - 1 - slen);
-	sg_set_buf(&in_sg[1], src, slen);
+	sg_init_table(in_sg, 3);
+	sg_set_buf(&in_sg[0], in_buf, pad_len);
+	sg_set_buf(&in_sg[1], hash_prefix->data, hash_prefix->size);
+	sg_set_buf(&in_sg[2], src, slen);
 	sg_init_one(&out_sg, dst, dlen);
 	akcipher_request_set_tfm(child_req, ctx->child);
 	akcipher_request_set_crypt(child_req, in_sg, &out_sg,