diff mbox series

[crypto-2.6] crypto: rsassa-pkcs1 - Avoid pointing to rodata in scatterlists

Message ID 3de5d373c86dcaa5abc36f501c1398c4fbf05f2f.1732865109.git.lukas@wunner.de
State New
Headers show
Series [crypto-2.6] crypto: rsassa-pkcs1 - Avoid pointing to rodata in scatterlists | expand

Commit Message

Lukas Wunner Nov. 29, 2024, 7:46 a.m. UTC 
Zorro reports a BUG_ON() when running boot-time crypto selftests on
arm64:

Since commit 1e562deacecc ("crypto: rsassa-pkcs1 - Migrate to sig_alg
backend"), sg_set_buf() is called with the address of a test vector in
the .rodata section.  That fails on arm64 as virt_addr_valid() returns
false.  Architectures such as x86 are more lenient and transparently
translate the virtual address pointing into the kernel image to a
physical address.  However Mark points out that the BUG_ON() may still
occur if testmgr.c is built as a module because the test vectors would
then be module/vmalloc addresses rather than virt/linear or kernel image
addresses.

Avoid the issue by auto-detecting whether the src buffer of a sign or
verify operation is a valid virtual address and duplicating the buffer
if not.

An alternative approach would be to use crypto_akcipher_sync_encrypt()
and crypto_akcipher_sync_decrypt(), however that would *always*
duplicate the src buffer (rather than only when it's necessary)
and thus negatively impact performance.

In addition to the src buffer, the Full Hash Prefix likewise lives in
the .rodata section and is always included in a scatterlist when
performing a sign operation, so duplicate it on instance creation.

Fixes: 1e562deacecc ("crypto: rsassa-pkcs1 - Migrate to sig_alg backend")
Reported-by: Zorro Lang <zlang@redhat.com>
Closes: https://lore.kernel.org/r/20241122045106.tzhvm2wrqvttub6k@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 crypto/rsassa-pkcs1.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

Herbert Xu Nov. 29, 2024, 7:53 a.m. UTC | #1 
On Fri, Nov 29, 2024 at 08:46:58AM +0100, Lukas Wunner wrote:
>
> @@ -185,6 +187,16 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
>  	if (slen + hash_prefix->size > ctx->key_size - 11)
>  		return -EOVERFLOW;
>  
> +	/*
> +	 * Only kmalloc virtual addresses shall be used in a scatterlist,
> +	 * so duplicate src if it points e.g. into kernel or module rodata.
> +	 */
> +	if (!virt_addr_valid(src)) {

Please don't do this.  You cannot turn a virtual address into
an SG list in general.  This is just one of the many failure
scenarios.

The only safe way to do this is for the user to tell us that it's
OK.

So please switch over to the sync interface.

Thanks,
Lukas Wunner Dec. 6, 2024, 8:15 a.m. UTC | #2 
On Fri, Nov 29, 2024 at 03:53:29PM +0800, Herbert Xu wrote:
> On Fri, Nov 29, 2024 at 08:46:58AM +0100, Lukas Wunner wrote:
> > @@ -185,6 +187,16 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
> >  	if (slen + hash_prefix->size > ctx->key_size - 11)
> >  		return -EOVERFLOW;
> >  
> > +	/*
> > +	 * Only kmalloc virtual addresses shall be used in a scatterlist,
> > +	 * so duplicate src if it points e.g. into kernel or module rodata.
> > +	 */
> > +	if (!virt_addr_valid(src)) {
> 
> Please don't do this.  You cannot turn a virtual address into
> an SG list in general.  This is just one of the many failure
> scenarios.
> 
> The only safe way to do this is for the user to tell us that it's
> OK.

The dmaengine transporting data between memory and crypto accelerator
may have alignment or buswidth requirements not fulfilled by the
src buffer.

The caller cannot predict which crypto implementation (software or
accelerator) is going to be used and thus cannot know whether
location and length of the src buffer works for the dmaengine.

Hence I'm thinking that the sig or akcipher midlayer may need to
determine whether the src buffer is usable, and duplicate it if not.
The proposal above was meant as a step towards such an auto-detection
mechanism.

Thanks,

Lukas
Herbert Xu Dec. 6, 2024, 8:17 a.m. UTC | #3 
On Fri, Dec 06, 2024 at 09:15:23AM +0100, Lukas Wunner wrote:
>
> The caller cannot predict which crypto implementation (software or
> accelerator) is going to be used and thus cannot know whether
> location and length of the src buffer works for the dmaengine.

If the caller is supplying an SG list then it is expected to
have already taken care of DMA alignment issues.

This is how it works throughout the kernel.

For virtual addresses on the other hand of course there is *no*
guarantee.

Cheers,
diff mbox series

Patch

diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
index 4d077fc9..5839a85 100644
--- a/crypto/rsassa-pkcs1.c
+++ b/crypto/rsassa-pkcs1.c
@@ -153,6 +153,7 @@  struct rsassa_pkcs1_ctx {
 struct rsassa_pkcs1_inst_ctx {
 	struct crypto_akcipher_spawn spawn;
 	const struct hash_prefix *hash_prefix;
+	const u8 *hash_prefix_data;
 };
 
 static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
@@ -165,6 +166,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;
+	void *src_copy __free(kfree_sensitive) = NULL;
 	struct scatterlist in_sg[3], out_sg;
 	struct crypto_wait cwait;
 	unsigned int pad_len;
@@ -185,6 +187,16 @@  static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
 	if (slen + hash_prefix->size > ctx->key_size - 11)
 		return -EOVERFLOW;
 
+	/*
+	 * Only kmalloc virtual addresses shall be used in a scatterlist,
+	 * so duplicate src if it points e.g. into kernel or module rodata.
+	 */
+	if (!virt_addr_valid(src)) {
+		src = src_copy = kmemdup(src, slen, GFP_KERNEL);
+		if (!src)
+			return -ENOMEM;
+	}
+
 	pad_len = ctx->key_size - slen - hash_prefix->size - 1;
 
 	child_req = kmalloc(sizeof(*child_req) + child_reqsize + pad_len,
@@ -203,7 +215,7 @@  static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
 	crypto_init_wait(&cwait);
 	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[1], ictx->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);
@@ -239,6 +251,7 @@  static int rsassa_pkcs1_verify(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;
+	void *src_copy __free(kfree_sensitive) = NULL;
 	struct scatterlist in_sg, out_sg;
 	struct crypto_wait cwait;
 	unsigned int dst_len;
@@ -252,6 +265,16 @@  static int rsassa_pkcs1_verify(struct crypto_sig *tfm,
 	    rsassa_pkcs1_invalid_hash_len(dlen, hash_prefix))
 		return -EINVAL;
 
+	/*
+	 * Only kmalloc virtual addresses shall be used in a scatterlist,
+	 * so duplicate src if it points e.g. into kernel or module rodata.
+	 */
+	if (!virt_addr_valid(src)) {
+		src = src_copy = kmemdup(src, slen, GFP_KERNEL);
+		if (!src)
+			return -ENOMEM;
+	}
+
 	/* RFC 8017 sec 8.2.2 step 2 - RSA verification */
 	child_req = kmalloc(sizeof(*child_req) + child_reqsize + ctx->key_size,
 			    GFP_KERNEL);
@@ -366,6 +389,7 @@  static void rsassa_pkcs1_free(struct sig_instance *inst)
 	struct crypto_akcipher_spawn *spawn = &ctx->spawn;
 
 	crypto_drop_akcipher(spawn);
+	kfree(ctx->hash_prefix_data);
 	kfree(inst);
 }
 
@@ -412,6 +436,13 @@  static int rsassa_pkcs1_create(struct crypto_template *tmpl, struct rtattr **tb)
 		goto err_free_inst;
 	}
 
+	ctx->hash_prefix_data = kmemdup(ctx->hash_prefix->data,
+					ctx->hash_prefix->size, GFP_KERNEL);
+	if (!ctx->hash_prefix_data) {
+		err = -ENOMEM;
+		goto err_free_inst;
+	}
+
 	err = -ENAMETOOLONG;
 	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
 		     "pkcs1(%s,%s)", rsa_alg->base.cra_name,