Re: [dpdk-dev] [PATCH] crypto/aesni_mb: fix possible array overrun

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> In order to process crypto operations in the AESNI MB PMD,
> they need to be sent to the buffer manager of the Multi-buffer library,
> through the "job" structure.
> 
> Currently, it is checked if there are outstanding operations to process
> in the ring, before getting a new job. However, if there are no available
> jobs in the manager, a flush operation needs to take place, freeing some of the jobs,
> so it can be used for the outstanding operation.
> 
> In order to avoid leaving the dequeued operation without being processed,
> the maximum number of operations that can be flushed is the remaining operations
> to return, which is the maximum number of operations that can be return minus
> the number of operations ready to be returned (nb_ops - processed_jobs),
> minus 1 (for the new operation).
> 
> The problem comes when (nb_ops - processed_jobs) is 1 (last operation to dequeue).
> In that case, flush_mb_mgr is called with maximum number of operations equal to 0,
> which is wrong, causing a potential overrun in the "ops" array.
> Besides, the operation dequeued from the ring will be leaked, as no more operations can
> be returned.
> 
> The solution is to first check if there are jobs available in the manager.
> If there are not, flush operation gets called, and if enough operations are returned
> from the manager, then no more outstanding operations get dequeued from the ring,
> avoiding both the memory leak and the array overrun.
> If there are enough jobs, the PMD tries to dequeue an operation from the ring.
> If there are no operations in the ring, the new job pointer is not used,
> and it will be used in the next get_next_job call, so no memory leak happens.
> 
> Fixes: 0f548b50a160 ("crypto/aesni_mb: process crypto op on dequeue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> index 93dc7a443..e2dd834f0 100644
> --- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> +++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> @@ -833,22 +833,30 @@ aesni_mb_pmd_dequeue_burst(void *queue_pair, struct rte_crypto_op **ops,
> 
>  	uint8_t digest_idx = qp->digest_idx;
>  	do {
> -		/* Get next operation to process from ingress queue */
> -		retval = rte_ring_dequeue(qp->ingress_queue, (void **)&op);
> -		if (retval < 0)
> -			break;
> -
>  		/* Get next free mb job struct from mb manager */
>  		job = (*qp->op_fns->job.get_next)(qp->mb_mgr);
>  		if (unlikely(job == NULL)) {
>  			/* if no free mb job structs we need to flush mb_mgr */
>  			processed_jobs += flush_mb_mgr(qp,
>  					&ops[processed_jobs],
> -					(nb_ops - processed_jobs) - 1);
> +					nb_ops - processed_jobs);
> +
> +			if (nb_ops == processed_jobs)
> +				break;
> 
>  			job = (*qp->op_fns->job.get_next)(qp->mb_mgr);
>  		}
> 
> +		/*
> +		 * Get next operation to process from ingress queue.
> +		 * There is no need to return the job to the MB_MGR
> +		 * if there are no more operations to process, since the MB_MGR
> +		 * can use that pointer again in next get_next calls.
> +		 */
> +		retval = rte_ring_dequeue(qp->ingress_queue, (void **)&op);
> +		if (retval < 0)
> +			break;
> +
>  		retval = set_mb_job_params(job, qp, op, &digest_idx);
>  		if (unlikely(retval != 0)) {
>  			qp->stats.dequeue_err_count++;
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1