crypto: qce - fix CCM AAD buffer leak and the AEAD error paths

This series fixes a memory leak in the QCE AEAD driver together with two bugs in the error handling around it.

qce_aead_ccm_prepare_buf_assoclen() allocates rctx->adata for every CCM request that carries associated data, but the buffer is never freed.

While preparing that fix, two problems in the error path of qce_aead_async_req_handle() turned out to matter:

- It unmaps req->src although rctx->src_sg is the scatterlist that was

    passed to dma_map_sg().  For CCM requests with associated data,
    rctx->src_sg carries the AAD buffer, so its mapping is leaked, and
    freeing rctx->adata there would free memory that is still DMA-mapped.

- A dma_map_sg() failure for the source scatterlist is not detected, because the result is checked for a negative value while dma_map_sg() returns 0 on failure. The error path then unmaps a scatterlist that was never mapped.

Patch 1 unmaps the correct scatterlist, patch 2 adds the missing frees and patch 3 fixes the dma_map_sg() check.

All three were reported as pre-existing in the Sashiko review of the QCE BAM locking series. They touch the same function that was recently fixed by commit 7f2345f47dd1 ("crypto: qce - fix CCM AAD buffer underallocation").

---
ZhaoJinming (3):
crypto: qce - unmap the correct scatterlist in the CCM error path
crypto: qce - fix memory leak of the CCM AAD buffer
crypto: qce - check dma_map_sg() result for the source scatterlist

 drivers/crypto/qce/aead.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

---
base-commit: 0fcb05b72344f40806cbe810de8872f1698bf053
change-id: 20261015-fix-qce-ccm-adata-leak-1ef34f172ba1

Best regards,

ZhaoJinming <[email protected]>

qce_aead_async_req_handle() maps the source with dma_map_sg(qce->dev, rctx->src_sg, rctx->src_nents, dir_src), but its error path unmaps req->src instead:

	error_unmap_src:
		if (diff_dst)
			dma_unmap_sg(qce->dev, req->src, rctx->src_nents, dir_src);

For CCM requests with associated data and separate source and destination buffers, rctx->src_sg is a scatterlist freshly built by qce_aead_ccm_prepare_buf_assoclen() that carries the AAD, message and result buffers, so it is not req->src. The mapping created for rctx->src_sg is therefore leaked, and req->src is unmapped with a nents count that does not belong to it.

Unmap rctx->src_sg, matching the dma_map_sg() call, as qce_aead_done() already does on the completion path. This also makes it safe to free rctx->adata on this path, which the next patch does.

---
 drivers/crypto/qce/aead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a9642c67f476..f8e6054ff15d 100644
@@ -478,7 +478,7 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
 	qce_dma_terminate_all(&qce->dma);
 error_unmap_src:
 	if (diff_dst)
-		dma_unmap_sg(qce->dev, req->src, rctx->src_nents, dir_src);
+		dma_unmap_sg(qce->dev, rctx->src_sg, rctx->src_nents, dir_src);
 error_unmap_dst:
 	dma_unmap_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);
 error_free:

2.50.1

qce_aead_ccm_prepare_buf_assoclen() allocates rctx->adata with kzalloc() for every CCM request that carries associated data, but the buffer is never freed: qce_aead_done() releases only the scatterlist tables, and none of the error paths release it either. Every such request therefore leaks a small allocation.

Free rctx->adata in the completion handler and on all error paths: the sg_copy_to_buffer() and sg_alloc_table() failures in qce_aead_ccm_prepare_buf_assoclen() (through the new err_free_adata label) and the error cleanup of qce_aead_async_req_handle().

The buffer is referenced by a scatterlist that is DMA-mapped, so it is freed only after dma_unmap_sg() has run. The frees are guarded the same way as the surrounding sg_free_table() calls, so no path frees the buffer twice or frees a stale pointer.

---
 drivers/crypto/qce/aead.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

index f8e6054ff15d..5bf59cb9ae9a 100644
@@ -53,6 +53,7 @@ static void qce_aead_done(void *data)
 			sg_free_table(&rctx->src_tbl);
 			if (diff_dst)
 				sg_free_table(&rctx->dst_tbl);
+			kfree(rctx->adata);
 		} else {
 			if (!(IS_DECRYPT(rctx->flags) && !diff_dst))
 				sg_free_table(&rctx->dst_tbl);

@@ -220,8 +221,10 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
 	/* Copy the associated data */
 	if (sg_copy_to_buffer(req->src, sg_nents_for_len(req->src, assoclen),
 			      rctx->adata + adata_header_len,
-			      assoclen) != assoclen)
-		return -EINVAL;
+			      assoclen) != assoclen) {
+		ret = -EINVAL;
+		goto err_free_adata;
+	}
 
 	/* Pad associated data to block size */
 	rctx->assoclen = ALIGN(assoclen + adata_header_len, 16);

@@ -236,7 +239,7 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
 	gfp = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ? GFP_KERNEL : GFP_ATOMIC;
 	ret = sg_alloc_table(&rctx->src_tbl, rctx->src_nents, gfp);
 	if (ret)
-		return ret;
+		goto err_free_adata;
 
 	/* Associated Data */
 	sg_init_one(&rctx->adata_sg, rctx->adata, rctx->assoclen);

@@ -288,6 +291,9 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
 	return 0;
 err_free:
 	sg_free_table(&rctx->src_tbl);
+err_free_adata:
+	kfree(rctx->adata);
+	rctx->adata = NULL;
 	return ret;
 }
 

@@ -486,6 +492,7 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
 		sg_free_table(&rctx->src_tbl);
 		if (diff_dst)
 			sg_free_table(&rctx->dst_tbl);
+		kfree(rctx->adata);
 	} else {
 		sg_free_table(&rctx->dst_tbl);
 	}

2.50.1

qce_aead_async_req_handle() maps the source scatterlist with dma_map_sg(), which returns 0 on failure, but the result is checked with:

	if (src_nents < 0) {
		ret = src_nents;
		goto error_unmap_dst;
	}

so a mapping failure is not detected. The code then calls qce_dma_prep_sgs() with zero source entries, which fails, and the error path calls dma_unmap_sg() on a scatterlist that was never mapped.

Check for !src_nents instead and return -EIO, matching the check that is already used for the destination scatterlist. The error_unmap_dst path skips the source unmap and only releases the destination mapping, which is the one that was successfully created.

---
 drivers/crypto/qce/aead.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 5bf59cb9ae9a..15ce778f019a 100644
@@ -456,8 +456,8 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
 
 	if (diff_dst) {
 		src_nents = dma_map_sg(qce->dev, rctx->src_sg, rctx->src_nents, dir_src);
-		if (src_nents < 0) {
-			ret = src_nents;
+		if (!src_nents) {
+			ret = -EIO;
 			goto error_unmap_dst;
 		}
 	} else {

2.50.1

All three look correct to me, thanks!

Note that I reported these bugs (and others still unfixed) 3 months ago:
https://lore.kernel.org/linux-crypto/20260723183438.GB93534@quark/

- Eric

Hi Eric,

Thanks - you're right, and I should have dug up the original thread first. The Sashiko review only flagged these as pre-existing; you reported them first. I'll credit that properly and resend:

I'll fold those trailers in and send a v2 shortly.

The remaining bugs you listed are on my radar; I can take them as separate follow-ups unless you'd prefer otherwise.

Thanks,
ZhaoJinming

Please check the Sashiko review:

https://sashiko.dev/#/patchset/20261016-fix-qce-ccm-adata-leak-v1-0-f54a82af2de0%40uniontech.com

Thanks,

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