[RFC PATCH 0/1] crypto: caam - fix in-place RSA payload DMA mapping

[RFC PATCH 0/1] crypto: caam - fix in-place RSA payload DMA mapping

[Jeff Barnes]

Hi,

This RFC fixes a DMA ownership problem in the CAAM RSA driver when the
effective source and destination are the same scatterlist.

rsa_edesc_alloc() currently maps the effective source with DMA_TO_DEVICE
and the destination with DMA_FROM_DEVICE. For an in-place request, this
maps the same scatterlist twice concurrently with different DMA directions.

dma_map_sg() is allowed to modify the scatterlist DMA metadata. On the
arm64/IOMMU system where I reproduced this, the two mappings returned
different DMA addresses. The second mapping replaced the metadata needed
to tear down the first one.

Completion subsequently unmapped the destination and then attempted to
unmap the source using the overwritten scatterlist metadata. This resulted
in ARM-LPAE/IOMMU unmap warnings from the CAAM RSA completion paths.

The user-visible crypto self-test failure included:

  alg: sig: sign test failed: invalid output
  alg: sig: test 1 failed for pkcs1(rsa-caam,sha256): err -22

The warnings were observed through the CAAM RSA public and private
completion paths, rsa_pub_done() and rsa_priv_f_done().

The patch gives an exact effective-head alias a single DMA owner.
CAAM reads and writes the shared payload, so the scatterlist is mapped
once with DMA_BIDIRECTIONAL and unmapped once with the original SG count.
Requests with distinct effective source and destination heads retain the
existing DMA_TO_DEVICE / DMA_FROM_DEVICE mappings.

There is one intentional limitation in this RFC.

For the existing direct hardware representation, a shared mapping
returning one DMA segment is straightforward. A shared mapping returning
multiple DMA segments is not. The logical source and destination may
cover different extents of the shared scatterlist, while the existing
SEC4/PDB bookkeeping uses the mapped source/destination counts to select
and locate those hardware views.

Source analysis found request shapes for which assigning the shared
returned count to both views causes the destination SEC4 table position
to differ from the offset reconstructed by PDB setup. Short-source
public RSA also raises a separate source-view selection issue.

Rather than send a representation known to be incorrect to CAAM, this
RFC unmaps a shared mapping that returns more than one DMA segment and
returns -EOPNOTSUPP before SEC4/PDB construction or job submission.

I would particularly appreciate feedback on two questions:

1. Is a single DMA_BIDIRECTIONAL mapping the preferred ownership model
   for an exact effective source/destination alias?

2. For a shared mapping returning multiple DMA segments, should CAAM
   grow separately bounded source and destination SEC4 views, or should
   such requests instead use a distinct destination, for example through
   a bounce-buffer path?

I tested the patch on an LX2160A arm64 system with CAAM Era 10 behind an
ARM SMMU.

The rsa-caam and pkcs1(rsa-caam,sha256) self-tests passed, and the
certificate-verification RSA selftest completed successfully, with no
recurrence of the original ARM-LPAE/IOMMU unmap warnings.

I also exercised the RFC containment path with a targeted full-width
2048-bit RSA request. The same two-entry scatterlist was supplied as
source and destination, using two separately allocated 128-byte entries.
This reached the multi-segment shared-mapping containment path.

The operation returned -EOPNOTSUPP synchronously, no completion callback
ran, and no CAAM descriptor/job-ring, DMA/IOMMU, WARN, Oops, panic, or
invalid-output error occurred during the test interval. The test module
unloaded cleanly.

There are related RSA DMA fixes under discussion. CAAM work for unaligned
RSA destinations uses a bounce-buffer approach, which may provide one
possible direction for the general multi-segment alias case. Separately,
work on RSA buffers on non-coherent DMA systems identified a cacheline
sharing problem and also demonstrates legitimate in-place RSA requests.

Neither is required by this RFC. This patch is based on current Linus
master and addresses the independent problem of acquiring two streaming
DMA mappings over the same live scatterlist.

Thanks,
Jeff

Jeff Barnes (1):
  crypto: caam - fix in-place RSA payload DMA mapping

 drivers/crypto/caam/caampkc.c | 79 ++++++++++++++++++++++++++++++------
 drivers/crypto/caam/caampkc.h |  3 ++
 2 files changed, 69 insertions(+), 13 deletions(-)

-- 
2.43.0

[RFC PATCH 1/1] crypto: caam - fix in-place RSA payload DMA mapping

[Jeff Barnes]

CAAM can receive RSA requests whose effective source and destination
scatterlists are identical. The driver currently maps the source with
DMA_TO_DEVICE and then maps the destination with DMA_FROM_DEVICE.

When both refer to the same scatterlist, the second dma_map_sg() can
overwrite the DMA metadata stored in the scatterlist by the first mapping.
On an IOMMU-backed non-coherent arm64 system, the two mappings can return
different DMA addresses. Completion then unmaps the destination mapping and
subsequently uses the overwritten scatterlist metadata while attempting to
unmap the source.

As a result, the destination mapping can be unmapped a second time while
the original source mapping is no longer represented by the scatterlist DMA
metadata.

This was observed during CAAM RSA crypto self-tests. One visible failure
was:

  alg: sig: sign test failed: invalid output
  alg: sig: test 1 failed for pkcs1(rsa-caam,sha256): err -22

The failures were accompanied by ARM-LPAE/IOMMU unmap warnings through
dma_unmap_sg_attrs(), originating from the CAAM RSA completion paths. The
warnings were observed through both rsa_pub_done() and rsa_priv_f_done().

Fix the DMA ownership problem by mapping an exact effective-head alias only
once using DMA_BIDIRECTIONAL, since CAAM both reads from and writes to the
shared payload. Retain the original scatterlist entry count passed to
dma_map_sg() and use that same count for the matching dma_unmap_sg().

Keep the existing DMA_TO_DEVICE and DMA_FROM_DEVICE ownership for requests
whose effective source and destination heads differ.

The source and destination can cover different logical byte extents of the
same scatterlist. For a shared mapping, therefore map the number of
original SG entries needed to cover the larger of the effective source
length and destination length.

A shared mapping for which dma_map_sg() returns one DMA segment can
continue using the existing direct hardware views. If the shared mapping
returns more than one DMA segment, however, the source and destination
require separately bounded SEC4 views.

The current SEC4/PDB bookkeeping cannot correctly represent every such
multi-segment request. In particular, for a short source the reserved
source view and the actual destination-table position can differ from the
offset reconstructed later by the PDB setup. Using the shared returned
segment count for source-table selection can also select a padding-
prefixed source view whose representation does not match the original
logical source length.

For this RFC, release a shared mapping that returns multiple DMA segments
and return -EOPNOTSUPP before allocating the extended descriptor,
constructing the SEC4/PDB representation, or submitting a job to CAAM. This
prevents a known-invalid representation from reaching hardware while
leaving the general multi-segment representation for further discussion.

The ownership fix was validated on an LX2160A arm64 system with CAAM Era 10
and an ARM SMMU. The existing single-segment exact-alias paths passed the
rsa-caam and pkcs1(rsa-caam,sha256) self-tests, including certificate
verification, with no recurrence of the ARM-LPAE/IOMMU unmap warnings.

The RFC containment path was also exercised directly on the same hardware.
A full-width 2048-bit RSA request used the same two-entry scatterlist as
both source and destination, with two separately allocated 128-byte
entries. The shared DMA mapping returned multiple DMA segments and reached
the containment path.

The RSA operation returned -EOPNOTSUPP synchronously before CAAM
submission, with no completion callback. The test unloaded cleanly, and no
ARM-LPAE, IOMMU/DMA API, CAAM descriptor, invalid-output, WARN, Oops, or
panic message was observed during the test interval.

The runtime test establishes that the multi-segment containment path was
reached and returned synchronously without submitting the RSA operation.
The single matching dma_unmap_sg() of that mapping follows from the
reviewed control flow; the test did not independently instrument or count
DMA unmap calls.

Signed-off-by: Jeff Barnes <jeffbarnes@linux.microsoft.com>
---
 drivers/crypto/caam/caampkc.c | 79 +++++++++++++++++++++++++++++------
 drivers/crypto/caam/caampkc.h |  3 ++
 2 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index cb001aa1de66..fed20a401712 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -51,8 +51,14 @@ static void rsa_io_unmap(struct device *dev, struct rsa_edesc *edesc,
 {
 	struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
 
-	dma_unmap_sg(dev, req->dst, edesc->dst_nents, DMA_FROM_DEVICE);
-	dma_unmap_sg(dev, req_ctx->fixup_src, edesc->src_nents, DMA_TO_DEVICE);
+	if (edesc->shared_nents) {
+		dma_unmap_sg(dev, req_ctx->fixup_src, edesc->shared_nents,
+			     DMA_BIDIRECTIONAL);
+	} else {
+		dma_unmap_sg(dev, req->dst, edesc->dst_nents, DMA_FROM_DEVICE);
+		dma_unmap_sg(dev, req_ctx->fixup_src, edesc->src_nents,
+			     DMA_TO_DEVICE);
+	}
 
 	if (edesc->sec4_sg_bytes)
 		dma_unmap_single(dev, edesc->sec4_sg_dma, edesc->sec4_sg_bytes,
@@ -260,6 +266,7 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	int sg_flags = (flags == GFP_ATOMIC) ? SG_MITER_ATOMIC : 0;
 	int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
 	int src_nents, dst_nents;
+	int shared_nents = 0;
 	int mapped_src_nents, mapped_dst_nents;
 	unsigned int diff_size = 0;
 	int lzeros;
@@ -289,19 +296,59 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 
 	src_nents = sg_nents_for_len(req_ctx->fixup_src,
 				     req_ctx->fixup_src_len);
-	dst_nents = sg_nents_for_len(req->dst, req->dst_len);
+	if (unlikely(src_nents < 0)) {
+		dev_err(dev, "Insufficient bytes (%u) in src S/G\n",
+			req_ctx->fixup_src_len);
+		return ERR_PTR(src_nents);
+	}
 
-	mapped_src_nents = dma_map_sg(dev, req_ctx->fixup_src, src_nents,
-				      DMA_TO_DEVICE);
-	if (unlikely(!mapped_src_nents)) {
-		dev_err(dev, "unable to map source\n");
-		return ERR_PTR(-ENOMEM);
+	dst_nents = sg_nents_for_len(req->dst, req->dst_len);
+	if (unlikely(dst_nents < 0)) {
+		dev_err(dev, "Insufficient bytes (%u) in dst S/G\n",
+			req->dst_len);
+		return ERR_PTR(dst_nents);
 	}
-	mapped_dst_nents = dma_map_sg(dev, req->dst, dst_nents,
-				      DMA_FROM_DEVICE);
-	if (unlikely(!mapped_dst_nents)) {
-		dev_err(dev, "unable to map destination\n");
-		goto src_fail;
+
+	if (req_ctx->fixup_src == req->dst) {
+		shared_nents = sg_nents_for_len(req_ctx->fixup_src,
+						max(req_ctx->fixup_src_len,
+						    req->dst_len));
+		if (unlikely(shared_nents < 0)) {
+			dev_err(dev, "Invalid shared S/G extent\n");
+			return ERR_PTR(shared_nents);
+		}
+
+		mapped_src_nents = dma_map_sg(dev, req_ctx->fixup_src,
+					      shared_nents, DMA_BIDIRECTIONAL);
+		if (unlikely(!mapped_src_nents)) {
+			dev_err(dev, "unable to map shared S/G\n");
+			return ERR_PTR(-ENOMEM);
+		}
+		if (mapped_src_nents > 1) {
+			/*
+			 * Multi-segment aliases need separately bounded source
+			 * and destination hardware views. The current SEC4/PDB
+			 * layout cannot represent all such requests; defer support.
+			 */
+			dma_unmap_sg(dev, req_ctx->fixup_src, shared_nents,
+				     DMA_BIDIRECTIONAL);
+			dev_err(dev, "Multi-segment shared S/G is not supported\n");
+			return ERR_PTR(-EOPNOTSUPP);
+		}
+		mapped_dst_nents = mapped_src_nents;
+	} else {
+		mapped_src_nents = dma_map_sg(dev, req_ctx->fixup_src, src_nents,
+					      DMA_TO_DEVICE);
+		if (unlikely(!mapped_src_nents)) {
+			dev_err(dev, "unable to map source\n");
+			return ERR_PTR(-ENOMEM);
+		}
+		mapped_dst_nents = dma_map_sg(dev, req->dst, dst_nents,
+					      DMA_FROM_DEVICE);
+		if (unlikely(!mapped_dst_nents)) {
+			dev_err(dev, "unable to map destination\n");
+			goto src_fail;
+		}
 	}
 
 	if (!diff_size && mapped_src_nents == 1)
@@ -338,6 +385,7 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	/* Save nents for later use in Job Descriptor */
 	edesc->src_nents = src_nents;
 	edesc->dst_nents = dst_nents;
+	edesc->shared_nents = shared_nents;
 
 	req_ctx->edesc = edesc;
 
@@ -365,6 +413,11 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 sec4_sg_fail:
 	kfree(edesc);
 dst_fail:
+	if (shared_nents) {
+		dma_unmap_sg(dev, req_ctx->fixup_src, shared_nents,
+			     DMA_BIDIRECTIONAL);
+		return ERR_PTR(-ENOMEM);
+	}
 	dma_unmap_sg(dev, req->dst, dst_nents, DMA_FROM_DEVICE);
 src_fail:
 	dma_unmap_sg(dev, req_ctx->fixup_src, src_nents, DMA_TO_DEVICE);
diff --git a/drivers/crypto/caam/caampkc.h b/drivers/crypto/caam/caampkc.h
index 96d03704c9be..09b6aa5cee98 100644
--- a/drivers/crypto/caam/caampkc.h
+++ b/drivers/crypto/caam/caampkc.h
@@ -119,6 +119,8 @@ struct caam_rsa_req_ctx {
  * rsa_edesc - s/w-extended rsa descriptor
  * @src_nents     : number of segments in input s/w scatterlist
  * @dst_nents     : number of segments in output s/w scatterlist
+ * @shared_nents  : original count of a successful shared BIDIRECTIONAL mapping;
+ *                 zero for independent source and destination mappings
  * @mapped_src_nents: number of segments in input h/w link table
  * @mapped_dst_nents: number of segments in output h/w link table
  * @sec4_sg_bytes : length of h/w link table
@@ -131,6 +133,7 @@ struct caam_rsa_req_ctx {
 struct rsa_edesc {
 	int src_nents;
 	int dst_nents;
+	int shared_nents;
 	int mapped_src_nents;
 	int mapped_dst_nents;
 	int sec4_sg_bytes;
-- 
2.43.0