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

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

[Pablo de Lara]

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++;
-- 
2.17.1

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

[Ananyev, Konstantin]

> 
> 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

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

[Akhil Goyal]

On 8/2/2018 10:19 AM, Pablo de Lara wrote:
> 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>
> ---
>

Applied to dpdk-next-crypto
Aligned description to 75 characters.

Thanks