[PATCH] crypto: hisilicon/sec - Fix element drop and UAF in sec_send_request()

[PATCH] crypto: hisilicon/sec - Fix element drop and UAF in sec_send_request()

[Arbab Haider]

The capacity check in sec_alg_skcipher_crypto() only verified that the
hardware queue could accept all steps or that the softqueue had
capacity, but failed to account for the case where softqueue already
has pending entries. When softqueue is non-empty, all new entries must
go to softqueue (to preserve cipher chaining ordering), even if the
hardware queue has available capacity.

This means the check could pass via hardware capacity, yet the first
element goes to hardware and subsequent elements go to softqueue —
which might be full. kfifo_put() silently returns 0 on a full fifo,
so those elements are dropped and the request never completes.

Fix by:

- Replacing the capacity check with correct logic that accounts
  for all three queueing scenarios.

- Making sec_send_request() void and checking both sec_queue_send()
  and kfifo_put() with WARN_ON_ONCE. A return value is not returned
  to the caller because a mid-loop error after partial queuing would
  cause the caller's err_free_elements path to free elements still
  referenced by the hardware shadow array or softqueue (UAF). The
  corrected capacity check guarantees these paths are unreachable.

Fixes: 915e4e8413da ("crypto: hisilicon - SEC security accelerator driver")
Signed-off-by: Arbab Haider <arbabhaider649@gmail.com>
---
 drivers/crypto/hisilicon/sec/sec_algs.c | 65 ++++++++++++++-----------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/crypto/hisilicon/sec/sec_algs.c b/drivers/crypto/hisilicon/sec/sec_algs.c
index 85eecbb40e7e..ce1dd95455ff 100644
--- a/drivers/crypto/hisilicon/sec/sec_algs.c
+++ b/drivers/crypto/hisilicon/sec/sec_algs.c
@@ -381,10 +381,9 @@ static void sec_alg_free_el(struct sec_request_el *el,
 }
 
 /* queuelock must be held */
-static int sec_send_request(struct sec_request *sec_req, struct sec_queue *queue)
+static void sec_send_request(struct sec_request *sec_req, struct sec_queue *queue)
 {
 	struct sec_request_el *el, *temp;
-	int ret = 0;
 
 	mutex_lock(&sec_req->lock);
 	list_for_each_entry_safe(el, temp, &sec_req->elements, head) {
@@ -401,21 +400,17 @@ static int sec_send_request(struct sec_request *sec_req, struct sec_queue *queue
 		if (!queue->havesoftqueue ||
 		    (kfifo_is_empty(&queue->softqueue) &&
 		     sec_queue_empty(queue))) {
-			ret = sec_queue_send(queue, &el->req, sec_req);
-			if (ret == -EAGAIN) {
-				/* Wait unti we can send then try again */
-				/* DEAD if here - should not happen */
-				ret = -EBUSY;
-				goto err_unlock;
-			}
+			if (WARN_ON_ONCE(sec_queue_send(queue, &el->req,
+							sec_req)))
+				/* Should not happen with proper capacity check */
+				;
 		} else {
-			kfifo_put(&queue->softqueue, el);
+			if (WARN_ON_ONCE(!kfifo_put(&queue->softqueue, el)))
+				/* Should not happen with proper capacity check */
+				;
 		}
 	}
-err_unlock:
 	mutex_unlock(&sec_req->lock);
-
-	return ret;
 }
 
 static void sec_skcipher_alg_callback(struct sec_bd_info *sec_resp,
@@ -790,28 +785,44 @@ static int sec_alg_skcipher_crypto(struct skcipher_request *skreq,
 
 	/*
 	 * Only attempt to queue if the whole lot can fit in the queue -
-	 * we can't successfully cleanup after a partial queing so this
+	 * we can't successfully cleanup after a partial queuing so this
 	 * must succeed or fail atomically.
 	 *
-	 * Big hammer test of both software and hardware queues - could be
-	 * more refined but this is unlikely to happen so no need.
+	 * When the softqueue has pending entries, all new entries must
+	 * go to the softqueue to preserve ordering.  When both queues
+	 * are empty the first entry goes to hardware and the rest to
+	 * the softqueue (if enabled).
 	 */
 
 	/* Grab a big lock for a long time to avoid concurrency issues */
 	spin_lock_bh(&queue->queuelock);
 
 	/*
-	 * Can go on to queue if we have space in either:
-	 * 1) The hardware queue and no software queue
-	 * 2) The software queue
-	 * AND there is nothing in the backlog.  If there is backlog we
-	 * have to only queue to the backlog queue and return busy.
+	 * Can go on to queue if we have space for everything upfront.
+	 * If there is backlog we must queue to the backlog and return busy.
 	 */
-	if ((!sec_queue_can_enqueue(queue, steps) &&
-	     (!queue->havesoftqueue ||
-	      kfifo_avail(&queue->softqueue) > steps)) ||
-	    !list_empty(&ctx->backlog)) {
+	if (!list_empty(&ctx->backlog)) {
 		ret = -EBUSY;
+		goto backlog;
+	}
+
+	if (!queue->havesoftqueue) {
+		if (!sec_queue_can_enqueue(queue, steps))
+			ret = -EBUSY;
+	} else if (!kfifo_is_empty(&queue->softqueue) ||
+		   !sec_queue_empty(queue)) {
+		/* All entries must go to the softqueue */
+		if (kfifo_avail(&queue->softqueue) <= steps)
+			ret = -EBUSY;
+	} else {
+		/* First to hardware, rest to softqueue */
+		if (!sec_queue_can_enqueue(queue, 1) ||
+		    kfifo_avail(&queue->softqueue) < steps - 1)
+			ret = -EBUSY;
+	}
+
+	if (ret) {
+backlog:
 		if ((skreq->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
 			list_add_tail(&sec_req->backlog_head, &ctx->backlog);
 			spin_unlock_bh(&queue->queuelock);
@@ -821,10 +832,8 @@ static int sec_alg_skcipher_crypto(struct skcipher_request *skreq,
 		spin_unlock_bh(&queue->queuelock);
 		goto err_free_elements;
 	}
-	ret = sec_send_request(sec_req, queue);
+	sec_send_request(sec_req, queue);
 	spin_unlock_bh(&queue->queuelock);
-	if (ret)
-		goto err_free_elements;
 
 	ret = -EINPROGRESS;
 out:
-- 
2.53.0

Re: [PATCH] crypto: hisilicon/sec - Fix element drop and UAF in sec_send_request()

[kernel test robot]

Hi Arbab,

kernel test robot noticed the following build warnings:

[auto build test WARNING on herbert-cryptodev-2.6/master]
[also build test WARNING on herbert-crypto-2.6/master linus/master v7.2-rc5 next-20260731]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Arbab-Haider/crypto-hisilicon-sec-Fix-element-drop-and-UAF-in-sec_send_request/20260731-234619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:    https://lore.kernel.org/r/20260730215449.299311-1-arbabhaider649%40gmail.com
patch subject: [PATCH] crypto: hisilicon/sec - Fix element drop and UAF in sec_send_request()
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20260801/202608010642.kGjbBrg7-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 16.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260801/202608010642.kGjbBrg7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202608010642.kGjbBrg7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/crypto/hisilicon/sec/sec_algs.c: In function 'sec_send_request':
>> drivers/crypto/hisilicon/sec/sec_algs.c:406:33: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
     406 |                                 ;
         |                                 ^
   drivers/crypto/hisilicon/sec/sec_algs.c:410:33: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
     410 |                                 ;
         |                                 ^


vim +/if +406 drivers/crypto/hisilicon/sec/sec_algs.c

   382	
   383	/* queuelock must be held */
   384	static void sec_send_request(struct sec_request *sec_req, struct sec_queue *queue)
   385	{
   386		struct sec_request_el *el, *temp;
   387	
   388		mutex_lock(&sec_req->lock);
   389		list_for_each_entry_safe(el, temp, &sec_req->elements, head) {
   390			/*
   391			 * Add to hardware queue only under following circumstances
   392			 * 1) Software and hardware queue empty so no chain dependencies
   393			 * 2) No dependencies as new IV - (check software queue empty
   394			 *    to maintain order)
   395			 * 3) No dependencies because the mode does no chaining.
   396			 *
   397			 * In other cases first insert onto the software queue which
   398			 * is then emptied as requests complete
   399			 */
   400			if (!queue->havesoftqueue ||
   401			    (kfifo_is_empty(&queue->softqueue) &&
   402			     sec_queue_empty(queue))) {
   403				if (WARN_ON_ONCE(sec_queue_send(queue, &el->req,
   404								sec_req)))
   405					/* Should not happen with proper capacity check */
 > 406					;
   407			} else {
   408				if (WARN_ON_ONCE(!kfifo_put(&queue->softqueue, el)))
   409					/* Should not happen with proper capacity check */
   410					;
   411			}
   412		}
   413		mutex_unlock(&sec_req->lock);
   414	}
   415	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki