crypto: safexcel: fix AEAD DMA mapping and cleanup

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

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.

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

index a8349b684693..4982c64bf236 100644
@@ -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

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.

---
 .../crypto/inside-secure/safexcel_cipher.c    | 149 ++++++++++++++++--
 1 file changed, 133 insertions(+), 16 deletions(-)

index 4982c64bf236..491fcd97ebdb 100644
@@ -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

Hi Herbert,

No, the regions do not need to occupy separate sg entries, the patch selects the direction conservatively for the whole entry.

For a completely linear in-place request, the entry overlaps the output range and is therefore mapped DMA_BIDIRECTIONAL. For an out-of-place request, the source is mapped DMA_TO_DEVICE. For example, a single destination entry covering:

[ AAD | ciphertext | tag ]

has a logical output range covering only:

[ ciphertext | tag ]

Since the entry also contains the preserved AAD prefix, it is classified as mixed and mapped DMA_BIDIRECTIONAL. The same applies if an entry extends beyond the end of the output range.

Only entries wholly outside the output range use DMA_TO_DEVICE, while only entries wholly contained within an out-of-place output range use DMA_FROM_DEVICE.

Ralf Lici
Mandelbit Srl

Wait, I think this suggests that the caller is giving us bogus input.

If the input is out-of-place, it shouldn't contain overlapping data.

Which caller is this?

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sorry, that was a misunderstanding, used "overlap" poorly. I only meant that one sg entry can contain both the reserved AAD prefix and the bytes written by the device. I did not mean that the source and destination share memory.

There is no caller to identify for that particular single-entry layout, it was only a hypothetical example in response to your question. Even for out-of-place AEAD, the destination starts with space reserved for the associated data (as documented in the comment at the top of include/crypto/aead.h). A single linear destination entry can therefore contain both that reserved prefix, which the device does not write, and the ciphertext and tag, which it does write. Because the DMA direction applies to the whole entry, such a mixed entry is mapped DMA_BIDIRECTIONAL.

Ralf Lici
Mandelbit Srl

So why is it a problem if the dst SG list entries aren't pointing to memory that's also occupied by the SG list entries?

Even if the hardware doesn't write to the data, it should be OK to map them.

The only issue that I can see is if the same memory is present in both the src SG list and the dst SG list, but that is expressly forbidden for out-of-place operations, and indeed would be a grave security issue.

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I think the missing point is that DMA_FROM_DEVICE is not neutral for bytes which the device does not write.

For an out-of-place request, the caller may have already populated the reserved destination AAD area. The AEAD API says that this area will not be written by the cipher operation:

Even in the out-of-place case, space must be reserved in the destination for the associated data, even though it won't be written to.

With SWIOTLB, however, mapping it as DMA_FROM_DEVICE may leave the corresponding bounce-buffer bytes uninitialized, and unmapping then copies those untouched bytes back over the caller's AAD.

The API also explicitly permits the source and destination AAD entries to describe the exact same byte range (and users do this in practice, for example nitrox_rfc4106_set_aead_rctx_sglist):

It is permissible for the "destination" associated data to alias the "source" associated data.

Therefore, this is a valid out-of-place request:

src: [ AAD X ] [ plaintext P ]
dst: [ AAD X ] [ ciphertext C ] [ tag T ]

P and C use separate storage. Only the two AAD entries point to the same addresses X.

Safexcel currently maps the source list as DMA_TO_DEVICE and the whole destination list as DMA_FROM_DEVICE. Consequently, X is mapped as device output even though the accelerator deliberately does not write it. On a non-coherent system or with SWIOTLB, unmapping the destination can then overwrite X with stale data.

Thanks for following up.

Ralf Lici
Mandelbit Srl

That's what I was asking. Which caller is doing this and thus would be broken without your patch?

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

The caller which exposed this is the ovpn UDP-GSO transmit path [1], part of a series currently under review and not in mainline yet. It builds an out-of-place AEAD request and explicitly copies the authenticated header into the reserved destination prefix before submitting it:

  ovpn_aead_encrypt_header(..., aad);
  skb_store_bits(gso_skb, offset, aad, aad_size);

The source AAD then points to the temporary aad buffer, while the destination AAD points into gso_skb (they are separate storage). The accelerator skips the destination prefix as required by the AEAD API, but safexcel maps it as DMA_FROM_DEVICE, so SWIOTLB may copy untouched bounce-buffer contents over the header during unmap. This is the out-of-place case in which I reproduced the corruption.

Technically, the in-tree ESP out-of-place path also constructs its destination around an already initialized header, but my ESP tests did not reproduce the corruption, so ovpn is the concrete demonstrated case here.

[1] https://lore.kernel.org/openvpn-devel/f075b9782b14d64756c84e132c138482f08287b9.1790843153.git.ralf@mandelbit.com/

Ralf Lici
Mandelbit Srl

Hi Herbert,

I tried that simplification on the MT7986 before respinning, but unfortunately mapping the complete AEAD destination DMA_BIDIRECTIONAL does not fix the corruption.

The relevant ovpn layout is physically:

[ AAD ][ tag ][ payload ]

and is described to the AEAD implementation in logical order as:

[ AAD ][ payload ][ tag ]

The entries do not overlap in bytes, but a small AAD entry and an adjacent output entry can share a cache line. When mapped DMA_BIDIRECTIONAL on this non-coherent system, the AAD entry is bounced. After the accelerator has written the output to RAM, unmapping the AAD copies its unchanged bytes back through the CPU. This dirties the CPU's stale copy of the cache line corresponding to the memory where part of the accelerator output now resides. Cache maintenance, while unmapping the adjacent output entry, can then clean that line and overwrite the output with stale pre-crypto bytes.

This also affects the in-place path. Although an in-place request uses the same scatterlist as source and destination, its AAD entry remains input-only. Mapping the whole list DMA_BIDIRECTIONAL therefore gives that entry an unnecessary and harmful copyback.

So I don't think the source/in-place handling can be dropped on this platform. I can add the physical/logical layout and the simplified A/B result to the next cover letter to make the reason for the finer-grained mapping clearer.

Thanks!

Ralf Lici
Mandelbit Srl