[PATCH 0/2] crypto: fix RSA results corrupted by non-coherent DMA

[PATCH 0/2] crypto: fix RSA results corrupted by non-coherent DMA

[Stephane Debusne]

On i.MX8M Mini with the CAAM (rsa-caam, caam_jr loaded), PKCS#1 v1.5
signature verification fails almost every time: cfg80211 rejects
regulatory.db ("loaded regulatory.db is malformed or signature is
missing/invalid"), and a valid self-signed X.509 certificate cannot be
added to a keyring (EINVAL). Seen with the NXP BSP lf-6.18.37 and stable
updates up to 6.18.57; the code involved is identical in mainline.

crypto_akcipher_sync_prep() and rsassa_pkcs1_verify() place the data
buffer right after the request context, in the same allocation. caampkc
writes req_ctx->edesc and req_ctx->akcipher_op_done after mapping the
destination, so the cache line shared by the end of the context and the
start of the buffer is dirty while the device writes the result. On
arm64, dma_unmap_sg(DMA_FROM_DEVICE) cleans and invalidates that partial
line, and the stale CPU copy overwrites the first bytes of the result.
The self-tests pass because testmgr uses separate buffers.

A test module comparing rsa-caam with rsa-generic (2048-bit key, 200
public-key operations per case, CAAM era 9, non-coherent DMA) gives:

  destination buffer                        wrong results
  separate kmalloc() buffers                0/200
  in place, own kmalloc() buffer            0/200
  separate source, dst after req. context   200/200
  in place, after req. context (rsassa)     200/200
  crypto_akcipher_sync_encrypt()            199/200 (patch 1: 0/200)
  pkcs1(rsa-caam,sha256) verification       398/400 (patch 2: 0/200)

In the wrong results reported, bytes 0 to 23 always hold the previous
content of the buffer: 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 rare correct results are consistent with that
line being evicted before the device writes.

Both patches give the data buffer its own allocation, as rsa-pkcs1pad
did before 6.13. The driver could instead avoid writing its request
context while the buffers are mapped, but any DMA-based akcipher on a
non-coherent platform could hit the same problem, so fixing the callers
seemed more robust. rsassa_pkcs1_verify() could also use the fixed sync
helper instead of its own allocation; I kept the change minimal, and
will gladly rework the series in either direction.

Tested on i.MX8M Mini with 6.18.57: with caam_jr loaded, regulatory.db
loads again, and the test module reports no wrong result through the
sync helper and the pkcs1 template. The test module is available if
useful.

Stephane Debusne (2):
  crypto: akcipher - use a separate buffer in the sync helpers
  crypto: rsassa-pkcs1 - use a separate buffer for the RSA output

 crypto/akcipher.c     | 21 ++++++++++++++-------
 crypto/rsassa-pkcs1.c | 16 +++++++++++++---
 2 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH 1/2] crypto: akcipher - use a separate buffer in the sync helpers

[Stephane Debusne]

crypto_akcipher_sync_prep() places the data buffer right after the
request context, in the same allocation, and passes it to the driver
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. A DMA-based driver such as the
CAAM (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.

crypto_akcipher_sync_encrypt() and crypto_akcipher_sync_decrypt() are
used by rsassa_pkcs1_sign() and by software_key_eds_op() (keyctl
pkey_encrypt and pkey_decrypt), so RSA signing and encryption through
rsa-caam are affected.

On i.MX8M Mini (CAAM era 9, rsa-caam, 6.18), the buffer starts at
offset 40 of a 64-byte line shared with the end of the request context,
and crypto_akcipher_sync_encrypt() returns a result whose first 24
bytes still hold the input in 199 of 200 calls.

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

Fixes: addde1f2c966 ("crypto: akcipher - Add sync interface without SG lists")
Cc: stable@vger.kernel.org
Signed-off-by: Stephane Debusne <stephane.debusne@cynove.com>
---
 crypto/akcipher.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index 630bb1973..2381ed43b 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -170,23 +170,29 @@ static int crypto_akcipher_sync_prep(struct crypto_akcipher_sync_data *data)
 	struct akcipher_request *req;
 	struct scatterlist *sg;
 	unsigned int mlen;
-	unsigned int len;
 	u8 *buf;
 
 	mlen = max(data->slen, data->dlen);
 
-	len = sizeof(*req) + reqsize + mlen;
-	if (len < mlen)
-		return -EOVERFLOW;
-
-	req = kzalloc(len, GFP_KERNEL);
+	req = kzalloc(sizeof(*req) + reqsize, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
+	/*
+	 * The data buffer is mapped for DMA by hardware 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 result on non-coherent DMA.
+	 */
+	buf = kzalloc(mlen, GFP_KERNEL);
+	if (!buf) {
+		kfree(req);
+		return -ENOMEM;
+	}
+
 	data->req = req;
 	akcipher_request_set_tfm(req, data->tfm);
 
-	buf = (u8 *)(req + 1) + reqsize;
 	data->buf = buf;
 	memcpy(buf, data->src, data->slen);
 
@@ -207,6 +213,7 @@ static int crypto_akcipher_sync_post(struct crypto_akcipher_sync_data *data,
 	err = crypto_wait_req(err, &data->cwait);
 	memcpy(data->dst, data->buf, data->dlen);
 	data->dlen = data->req->dst_len;
+	kfree_sensitive(data->buf);
 	kfree_sensitive(data->req);
 	return err;
 }
-- 
2.43.0

[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@vger.kernel.org
Signed-off-by: Stephane Debusne <stephane.debusne@cynove.com>
---
 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

Re: [PATCH 0/2] crypto: fix RSA results corrupted by non-coherent DMA

[Jeff Barnes]

Hi Stephane,

Thanks for posting this. While investigating CAAM RSA on another arm64
non-coherent system, I found a separate DMA issue affecting the same
in-place request shape.

Your patches move the synchronous-helper and RSASSA-PKCS1 buffers into
separate allocations, while intentionally retaining src == dst. That
addresses cache-line sharing between the payload and mutable request
context. The issue I observed is in rsa-caam's ownership of that
exact-alias SG.

On an NXP LX2160A with CAAM Era 10 behind an ARM SMMUv2 translated DMA
domain, rsa-caam mapped the same effective SG twice:

- first with DMA_TO_DEVICE, producing mapping A;
- then with DMA_FROM_DEVICE, producing a different mapping B.

Because dma_map_sg() writes DMA metadata into the SG, the second map
replaced the metadata describing A. Completion subsequently unmapped B
as the destination and then consumed the same overwritten metadata when
unmapping the source. The second teardown attempted to remove B again,
while the SG no longer retained the metadata needed to release A.

The public completion warning included:

    __arm_lpae_unmap
    arm_lpae_unmap_pages
    arm_smmu_unmap_pages
    __iommu_unmap
    iommu_unmap_fast
    __iommu_dma_unmap
    iommu_dma_unmap_sg
    dma_unmap_sg_attrs
    rsa_io_unmap
    rsa_pub_done
    caam_jr_dequeue

A corresponding private RSA case reached rsa_io_unmap() through
rsa_priv_f_done(). Certificate verification reproduced the public path
with a 512-byte operand.

I tested a narrow downstream prototype that, after source normalization,
maps an exact effective-head alias once with DMA_BIDIRECTIONAL over the
larger of the source and destination extents. The observed public 256/256
and certificate-verification 512/512 requests each had one original SG
entry and returned one DMA segment. They then used one BIDIRECTIONAL map
and one matching unmap with the original entry count.

With that prototype, the system booted successfully and the tested logs
contained no matches for __arm_lpae_unmap, __arm_lpae_map,
__iommu_dma_unmap, RSA algorithm failures, invalid output, RSA self-test
failures, or rsa-caam failures. These are downstream results for the
observed one-entry mappings, not validation of arbitrary multi-entry
exact aliases or a current upstream patch.

Changwei's pending unaligned-destination bounce-buffer patch appears
complementary:

https://lore.kernel.org/linux-crypto/20261004210200.290119-3-changwei.zou@canonical.com/

For an unaligned destination, bounce substitution would make the
effective source and destination distinct, allowing separate TO/FROM
ownership. For an aligned exact alias, the original SG would remain
shared and would still need one BIDIRECTIONAL mapping. If combined,
destination substitution appears to need to happen before effective-head
alias classification.

The unresolved case is a shared mapping that returns more than one DMA
segment. Using the union mapping's returned count for both logical
hardware views can change existing SEC4 source-table selection,
particularly for a shorter source. I do not yet have hardware evidence
for that case.

Would the preferred direction be:

1. an initial narrowly bounded CAAM RFC demonstrating the one-owner
   BIDIRECTIONAL correction and explicitly rejecting unresolved
   multi-segment aliases;
2. coordination with the pending destination-bounce work for problematic
   aliases; or
3. constructing separately bounded source and destination SEC4 views
   over one shared mapping?

I wanted to raise this here before posting a separate RFC because your
series confirms that exact-alias requests remain intentional, and the two
DMA issues are independent but affect the same callers.

Thanks,
Jeff