From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A5F9456A3; Mon, 27 Aug 2018 18:02:30 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Aug 2018 09:02:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,295,1531810800"; d="scan'208";a="68472054" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by orsmga008.jf.intel.com with ESMTP; 27 Aug 2018 09:02:04 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.54]) by IRSMSX104.ger.corp.intel.com ([169.254.5.213]) with mapi id 14.03.0319.002; Mon, 27 Aug 2018 17:02:03 +0100 From: "Ananyev, Konstantin" To: "De Lara Guarch, Pablo" , "Doherty, Declan" CC: "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH] crypto/aesni_mb: fix possible array overrun Thread-Index: AQHUKmAz1fNNM+59tkG+C9hcnuvSIaTT6ZIQ Date: Mon, 27 Aug 2018 16:02:03 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258EA94D65E@irsmsx105.ger.corp.intel.com> References: <20180802044940.23114-1-pablo.de.lara.guarch@intel.com> In-Reply-To: <20180802044940.23114-1-pablo.de.lara.guarch@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTBkY2NlMmEtNThkOC00MWY3LTlkNzUtMmJhZWMyNTdlYzZmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWklLQWxRWXVlSFZSQkRnckpVRnZoWjVuWEJrNlFCeEx4TFpkZUo5OGFTaU9lS0NONWs0eUNuT29ua2ZaWFF5VCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] crypto/aesni_mb: fix possible array overrun X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Aug 2018 16:02:31 -0000 >=20 > 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. >=20 > 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. >=20 > In order to avoid leaving the dequeued operation without being processed, > the maximum number of operations that can be flushed is the remaining ope= rations > to return, which is the maximum number of operations that can be return m= inus > the number of operations ready to be returned (nb_ops - processed_jobs), > minus 1 (for the new operation). >=20 > 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 eq= ual 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. >=20 > 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 a= re returned > from the manager, then no more outstanding operations get dequeued from t= he 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 happ= ens. >=20 > Fixes: 0f548b50a160 ("crypto/aesni_mb: process crypto op on dequeue") > Cc: stable@dpdk.org >=20 > Signed-off-by: Pablo de Lara > --- > drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) >=20 > 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, >=20 > uint8_t digest_idx =3D qp->digest_idx; > do { > - /* Get next operation to process from ingress queue */ > - retval =3D rte_ring_dequeue(qp->ingress_queue, (void **)&op); > - if (retval < 0) > - break; > - > /* Get next free mb job struct from mb manager */ > job =3D (*qp->op_fns->job.get_next)(qp->mb_mgr); > if (unlikely(job =3D=3D NULL)) { > /* if no free mb job structs we need to flush mb_mgr */ > processed_jobs +=3D flush_mb_mgr(qp, > &ops[processed_jobs], > - (nb_ops - processed_jobs) - 1); > + nb_ops - processed_jobs); > + > + if (nb_ops =3D=3D processed_jobs) > + break; >=20 > job =3D (*qp->op_fns->job.get_next)(qp->mb_mgr); > } >=20 > + /* > + * 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 =3D rte_ring_dequeue(qp->ingress_queue, (void **)&op); > + if (retval < 0) > + break; > + > retval =3D set_mb_job_params(job, qp, op, &digest_idx); > if (unlikely(retval !=3D 0)) { > qp->stats.dequeue_err_count++; > -- Acked-by: Konstantin Ananyev > 2.17.1