crypto: chelsio - Fix wrcount leak in chcr_aes_decrypt()

chcr_aes_decrypt() increments the device inflight count with chcr_inc_wrcount() before building the work request, but it returns early without dropping the count when the tx queue is full or when process_cipher() fails and returns no skb. The count is only dropped by the completion handlers, so every failed request permanently skews the count and eventually blocks the device from being detached, with the detach worker warning "CHCR:request Still Pending".

Add an error label that calls chcr_dec_wrcount(), mirroring chcr_aes_encrypt(), and jump to it on the failure paths.

---
 drivers/crypto/chelsio/chcr_algo.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

index eece1ac1085a..6fd25c54aa1e 100644
@@ -1396,17 +1396,22 @@ static int chcr_aes_decrypt(struct skcipher_request *req)
 		return -ENXIO;
 
 	if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
-						reqctx->txqidx) &&
-		(!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))))
-			return -ENOSPC;
+					    reqctx->txqidx) &&
+	    (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))) {
+		err = -ENOSPC;
+		goto error;
+	}
 	err = process_cipher(req, u_ctx->lldi.rxq_ids[reqctx->rxqidx],
 			     &skb, CHCR_DECRYPT_OP);
 	if (err || !skb)
-		return err;
+		goto error;
 	skb->dev = u_ctx->lldi.ports[0];
 	set_wr_txq(skb, CPL_PRIORITY_DATA, reqctx->txqidx);
 	chcr_send_wr(skb);
 	return -EINPROGRESS;
+error:
+	chcr_dec_wrcount(dev);
+	return err;
 }
 static int chcr_device_init(struct chcr_context *ctx)
 {

2.34.1

Plesae combine this with the encrypt patch and repost.

Thanks,

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

chcr_aes_encrypt() and chcr_aes_decrypt() increment the device inflight count with chcr_inc_wrcount() before building the work request, but both can return early without dropping the count.

chcr_aes_encrypt() leaks the count when process_cipher() fails or returns no skb. chcr_aes_decrypt() leaks it when the tx queue is full or when process_cipher() fails and returns no skb. No completion can balance those increments, so every failed request permanently skews the count and eventually blocks the device from being detached, with the detach worker warning "CHCR:request Still Pending".

chcr_aes_encrypt() already has an error label that calls chcr_dec_wrcount(), so jump to it on the remaining failure path. Add the same label to chcr_aes_decrypt() and jump to it from both of its failure paths, mirroring the encrypt side.

---
Changes in v2:
- Fix chcr_aes_encrypt() and chcr_aes_decrypt() in a single patch, as requested. Sorry for the late reply.
- Link to v1: https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/

---
 drivers/crypto/chelsio/chcr_algo.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

index e2f2b2a6200e..9ce9eefef777 100644
@@ -1359,7 +1359,7 @@ static int chcr_aes_encrypt(struct skcipher_request *req)
 	err = process_cipher(req, u_ctx->lldi.rxq_ids[reqctx->rxqidx],
 			     &skb, CHCR_ENCRYPT_OP);
 	if (err || !skb)
-		return  err;
+		goto error;
 	skb->dev = u_ctx->lldi.ports[0];
 	set_wr_txq(skb, CPL_PRIORITY_DATA, reqctx->txqidx);
 	chcr_send_wr(skb);

@@ -1397,16 +1397,21 @@ static int chcr_aes_decrypt(struct skcipher_request *req)
 
 	if (unlikely(cxgb4_is_crypto_q_full(u_ctx->lldi.ports[0],
 						reqctx->txqidx) &&
-		(!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))))
-			return -ENOSPC;
+		(!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))) {
+		err = -ENOSPC;
+		goto error;
+	}
 	err = process_cipher(req, u_ctx->lldi.rxq_ids[reqctx->rxqidx],
 			     &skb, CHCR_DECRYPT_OP);
 	if (err || !skb)
-		return err;
+		goto error;
 	skb->dev = u_ctx->lldi.ports[0];
 	set_wr_txq(skb, CPL_PRIORITY_DATA, reqctx->txqidx);
 	chcr_send_wr(skb);
 	return -EINPROGRESS;
+error:
+	chcr_dec_wrcount(dev);
+	return err;
 }
 static int chcr_device_init(struct chcr_context *ctx)
 {

2.34.1

After further discussions/postings it is likely that vulab@ is not a real person, but a bot. It is also possible that it hides intentional bugs among good looking patches, thus please be cautious and thoroughly review the patch.

Best regards,
Krzysztof