[PATCH 2/2] crypto: rsassa-pkcs1 - use a separate buffer for the RSA output

[Stephane Debusne]

rsassa_pkcs1_verify() places the buffer it hands to the akcipher driver
right after the driver's request context, in the same allocation, and
since commit 8552cb04e083 ("crypto: rsassa-pkcs1 - Copy source data for
SG list") uses it in place (src == dst).

On non-coherent DMA, the start of this buffer may share a cache line
with the end of the request context. The CAAM driver (caampkc) writes
req_ctx->edesc and req_ctx->akcipher_op_done after dma_map_sg() of the
destination, which dirties that line. On arm64,
dma_unmap_sg(DMA_FROM_DEVICE) cleans and invalidates partial lines
(dc civac), so the stale CPU copy is written back over the result
written by the device.

On i.MX8M Mini (CAAM era 9, rsa-caam, 6.18), the buffer starts 168 bytes
into the allocation, at offset 40 of the 64-byte line that also holds
edesc and akcipher_op_done: the first 24 bytes of the RSA output are
replaced by the signature bytes, and rsassa_pkcs1_verify() returns
-EINVAL in 398 of 400 calls over two runs (the rare successes are
consistent with the dirty line being evicted before the device writes).
Visible effects: cfg80211 rejects regulatory.db ("loaded regulatory.db
is malformed or signature is missing/invalid"), and adding a valid
self-signed X.509 certificate to a keyring fails with EINVAL. The
akcipher self-tests pass, as they use separate buffers.

A test module comparing rsa-caam with rsa-generic shows that only the
position of the destination matters: separate or in-place buffers from
their own kmalloc() give correct results, while a destination right
after the request context is corrupted, in place or not (200 of 200
calls, the 24 first bytes always holding the previous content of the
buffer).

Allocate the output buffer separately, so that it is DMA-safe like any
kmalloc() buffer.

Fixes: 1e562deacecc ("crypto: rsassa-pkcs1 - Migrate to sig_alg backend")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Stephane Debusne <stephane.debusne@xxxxxxxxxx>
---
 crypto/rsassa-pkcs1.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
index 94fa5e960..205493037 100644
--- a/crypto/rsassa-pkcs1.c
+++ b/crypto/rsassa-pkcs1.c
@@ -223,6 +223,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;
+	u8 *out_alloc __free(kfree_sensitive) = NULL;
 	struct crypto_wait cwait;
 	struct scatterlist sg;
 	unsigned int dst_len;
@@ -237,12 +238,21 @@ static int rsassa_pkcs1_verify(struct crypto_sig *tfm,
 		return -EINVAL;
 
 	/* RFC 8017 sec 8.2.2 step 2 - RSA verification */
-	child_req = kmalloc(sizeof(*child_req) + child_reqsize + ctx->key_size,
-			    GFP_KERNEL);
+	child_req = kmalloc(sizeof(*child_req) + child_reqsize, GFP_KERNEL);
 	if (!child_req)
 		return -ENOMEM;
 
-	out_buf = (u8 *)(child_req + 1) + child_reqsize;
+	/*
+	 * The output buffer is mapped for DMA by hardware RSA drivers. Keep
+	 * it in its own allocation: placed right after the request context,
+	 * it may share a cache line with fields the driver writes while the
+	 * buffer is mapped, which corrupts the output on non-coherent DMA.
+	 */
+	out_alloc = kmalloc(ctx->key_size, GFP_KERNEL);
+	if (!out_alloc)
+		return -ENOMEM;
+
+	out_buf = out_alloc;
 	memcpy(out_buf, src, slen);
 
 	crypto_init_wait(&cwait);
-- 
2.43.0