[dpdk-stable] patch 'crypto/aesni_mb: fix possible array overrun' has been queued to LTS release 17.11.5

[Yongseok Koh]

Hi,

FYI, your patch has been queued to LTS release 17.11.5

Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 12/01/18. So please
shout if anyone has objections.

Also note that after the patch there's a diff of the upstream commit vs the patch applied
to the branch. If the code is different (ie: not only metadata diffs), due for example to
a change in context or macro names, please double check it.

Thanks.

Yongseok

---
>From a215e63fca05d81655ab402b147c530b67613862 Mon Sep 17 00:00:00 2001
From: Pablo de Lara <pablo.de.lara.guarch at intel.com>
Date: Thu, 2 Aug 2018 05:49:40 +0100
Subject: [PATCH] crypto/aesni_mb: fix possible array overrun

[ upstream commit 95978a85a410e7fa1a03d4f3b90c8770f7f29e72 ]

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, 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")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch at intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev at 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 700438978..f5bb6aa91 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -663,22 +663,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.11.0

---
  Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- -	2018-11-29 15:01:46.711303920 -0800
+++ 0033-crypto-aesni_mb-fix-possible-array-overrun.patch	2018-11-29 15:01:45.042968000 -0800
@@ -1,8 +1,10 @@
-From 95978a85a410e7fa1a03d4f3b90c8770f7f29e72 Mon Sep 17 00:00:00 2001
+From a215e63fca05d81655ab402b147c530b67613862 Mon Sep 17 00:00:00 2001
 From: Pablo de Lara <pablo.de.lara.guarch at intel.com>
 Date: Thu, 2 Aug 2018 05:49:40 +0100
 Subject: [PATCH] crypto/aesni_mb: fix possible array overrun
 
+[ upstream commit 95978a85a410e7fa1a03d4f3b90c8770f7f29e72 ]
+
 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.
@@ -36,7 +38,6 @@
 happens.
 
 Fixes: 0f548b50a160 ("crypto/aesni_mb: process crypto op on dequeue")
-Cc: stable at dpdk.org
 
 Signed-off-by: Pablo de Lara <pablo.de.lara.guarch at intel.com>
 Acked-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
@@ -45,10 +46,10 @@
  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
+index 700438978..f5bb6aa91 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,
+@@ -663,22 +663,30 @@ aesni_mb_pmd_dequeue_burst(void *queue_pair, struct rte_crypto_op **ops,
  
  	uint8_t digest_idx = qp->digest_idx;
  	do {
@@ -58,7 +59,7 @@
 -			break;
 -
  		/* Get next free mb job struct from mb manager */
- 		job = (*qp->op_fns->job.get_next)(qp->mb_mgr);
+ 		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,
@@ -69,7 +70,7 @@
 +			if (nb_ops == processed_jobs)
 +				break;
  
- 			job = (*qp->op_fns->job.get_next)(qp->mb_mgr);
+ 			job = (*qp->op_fns->job.get_next)(&qp->mb_mgr);
  		}
  
 +		/*