[PATCH crypto 0/2] crypto: safexcel: fix AEAD DMA mapping and cleanup

[PATCH crypto 0/2] crypto: safexcel: fix AEAD DMA mapping and cleanup

[Ralf Lici]

Hi,

This series fixes a couple of bugs that affect the safexcel driver with
the first one targeting a double unmap path and the second one (more
important) mapping AEAD buffers with finer granularity.

While testing ovpn (the OpenVPN data channel offload kernel module) on
two GL.iNet GL-MT6000 routers (using the MT7986 EIP97 on a non-coherent
Cortex-A53 system), once the kernel selects the hardware AES-GCM
provider, traffic became unreliable in ways that were difficult to
explain from the ovpn side. This is not the first such report: OpenWrt
users had described tunnels that stopped passing traffic when kernel
ovpn or ipsec and safexcel were loaded together.

During the tests I noticed that the stock driver reported successful
crypto completion, yet the reproducer found damaged output. At data
offsets 0, 32 and 66 modulo 128, respectively, the first 40, 8 and 38
payload bytes were wrong. Those lengths match the part of the payload
sharing a 64-byte cache line with the small AAD mapping. Fragmented
out-of-place encryption also failed with only one request in flight.

The problem is that safexcel describes too much of an AEAD request as
device output. It maps every entry of an in-place request as
DMA_BIDIRECTIONAL and every entry of an out-of-place destination as
DMA_FROM_DEVICE, even though AAD is read-only and a decrypt tag is
input-only. On these routers, an unaligned, small entry can be bounced
by SWIOTLB. Unmapping that entry as device output copies unchanged
bounce contents back; cache maintenance on the neighboring payload
mapping can then write stale CPU bytes over ciphertext or plaintext
which the EIP97 had produced correctly. The fixed SWIOTLB pool is only 1
MiB, with 512 slots of 2 KiB each, so these tiny entries also have a
disproportionate bounce cost under network load.

Thus, the second patch in this series gives each AEAD entry the
direction its contents require: input-only entries use DMA_TO_DEVICE,
pure out-of-place output uses DMA_FROM_DEVICE, and mixed or in-place
output uses DMA_BIDIRECTIONAL.

I also checked whether other network users of the accelerator behaved
the same way. With the corrected driver, RFC 4106 AES-128-GCM ESP passed
transport-mode tests with one, two and four SA pairs, without
authentication or replay errors.

kTLS revealed a different limit. Sustained TLS 1.2 AES-128-GCM still
corrupts when multiple 16 KiB records are live concurrently, both with
stock safexcel and with the corrected prototype. Serializing records
made 256 MiB transfers pass. I think the remaining issue is of the same
class: kTLS can place neighboring records in regions of shared pages,
making concurrent cache-line-sharing DMA mappings the leading
explanation, although I have not captured an exact cache-line trace.
These patches do not claim to fix that separate kTLS failure.

Cheers!

Ralf Lici
Mandelbit Srl

---
Ralf Lici (2):
  crypto: safexcel - Avoid unmapping failed DMA mappings
  crypto: safexcel - Map AEAD buffers with accurate DMA directions

 .../crypto/inside-secure/safexcel_cipher.c    | 173 +++++++++++++++---
 1 file changed, 150 insertions(+), 23 deletions(-)


base-commit: 60892a384aa1e65d0e703e1c513417bdf0c80777
-- 
2.55.0

[PATCH crypto 1/2] crypto: safexcel - Avoid unmapping failed DMA mappings

[Ralf Lici]

dma_map_sg undoes any partial mapping before returning zero. If mapping
the destination of an out-of-place request fails, safexcel nevertheless
reaches its common cleanup path with a positive destination SG count and
calls dma_unmap_sg on the already-unmapped list.

Track which mappings completed successfully and restrict error cleanup
to those lists.

Fixes: 87e02063d077 ("crypto: safexcel - Add error handling for dma_map_sg() calls")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
 .../crypto/inside-secure/safexcel_cipher.c    | 36 ++++++++++++-------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index a8349b684693..4982c64bf236 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -695,6 +695,7 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 	struct safexcel_token *atoken;
 	int n_cdesc = 0, n_rdesc = 0;
 	int queued, i, ret = 0;
+	bool src_mapped = false, dst_mapped = false;
 	bool first = true;
 
 	sreq->nr_src = sg_nents_for_len(src, totlen_src);
@@ -746,9 +747,12 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 				max(totlen_src, totlen_dst));
 			return -EINVAL;
 		}
-		if (sreq->nr_src > 0 &&
-		    !dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL))
-			return -EIO;
+		if (sreq->nr_src > 0) {
+			src_mapped = dma_map_sg(priv->dev, src, sreq->nr_src,
+						DMA_BIDIRECTIONAL);
+			if (!src_mapped)
+				return -EIO;
+		}
 	} else {
 		if (unlikely(totlen_src && (sreq->nr_src <= 0))) {
 			dev_err(priv->dev, "Source buffer not large enough (need %d bytes)!",
@@ -756,9 +760,12 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 			return -EINVAL;
 		}
 
-		if (sreq->nr_src > 0 &&
-		    !dma_map_sg(priv->dev, src, sreq->nr_src, DMA_TO_DEVICE))
-			return -EIO;
+		if (sreq->nr_src > 0) {
+			src_mapped = dma_map_sg(priv->dev, src, sreq->nr_src,
+						DMA_TO_DEVICE);
+			if (!src_mapped)
+				return -EIO;
+		}
 
 		if (unlikely(totlen_dst && (sreq->nr_dst <= 0))) {
 			dev_err(priv->dev, "Dest buffer not large enough (need %d bytes)!",
@@ -767,10 +774,13 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 			goto unmap;
 		}
 
-		if (sreq->nr_dst > 0 &&
-		    !dma_map_sg(priv->dev, dst, sreq->nr_dst, DMA_FROM_DEVICE)) {
-			ret = -EIO;
-			goto unmap;
+		if (sreq->nr_dst > 0) {
+			dst_mapped = dma_map_sg(priv->dev, dst, sreq->nr_dst,
+						DMA_FROM_DEVICE);
+			if (!dst_mapped) {
+				ret = -EIO;
+				goto unmap;
+			}
 		}
 	}
 
@@ -903,14 +913,14 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
 unmap:
 	if (src == dst) {
-		if (sreq->nr_src > 0)
+		if (src_mapped)
 			dma_unmap_sg(priv->dev, src, sreq->nr_src,
 				     DMA_BIDIRECTIONAL);
 	} else {
-		if (sreq->nr_src > 0)
+		if (src_mapped)
 			dma_unmap_sg(priv->dev, src, sreq->nr_src,
 				     DMA_TO_DEVICE);
-		if (sreq->nr_dst > 0)
+		if (dst_mapped)
 			dma_unmap_sg(priv->dev, dst, sreq->nr_dst,
 				     DMA_FROM_DEVICE);
 	}
-- 
2.55.0

[PATCH crypto 2/2] crypto: safexcel - Map AEAD buffers with accurate DMA directions

[Ralf Lici]

Safexcel maps every entry of an in-place AEAD request with
DMA_BIDIRECTIONAL and every entry of an out-of-place destination with
DMA_FROM_DEVICE. These directions are too broad: associated data is never
written, and an authentication tag is input-only during decryption.

On a non-coherent system, unmapping a small input-only entry as device
output can copy stale data back over valid accelerator output in a
neighboring part of the same cache line. The request still completes
successfully but its plaintext or ciphertext is corrupted afterwards.

Map AEAD entries separately according to their overlap with the logical
output range. Use DMA_TO_DEVICE for input-only entries, DMA_FROM_DEVICE for
pure out-of-place output entries, and DMA_BIDIRECTIONAL where an entry
contains both kinds of data or is updated in place.

This was validated on an MT7986 EIP97 with in-place and fragmented
out-of-place AES-GCM requests, for encryption and decryption, at queue
depths up to 400.

Fixes: f6beaea30487 ("crypto: inside-secure - authenc(hmac(sha256), cbc(aes)) support")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
 .../crypto/inside-secure/safexcel_cipher.c    | 149 ++++++++++++++++--
 1 file changed, 133 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 4982c64bf236..491fcd97ebdb 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -73,6 +73,8 @@ struct safexcel_cipher_req {
 	unsigned int rdescs;
 	bool needs_inv;
 	int  nr_src, nr_dst;
+	unsigned int output_offset;
+	unsigned int output_len;
 };
 
 static int safexcel_skcipher_iv(struct safexcel_cipher_ctx *ctx, u8 *iv,
@@ -608,6 +610,71 @@ static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
 	return 0;
 }
 
+/* classify each scatterlist entry by its overlap with the logical output */
+static enum dma_data_direction safexcel_aead_dma_dir(unsigned int offset,
+						     unsigned int len,
+						     unsigned int output_offset,
+						     unsigned int output_len,
+						     bool inplace)
+{
+	unsigned int end = offset + len;
+	unsigned int output_end = output_offset + output_len;
+
+	if (!output_len || offset >= output_end || output_offset >= end)
+		return DMA_TO_DEVICE;
+
+	if (inplace || offset < output_offset || end > output_end)
+		return DMA_BIDIRECTIONAL;
+
+	return DMA_FROM_DEVICE;
+}
+
+static void safexcel_unmap_aead(struct device *dev, struct scatterlist *sgl,
+				int nents, unsigned int output_offset,
+				unsigned int output_len, bool inplace)
+{
+	struct scatterlist *sg;
+	unsigned int offset = 0;
+	int i;
+
+	for_each_sg(sgl, sg, nents, i) {
+		enum dma_data_direction dir;
+
+		dir = safexcel_aead_dma_dir(offset, sg->length, output_offset,
+					    output_len, inplace);
+		dma_unmap_page(dev, sg_dma_address(sg), sg_dma_len(sg), dir);
+		offset += sg->length;
+	}
+}
+
+static int safexcel_map_aead(struct device *dev, struct scatterlist *sgl,
+			     int nents, unsigned int output_offset,
+			     unsigned int output_len, bool inplace)
+{
+	struct scatterlist *sg;
+	unsigned int offset = 0;
+	int i;
+
+	for_each_sg(sgl, sg, nents, i) {
+		enum dma_data_direction dir;
+
+		dir = safexcel_aead_dma_dir(offset, sg->length, output_offset,
+					    output_len, inplace);
+		sg_dma_address(sg) = dma_map_page(dev, sg_page(sg), sg->offset,
+						  sg->length, dir);
+		if (dma_mapping_error(dev, sg_dma_address(sg)))
+			goto err_unmap;
+		sg_dma_len(sg) = sg->length;
+		offset += sg->length;
+	}
+
+	return nents;
+
+err_unmap:
+	safexcel_unmap_aead(dev, sgl, i, output_offset, output_len, inplace);
+	return 0;
+}
+
 static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int ring,
 				      struct crypto_async_request *async,
 				      struct scatterlist *src,
@@ -645,16 +712,30 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin
 	safexcel_complete(priv, ring);
 
 	if (src == dst) {
-		if (sreq->nr_src > 0)
-			dma_unmap_sg(priv->dev, src, sreq->nr_src,
-				     DMA_BIDIRECTIONAL);
+		if (sreq->nr_src > 0) {
+			if (ctx->aead)
+				safexcel_unmap_aead(priv->dev, src,
+						    sreq->nr_src,
+						    sreq->output_offset,
+						    sreq->output_len, true);
+			else
+				dma_unmap_sg(priv->dev, src, sreq->nr_src,
+					     DMA_BIDIRECTIONAL);
+		}
 	} else {
 		if (sreq->nr_src > 0)
 			dma_unmap_sg(priv->dev, src, sreq->nr_src,
 				     DMA_TO_DEVICE);
-		if (sreq->nr_dst > 0)
-			dma_unmap_sg(priv->dev, dst, sreq->nr_dst,
-				     DMA_FROM_DEVICE);
+		if (sreq->nr_dst > 0) {
+			if (ctx->aead)
+				safexcel_unmap_aead(priv->dev, dst,
+						    sreq->nr_dst,
+						    sreq->output_offset,
+						    sreq->output_len, false);
+			else
+				dma_unmap_sg(priv->dev, dst, sreq->nr_dst,
+					     DMA_FROM_DEVICE);
+		}
 	}
 
 	/*
@@ -737,6 +818,10 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 	 */
 	totlen = totlen_src;
 	queued = totlen_src;
+	if (ctx->aead) {
+		sreq->output_offset = assoclen;
+		sreq->output_len = totlen_dst - assoclen;
+	}
 
 	if (src == dst) {
 		sreq->nr_src = max(sreq->nr_src, sreq->nr_dst);
@@ -748,8 +833,17 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 			return -EINVAL;
 		}
 		if (sreq->nr_src > 0) {
-			src_mapped = dma_map_sg(priv->dev, src, sreq->nr_src,
-						DMA_BIDIRECTIONAL);
+			if (ctx->aead)
+				src_mapped =
+					safexcel_map_aead(priv->dev, src,
+							  sreq->nr_src,
+							  sreq->output_offset,
+							  sreq->output_len,
+							  true);
+			else
+				src_mapped = dma_map_sg(priv->dev, src,
+							sreq->nr_src,
+							DMA_BIDIRECTIONAL);
 			if (!src_mapped)
 				return -EIO;
 		}
@@ -775,8 +869,17 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 		}
 
 		if (sreq->nr_dst > 0) {
-			dst_mapped = dma_map_sg(priv->dev, dst, sreq->nr_dst,
-						DMA_FROM_DEVICE);
+			if (ctx->aead)
+				dst_mapped =
+					safexcel_map_aead(priv->dev, dst,
+							  sreq->nr_dst,
+							  sreq->output_offset,
+							  sreq->output_len,
+							  false);
+			else
+				dst_mapped = dma_map_sg(priv->dev, dst,
+							sreq->nr_dst,
+							DMA_FROM_DEVICE);
 			if (!dst_mapped) {
 				ret = -EIO;
 				goto unmap;
@@ -913,16 +1016,30 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
 unmap:
 	if (src == dst) {
-		if (src_mapped)
-			dma_unmap_sg(priv->dev, src, sreq->nr_src,
-				     DMA_BIDIRECTIONAL);
+		if (src_mapped) {
+			if (ctx->aead)
+				safexcel_unmap_aead(priv->dev, src,
+						    sreq->nr_src,
+						    sreq->output_offset,
+						    sreq->output_len, true);
+			else
+				dma_unmap_sg(priv->dev, src, sreq->nr_src,
+					     DMA_BIDIRECTIONAL);
+		}
 	} else {
 		if (src_mapped)
 			dma_unmap_sg(priv->dev, src, sreq->nr_src,
 				     DMA_TO_DEVICE);
-		if (dst_mapped)
-			dma_unmap_sg(priv->dev, dst, sreq->nr_dst,
-				     DMA_FROM_DEVICE);
+		if (dst_mapped) {
+			if (ctx->aead)
+				safexcel_unmap_aead(priv->dev, dst,
+						    sreq->nr_dst,
+						    sreq->output_offset,
+						    sreq->output_len, false);
+			else
+				dma_unmap_sg(priv->dev, dst, sreq->nr_dst,
+					     DMA_FROM_DEVICE);
+		}
 	}
 
 	return ret;
-- 
2.55.0