Re: [PATCH] crypto: hisilicon/sec - Fix use-after-free and unchecked kfifo_put in sec_send_request()

On 2026/7/30 20:37, Arbab Haider wrote:
> The capacity check in sec_alg_skcipher_crypto() only verified that the
> hardware queue could accept all elements or that the software queue had
> enough space, but failed to account for the case where the software
> queue already had pending entries. When the software queue is non-empty,
> all new entries must go to the software queue to preserve cipher
> chaining ordering, even if the hardware queue has available capacity.
> 
> This could lead to two issues:
> 
> 1. Elements silently dropped from the software queue if
>    kfifo_put() returns 0 (queue full), causing the request to never
>    complete.
> 
> 2. Use-after-free if sec_queue_send() returns -EAGAIN after some
>    elements were already queued to hardware or software — the error
>    path in sec_send_request() returns -EBUSY, and the caller's
>    err_free_elements path frees all elements, including those
>    referenced by the hardware queue or software queue.
> 
> Fix by:
> 
> - Checking the return value of kfifo_put() and returning -ENOSPC on
>   failure, with a WARN_ON_ONCE since this should not happen with the
>   corrected capacity check.
> 
> - Catching all errors from sec_queue_send() (not just -EAGAIN) with a
>   WARN_ON_ONCE, as this path is unreachable with proper capacity
>   accounting.
> 
> - Replacing the capacity check with correct logic that accounts for
>   the three possible queueing scenarios:
>   * Software queue has entries or hardware queue is non-empty: all
>     entries go to the software queue → verify software queue capacity
>     for all steps.
>   * Both queues empty: first entry goes to hardware, rest to software
>     → verify hardware can take 1 and software can take steps - 1.
>   * No software queue configured: all to hardware → verify hardware
>     capacity for all steps.
> 
> Fixes: 915e4e8413da ("crypto: hisilicon - SEC security accelerator driver")
> Signed-off-by: Arbab Haider <[email protected]>
> ---
>  drivers/crypto/hisilicon/sec/sec_algs.c | 51 +++++++++++++++++--------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/crypto/hisilicon/sec/sec_algs.c b/drivers/crypto/hisilicon/sec/sec_algs.c
> index 85eecbb40e7e..6cc08e9a07d0 100644
> --- a/drivers/crypto/hisilicon/sec/sec_algs.c
> +++ b/drivers/crypto/hisilicon/sec/sec_algs.c
> @@ -402,14 +402,17 @@ static int sec_send_request(struct sec_request *sec_req, struct sec_queue *queue
>  		    (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 */
> +			if (ret) {
> +				WARN_ON_ONCE(ret == -EAGAIN);
>  				ret = -EBUSY;
>  				goto err_unlock;
>  			}
>  		} else {
> -			kfifo_put(&queue->softqueue, el);
> +			if (!kfifo_put(&queue->softqueue, el)) {
> +				WARN_ON_ONCE(1);
> +				ret = -ENOSPC;
> +				goto err_unlock;
> +			}
>  		}
>  	}
>  err_unlock:
> @@ -790,28 +793,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);
>
This SEC accelerator is a very old module that no longer has active users.
The primary maintainer, Jonathan, is no longer maintaining it.
Users have migrated to the new SEC2 accelerator instead.
Therefore, we have decided to remove this driver and will submit the deletion patch shortly.

Thank you.
Longfang.