diff mbox series

test: fix crypto_op length for sessionless case

Message ID 20210630124637.2987722-1-abhinandan.gujjar@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: akhil goyal
Headers
Series test: fix crypto_op length for sessionless case |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Gujjar, Abhinandan S June 30, 2021, 12:46 p.m. UTC 
  Currently, private_data_offset for the sessionless is computed
wrongly which includes extra bytes added because of using
sizeof(struct rte_crypto_sym_xform) * 2) instead of
(sizeof(union rte_event_crypto_metadata)). Due to this buffer
overflow, the corruption was leading to test application
crash while freeing the ops mempool.

Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
Reported-by: ciara.power@intel.com

Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
 app/test/test_event_crypto_adapter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Gujjar, Abhinandan S July 2, 2021, 5:08 p.m. UTC | #1 
Hi Aaron/dpdklab,

This patch's CI seems to have lot of false positive!
Ferruh triggered the re-test sometime back. Now, it is reporting less.
Could you please check from your end? Thanks!

Regards
Abhinandan


> -----Original Message-----
> From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> Sent: Wednesday, June 30, 2021 6:17 PM
> To: dev@dpdk.org; jerinj@marvell.com
> Cc: gakhil@marvell.com; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>; Power, Ciara <ciara.power@intel.com>
> Subject: [PATCH] test: fix crypto_op length for sessionless case
> 
> Currently, private_data_offset for the sessionless is computed wrongly which
> includes extra bytes added because of using sizeof(struct
> rte_crypto_sym_xform) * 2) instead of (sizeof(union
> rte_event_crypto_metadata)). Due to this buffer overflow, the corruption was
> leading to test application crash while freeing the ops mempool.
> 
> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> Reported-by: ciara.power@intel.com
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  app/test/test_event_crypto_adapter.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index f689bc1f2..688ac0b2f 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
>  		first_xform = &cipher_xform;
>  		sym_op->xform = first_xform;
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> -				(sizeof(struct rte_crypto_sym_xform) * 2);
> +				(sizeof(union rte_event_crypto_metadata));
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
>  		rte_memcpy(&m_data.response_info, &response_info, @@ -
> 424,7 +424,7 @@ test_op_new_mode(uint8_t session_less)
>  		first_xform = &cipher_xform;
>  		sym_op->xform = first_xform;
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> -				(sizeof(struct rte_crypto_sym_xform) * 2);
> +				(sizeof(union rte_event_crypto_metadata));
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
>  		rte_memcpy(&m_data.response_info, &response_info,
> --
> 2.25.1
Ferruh Yigit July 2, 2021, 11:26 p.m. UTC | #2 
On 7/2/2021 7:08 PM, Gujjar, Abhinandan S wrote:
> Hi Aaron/dpdklab,
> 
> This patch's CI seems to have lot of false positive!
> Ferruh triggered the re-test sometime back. Now, it is reporting less.
> Could you please check from your end? Thanks!
> 

Only a malloc related unit test is still failing, which seems unrelated with the
patch. I am triggering it one more time, third time lucky.

Also after re-run, some tests passing now still shown as fail in the patchwork
checks table. Isn't re-run sending the patchwork test status again?

> Regards
> Abhinandan
> 
> 
>> -----Original Message-----
>> From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
>> Sent: Wednesday, June 30, 2021 6:17 PM
>> To: dev@dpdk.org; jerinj@marvell.com
>> Cc: gakhil@marvell.com; Gujjar, Abhinandan S
>> <abhinandan.gujjar@intel.com>; Power, Ciara <ciara.power@intel.com>
>> Subject: [PATCH] test: fix crypto_op length for sessionless case
>>
>> Currently, private_data_offset for the sessionless is computed wrongly which
>> includes extra bytes added because of using sizeof(struct
>> rte_crypto_sym_xform) * 2) instead of (sizeof(union
>> rte_event_crypto_metadata)). Due to this buffer overflow, the corruption was
>> leading to test application crash while freeing the ops mempool.
>>
>> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
>> Reported-by: ciara.power@intel.com
>>
>> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
>> ---
>>  app/test/test_event_crypto_adapter.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test/test_event_crypto_adapter.c
>> b/app/test/test_event_crypto_adapter.c
>> index f689bc1f2..688ac0b2f 100644
>> --- a/app/test/test_event_crypto_adapter.c
>> +++ b/app/test/test_event_crypto_adapter.c
>> @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
>>               first_xform = &cipher_xform;
>>               sym_op->xform = first_xform;
>>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
>> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
>> +                             (sizeof(union rte_event_crypto_metadata));
>>               op->private_data_offset = len;
>>               /* Fill in private data information */
>>               rte_memcpy(&m_data.response_info, &response_info, @@ -
>> 424,7 +424,7 @@ test_op_new_mode(uint8_t session_less)
>>               first_xform = &cipher_xform;
>>               sym_op->xform = first_xform;
>>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
>> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
>> +                             (sizeof(union rte_event_crypto_metadata));
>>               op->private_data_offset = len;
>>               /* Fill in private data information */
>>               rte_memcpy(&m_data.response_info, &response_info,
>> --
>> 2.25.1
>
Gujjar, Abhinandan S July 5, 2021, 6:30 a.m. UTC | #3 
Hi Jerin/Akhil,

Could you please review the patch?

Regards
Abhinandan

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Saturday, July 3, 2021 4:56 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> jerinj@marvell.com; dpdklab@iol.unh.edu; aconole@redhat.com
> Cc: gakhil@marvell.com; Power, Ciara <ciara.power@intel.com>; Ali Alnubani
> <alialnu@nvidia.com>
> Subject: Re: [PATCH] test: fix crypto_op length for sessionless case
> 
> On 7/2/2021 7:08 PM, Gujjar, Abhinandan S wrote:
> > Hi Aaron/dpdklab,
> >
> > This patch's CI seems to have lot of false positive!
> > Ferruh triggered the re-test sometime back. Now, it is reporting less.
> > Could you please check from your end? Thanks!
> >
> 
> Only a malloc related unit test is still failing, which seems unrelated with the
> patch. I am triggering it one more time, third time lucky.
> 
> Also after re-run, some tests passing now still shown as fail in the patchwork
> checks table. Isn't re-run sending the patchwork test status again?
> 
> > Regards
> > Abhinandan
> >
> >
> >> -----Original Message-----
> >> From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> >> Sent: Wednesday, June 30, 2021 6:17 PM
> >> To: dev@dpdk.org; jerinj@marvell.com
> >> Cc: gakhil@marvell.com; Gujjar, Abhinandan S
> >> <abhinandan.gujjar@intel.com>; Power, Ciara <ciara.power@intel.com>
> >> Subject: [PATCH] test: fix crypto_op length for sessionless case
> >>
> >> Currently, private_data_offset for the sessionless is computed
> >> wrongly which includes extra bytes added because of using
> >> sizeof(struct
> >> rte_crypto_sym_xform) * 2) instead of (sizeof(union
> >> rte_event_crypto_metadata)). Due to this buffer overflow, the
> >> corruption was leading to test application crash while freeing the ops
> mempool.
> >>
> >> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> >> Reported-by: ciara.power@intel.com
> >>
> >> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> >> ---
> >>  app/test/test_event_crypto_adapter.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test/test_event_crypto_adapter.c
> >> b/app/test/test_event_crypto_adapter.c
> >> index f689bc1f2..688ac0b2f 100644
> >> --- a/app/test/test_event_crypto_adapter.c
> >> +++ b/app/test/test_event_crypto_adapter.c
> >> @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> >>               first_xform = &cipher_xform;
> >>               sym_op->xform = first_xform;
> >>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> >> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
> >> +                             (sizeof(union
> >> + rte_event_crypto_metadata));
> >>               op->private_data_offset = len;
> >>               /* Fill in private data information */
> >>               rte_memcpy(&m_data.response_info, &response_info, @@ -
> >> 424,7 +424,7 @@ test_op_new_mode(uint8_t session_less)
> >>               first_xform = &cipher_xform;
> >>               sym_op->xform = first_xform;
> >>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> >> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
> >> +                             (sizeof(union
> >> + rte_event_crypto_metadata));
> >>               op->private_data_offset = len;
> >>               /* Fill in private data information */
> >>               rte_memcpy(&m_data.response_info, &response_info,
> >> --
> >> 2.25.1
> >
Brandon Lo July 6, 2021, 4:09 p.m. UTC | #4 
Hi all,

I have rerun the failing unit test. It also recreated the report, so
that category should be passing now.
Currently, I am looking more into the ABI test that is failing on
Arch, as well as the failures with DTS tests.
I will keep this thread updated.

Thanks,
Brandon

On Mon, Jul 5, 2021 at 2:30 AM Gujjar, Abhinandan S
<abhinandan.gujjar@intel.com> wrote:
>
> Hi Jerin/Akhil,
>
> Could you please review the patch?
>
> Regards
> Abhinandan
>
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: Saturday, July 3, 2021 4:56 AM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> > jerinj@marvell.com; dpdklab@iol.unh.edu; aconole@redhat.com
> > Cc: gakhil@marvell.com; Power, Ciara <ciara.power@intel.com>; Ali Alnubani
> > <alialnu@nvidia.com>
> > Subject: Re: [PATCH] test: fix crypto_op length for sessionless case
> >
> > On 7/2/2021 7:08 PM, Gujjar, Abhinandan S wrote:
> > > Hi Aaron/dpdklab,
> > >
> > > This patch's CI seems to have lot of false positive!
> > > Ferruh triggered the re-test sometime back. Now, it is reporting less.
> > > Could you please check from your end? Thanks!
> > >
> >
> > Only a malloc related unit test is still failing, which seems unrelated with the
> > patch. I am triggering it one more time, third time lucky.
> >
> > Also after re-run, some tests passing now still shown as fail in the patchwork
> > checks table. Isn't re-run sending the patchwork test status again?
> >
> > > Regards
> > > Abhinandan
> > >
> > >
> > >> -----Original Message-----
> > >> From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> > >> Sent: Wednesday, June 30, 2021 6:17 PM
> > >> To: dev@dpdk.org; jerinj@marvell.com
> > >> Cc: gakhil@marvell.com; Gujjar, Abhinandan S
> > >> <abhinandan.gujjar@intel.com>; Power, Ciara <ciara.power@intel.com>
> > >> Subject: [PATCH] test: fix crypto_op length for sessionless case
> > >>
> > >> Currently, private_data_offset for the sessionless is computed
> > >> wrongly which includes extra bytes added because of using
> > >> sizeof(struct
> > >> rte_crypto_sym_xform) * 2) instead of (sizeof(union
> > >> rte_event_crypto_metadata)). Due to this buffer overflow, the
> > >> corruption was leading to test application crash while freeing the ops
> > mempool.
> > >>
> > >> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > >> Reported-by: ciara.power@intel.com
> > >>
> > >> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > >> ---
> > >>  app/test/test_event_crypto_adapter.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/app/test/test_event_crypto_adapter.c
> > >> b/app/test/test_event_crypto_adapter.c
> > >> index f689bc1f2..688ac0b2f 100644
> > >> --- a/app/test/test_event_crypto_adapter.c
> > >> +++ b/app/test/test_event_crypto_adapter.c
> > >> @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> > >>               first_xform = &cipher_xform;
> > >>               sym_op->xform = first_xform;
> > >>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > >> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
> > >> +                             (sizeof(union
> > >> + rte_event_crypto_metadata));
> > >>               op->private_data_offset = len;
> > >>               /* Fill in private data information */
> > >>               rte_memcpy(&m_data.response_info, &response_info, @@ -
> > >> 424,7 +424,7 @@ test_op_new_mode(uint8_t session_less)
> > >>               first_xform = &cipher_xform;
> > >>               sym_op->xform = first_xform;
> > >>               uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > >> -                             (sizeof(struct rte_crypto_sym_xform) * 2);
> > >> +                             (sizeof(union
> > >> + rte_event_crypto_metadata));
> > >>               op->private_data_offset = len;
> > >>               /* Fill in private data information */
> > >>               rte_memcpy(&m_data.response_info, &response_info,
> > >> --
> > >> 2.25.1
> > >
>
Akhil Goyal July 7, 2021, 2:07 p.m. UTC | #5 
Hi Abhinandan,

> Currently, private_data_offset for the sessionless is computed
> wrongly which includes extra bytes added because of using
> sizeof(struct rte_crypto_sym_xform) * 2) instead of
> (sizeof(union rte_event_crypto_metadata)). Due to this buffer
> overflow, the corruption was leading to test application
> crash while freeing the ops mempool.
> 
> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> Reported-by: ciara.power@intel.com
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  app/test/test_event_crypto_adapter.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index f689bc1f2..688ac0b2f 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
>  		first_xform = &cipher_xform;
>  		sym_op->xform = first_xform;
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> -				(sizeof(struct rte_crypto_sym_xform) * 2);
> +				(sizeof(union rte_event_crypto_metadata));
>  		op->private_data_offset = len;
I do not understand the need for this patch.
Event metadata is copied after private data offset,
and this patch is changing the offset value.

You changed the value of len = iv_off + max_iv_len + metadata_size,
but metadata is copied after this 'len'. See this
rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));

I do not agree with this patch, am I missing something?

>  		/* Fill in private data information */
>  		rte_memcpy(&m_data.response_info, &response_info,
> @@ -424,7 +424,7 @@ test_op_new_mode(uint8_t session_less)
>  		first_xform = &cipher_xform;
>  		sym_op->xform = first_xform;
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> -				(sizeof(struct rte_crypto_sym_xform) * 2);
> +				(sizeof(union rte_event_crypto_metadata));
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
>  		rte_memcpy(&m_data.response_info, &response_info,
> --
> 2.25.1
Gujjar, Abhinandan S July 8, 2021, 2:12 p.m. UTC | #6 
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, July 7, 2021 7:38 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: Power, Ciara <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless case
> 
> Hi Abhinandan,
> 
> > Currently, private_data_offset for the sessionless is computed wrongly
> > which includes extra bytes added because of using sizeof(struct
> > rte_crypto_sym_xform) * 2) instead of (sizeof(union
> > rte_event_crypto_metadata)). Due to this buffer overflow, the
> > corruption was leading to test application crash while freeing the ops
> > mempool.
> >
> > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > Reported-by: ciara.power@intel.com
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  app/test/test_event_crypto_adapter.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test/test_event_crypto_adapter.c
> > b/app/test/test_event_crypto_adapter.c
> > index f689bc1f2..688ac0b2f 100644
> > --- a/app/test/test_event_crypto_adapter.c
> > +++ b/app/test/test_event_crypto_adapter.c
> > @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> >  		first_xform = &cipher_xform;
> >  		sym_op->xform = first_xform;
> >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > +				(sizeof(union rte_event_crypto_metadata));
> >  		op->private_data_offset = len;
> I do not understand the need for this patch.
This is patch provide fix for segfault at the end of event_crypto_adapter_autotest()
RTE>>event_crypto_adapter_autotest 
 + ------------------------------------------------------- +
 + Test Suite : Event crypto adapter test suite
CRYPTODEV: Creating cryptodev crypto_nullCRYPTODEV: Initialisation parameters - name: crypto_null,socket id: 0, max queue pairs: 8
CRYPTODEV: elt_size 0 is expanded to 336 + ------------------------------------------------------- +
 + TestCase [ 0] : test_crypto_adapter_create succeeded
 + TestCase [ 1] : test_crypto_adapter_qp_add_del succeeded
 +------------------------------------------------------+
 + Crypto adapter stats for instance 0:
 + Event port poll count          0
 + Event dequeue count            0
 + Cryptodev enqueue count        0
 + Cryptodev enqueue failed count 0
 + Cryptodev dequeue count        0
 + Event enqueue count            0
 + Event enqueue retry count      0
 + Event enqueue fail count       0
 +------------------------------------------------------+
 + TestCase [ 2] : test_crypto_adapter_stats succeeded
Segmentation fault (core dumped)

> Event metadata is copied after private data offset, and this patch is changing
> the offset value.
> 
> You changed the value of len = iv_off + max_iv_len + metadata_size, but
> metadata is copied after this 'len'. See this rte_memcpy((uint8_t *)op + len,
> &m_data, sizeof(m_data));
Op_mpool is created with element of priv_size = DEFAULT_NUM_XFORMS * sizeof(struct rte_crypto_sym_xform) + MAXIMUM_IV_LENGTH.
Whereas for the "sessionless" length is set to " uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH + (sizeof(struct rte_crypto_sym_xform) * 2)"
Whereas, IV_OFFSET  = (sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct rte_crypto_sym_xform)).

So substituting IV_OFFSET, len = (sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct rte_crypto_sym_xform)) + MAXIMUM_IV_LENGTH + (sizeof(struct rte_crypto_sym_xform) * 2).
Which is a way ahead of the boundary which causes buffer overflow.

When memcpy is executed -> rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
The m_data will overwrite the beyond the boundary. Hope this clarifies the need for fix. 
> 
> I do not agree with this patch, am I missing something?
> 
> >  		/* Fill in private data information */
> >  		rte_memcpy(&m_data.response_info, &response_info, @@ -
> 424,7 +424,7
> > @@ test_op_new_mode(uint8_t session_less)
> >  		first_xform = &cipher_xform;
> >  		sym_op->xform = first_xform;
> >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > +				(sizeof(union rte_event_crypto_metadata));
> >  		op->private_data_offset = len;
> >  		/* Fill in private data information */
> >  		rte_memcpy(&m_data.response_info, &response_info,
> > --
> > 2.25.1
Akhil Goyal July 13, 2021, 9:11 a.m. UTC | #7 
Hi Abhinandan,
> >
> > > Currently, private_data_offset for the sessionless is computed wrongly
> > > which includes extra bytes added because of using sizeof(struct
> > > rte_crypto_sym_xform) * 2) instead of (sizeof(union
> > > rte_event_crypto_metadata)). Due to this buffer overflow, the
> > > corruption was leading to test application crash while freeing the ops
> > > mempool.
> > >
> > > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > > Reported-by: ciara.power@intel.com
> > >
> > > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > > ---
> > >  app/test/test_event_crypto_adapter.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test/test_event_crypto_adapter.c
> > > b/app/test/test_event_crypto_adapter.c
> > > index f689bc1f2..688ac0b2f 100644
> > > --- a/app/test/test_event_crypto_adapter.c
> > > +++ b/app/test/test_event_crypto_adapter.c
> > > @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> > >  		first_xform = &cipher_xform;
> > >  		sym_op->xform = first_xform;
> > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > > +				(sizeof(union rte_event_crypto_metadata));
> > >  		op->private_data_offset = len;
> > I do not understand the need for this patch.
> This is patch provide fix for segfault at the end of
> event_crypto_adapter_autotest()
> RTE>>event_crypto_adapter_autotest
>  + ------------------------------------------------------- +
>  + Test Suite : Event crypto adapter test suite
> CRYPTODEV: Creating cryptodev crypto_nullCRYPTODEV: Initialisation
> parameters - name: crypto_null,socket id: 0, max queue pairs: 8
> CRYPTODEV: elt_size 0 is expanded to 336 + -------------------------------------------
> ------------ +
>  + TestCase [ 0] : test_crypto_adapter_create succeeded
>  + TestCase [ 1] : test_crypto_adapter_qp_add_del succeeded
>  +------------------------------------------------------+
>  + Crypto adapter stats for instance 0:
>  + Event port poll count          0
>  + Event dequeue count            0
>  + Cryptodev enqueue count        0
>  + Cryptodev enqueue failed count 0
>  + Cryptodev dequeue count        0
>  + Event enqueue count            0
>  + Event enqueue retry count      0
>  + Event enqueue fail count       0
>  +------------------------------------------------------+
>  + TestCase [ 2] : test_crypto_adapter_stats succeeded
> Segmentation fault (core dumped)
> 
> > Event metadata is copied after private data offset, and this patch is
> changing
> > the offset value.
> >
> > You changed the value of len = iv_off + max_iv_len + metadata_size, but
> > metadata is copied after this 'len'. See this rte_memcpy((uint8_t *)op + len,
> > &m_data, sizeof(m_data));
> Op_mpool is created with element of priv_size = DEFAULT_NUM_XFORMS *
> sizeof(struct rte_crypto_sym_xform) + MAXIMUM_IV_LENGTH.
> Whereas for the "sessionless" length is set to " uint32_t len = IV_OFFSET +
> MAXIMUM_IV_LENGTH + (sizeof(struct rte_crypto_sym_xform) * 2)"
> Whereas, IV_OFFSET  = (sizeof(struct rte_crypto_op) + sizeof(struct
> rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> rte_crypto_sym_xform)).
> 
> So substituting IV_OFFSET, len = (sizeof(struct rte_crypto_op) + sizeof(struct
> rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> rte_crypto_sym_xform)) + MAXIMUM_IV_LENGTH + (sizeof(struct
> rte_crypto_sym_xform) * 2).
> Which is a way ahead of the boundary which causes buffer overflow.
> 
> When memcpy is executed -> rte_memcpy((uint8_t *)op + len, &m_data,
> sizeof(m_data));
> The m_data will overwrite the beyond the boundary. Hope this clarifies the
> need for fix.

You are setting len = sizeof(rte_crypto_op) + sizeof(rte_crypto_sym_op) + 2 *(sizeof(xform)) + IV_LEN + m_data_len
And then copying mdata at end of 'len', which is not correct. Here, len already include mdata and you are copying mdata after its designated space. Right? 
IMO, len should be set as IV_OFFSET+IV_LEN only.

> >
> > I do not agree with this patch, am I missing something?
> >
> > >  		/* Fill in private data information */
> > >  		rte_memcpy(&m_data.response_info, &response_info, @@
> -
> > 424,7 +424,7
> > > @@ test_op_new_mode(uint8_t session_less)
> > >  		first_xform = &cipher_xform;
> > >  		sym_op->xform = first_xform;
> > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > > +				(sizeof(union rte_event_crypto_metadata));
> > >  		op->private_data_offset = len;
> > >  		/* Fill in private data information */
> > >  		rte_memcpy(&m_data.response_info, &response_info,
> > > --
> > > 2.25.1
Gujjar, Abhinandan S July 18, 2021, 9:05 a.m. UTC | #8 
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, July 13, 2021 2:42 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: Power, Ciara <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless case
> 
> Hi Abhinandan,
> > >
> > > > Currently, private_data_offset for the sessionless is computed
> > > > wrongly which includes extra bytes added because of using
> > > > sizeof(struct
> > > > rte_crypto_sym_xform) * 2) instead of (sizeof(union
> > > > rte_event_crypto_metadata)). Due to this buffer overflow, the
> > > > corruption was leading to test application crash while freeing the
> > > > ops mempool.
> > > >
> > > > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > > > Reported-by: ciara.power@intel.com
> > > >
> > > > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > > > ---
> > > >  app/test/test_event_crypto_adapter.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/app/test/test_event_crypto_adapter.c
> > > > b/app/test/test_event_crypto_adapter.c
> > > > index f689bc1f2..688ac0b2f 100644
> > > > --- a/app/test/test_event_crypto_adapter.c
> > > > +++ b/app/test/test_event_crypto_adapter.c
> > > > @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> > > >  		first_xform = &cipher_xform;
> > > >  		sym_op->xform = first_xform;
> > > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > > > +				(sizeof(union rte_event_crypto_metadata));
> > > >  		op->private_data_offset = len;
> > > I do not understand the need for this patch.
> > This is patch provide fix for segfault at the end of
> > event_crypto_adapter_autotest()
> > RTE>>event_crypto_adapter_autotest
> >  + ------------------------------------------------------- +  + Test
> > Suite : Event crypto adapter test suite
> > CRYPTODEV: Creating cryptodev crypto_nullCRYPTODEV: Initialisation
> > parameters - name: crypto_null,socket id: 0, max queue pairs: 8
> > CRYPTODEV: elt_size 0 is expanded to 336 +
> > -------------------------------------------
> > ------------ +
> >  + TestCase [ 0] : test_crypto_adapter_create succeeded  + TestCase [
> > 1] : test_crypto_adapter_qp_add_del succeeded
> > +------------------------------------------------------+
> >  + Crypto adapter stats for instance 0:
> >  + Event port poll count          0
> >  + Event dequeue count            0
> >  + Cryptodev enqueue count        0
> >  + Cryptodev enqueue failed count 0
> >  + Cryptodev dequeue count        0
> >  + Event enqueue count            0
> >  + Event enqueue retry count      0
> >  + Event enqueue fail count       0
> >  +------------------------------------------------------+
> >  + TestCase [ 2] : test_crypto_adapter_stats succeeded Segmentation
> > fault (core dumped)
> >
> > > Event metadata is copied after private data offset, and this patch
> > > is
> > changing
> > > the offset value.
> > >
> > > You changed the value of len = iv_off + max_iv_len + metadata_size,
> > > but metadata is copied after this 'len'. See this
> > > rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
> > Op_mpool is created with element of priv_size = DEFAULT_NUM_XFORMS
> *
> > sizeof(struct rte_crypto_sym_xform) + MAXIMUM_IV_LENGTH.
> > Whereas for the "sessionless" length is set to " uint32_t len =
> > IV_OFFSET + MAXIMUM_IV_LENGTH + (sizeof(struct
> rte_crypto_sym_xform) * 2)"
> > Whereas, IV_OFFSET  = (sizeof(struct rte_crypto_op) + sizeof(struct
> > rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> > rte_crypto_sym_xform)).
> >
> > So substituting IV_OFFSET, len = (sizeof(struct rte_crypto_op) +
> > sizeof(struct
> > rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> > rte_crypto_sym_xform)) + MAXIMUM_IV_LENGTH + (sizeof(struct
> > rte_crypto_sym_xform) * 2).
> > Which is a way ahead of the boundary which causes buffer overflow.
> >
> > When memcpy is executed -> rte_memcpy((uint8_t *)op + len, &m_data,
> > sizeof(m_data)); The m_data will overwrite the beyond the boundary.
> > Hope this clarifies the need for fix.
> 
> You are setting len = sizeof(rte_crypto_op) + sizeof(rte_crypto_sym_op) + 2
> *(sizeof(xform)) + IV_LEN + m_data_len And then copying mdata at end of
> 'len', which is not correct. Here, len already include mdata and you are
> copying mdata after its designated space. Right?
> IMO, len should be set as IV_OFFSET+IV_LEN only.
Agree. I will update the changes in the next patch.

> 
> > >
> > > I do not agree with this patch, am I missing something?
> > >
> > > >  		/* Fill in private data information */
> > > >  		rte_memcpy(&m_data.response_info, &response_info, @@
> > -
> > > 424,7 +424,7
> > > > @@ test_op_new_mode(uint8_t session_less)
> > > >  		first_xform = &cipher_xform;
> > > >  		sym_op->xform = first_xform;
> > > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > > -				(sizeof(struct rte_crypto_sym_xform) * 2);
> > > > +				(sizeof(union rte_event_crypto_metadata));
> > > >  		op->private_data_offset = len;
> > > >  		/* Fill in private data information */
> > > >  		rte_memcpy(&m_data.response_info, &response_info,
> > > > --
> > > > 2.25.1
Gujjar, Abhinandan S July 18, 2021, 9:22 a.m. UTC | #9 
Hi Akhil,

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Sunday, July 18, 2021 2:36 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>
> Cc: Power, Ciara <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless case
> 
> Hi Akhil,
> 
> > -----Original Message-----
> > From: Akhil Goyal <gakhil@marvell.com>
> > Sent: Tuesday, July 13, 2021 2:42 PM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> > Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> > Cc: Power, Ciara <ciara.power@intel.com>
> > Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless
> > case
> >
> > Hi Abhinandan,
> > > >
> > > > > Currently, private_data_offset for the sessionless is computed
> > > > > wrongly which includes extra bytes added because of using
> > > > > sizeof(struct
> > > > > rte_crypto_sym_xform) * 2) instead of (sizeof(union
> > > > > rte_event_crypto_metadata)). Due to this buffer overflow, the
> > > > > corruption was leading to test application crash while freeing
> > > > > the ops mempool.
> > > > >
> > > > > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > > > > Reported-by: ciara.power@intel.com
> > > > >
> > > > > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > > > > ---
> > > > >  app/test/test_event_crypto_adapter.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/app/test/test_event_crypto_adapter.c
> > > > > b/app/test/test_event_crypto_adapter.c
> > > > > index f689bc1f2..688ac0b2f 100644
> > > > > --- a/app/test/test_event_crypto_adapter.c
> > > > > +++ b/app/test/test_event_crypto_adapter.c
> > > > > @@ -229,7 +229,7 @@ test_op_forward_mode(uint8_t session_less)
> > > > >  		first_xform = &cipher_xform;
> > > > >  		sym_op->xform = first_xform;
> > > > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > > > -				(sizeof(struct
> rte_crypto_sym_xform) * 2);
> > > > > +				(sizeof(union
> rte_event_crypto_metadata));
> > > > >  		op->private_data_offset = len;
> > > > I do not understand the need for this patch.
> > > This is patch provide fix for segfault at the end of
> > > event_crypto_adapter_autotest()
> > > RTE>>event_crypto_adapter_autotest
> > >  + ------------------------------------------------------- +  + Test
> > > Suite : Event crypto adapter test suite
> > > CRYPTODEV: Creating cryptodev crypto_nullCRYPTODEV: Initialisation
> > > parameters - name: crypto_null,socket id: 0, max queue pairs: 8
> > > CRYPTODEV: elt_size 0 is expanded to 336 +
> > > -------------------------------------------
> > > ------------ +
> > >  + TestCase [ 0] : test_crypto_adapter_create succeeded  + TestCase
> > > [ 1] : test_crypto_adapter_qp_add_del succeeded
> > > +------------------------------------------------------+
> > >  + Crypto adapter stats for instance 0:
> > >  + Event port poll count          0
> > >  + Event dequeue count            0
> > >  + Cryptodev enqueue count        0
> > >  + Cryptodev enqueue failed count 0
> > >  + Cryptodev dequeue count        0
> > >  + Event enqueue count            0
> > >  + Event enqueue retry count      0
> > >  + Event enqueue fail count       0
> > >  +------------------------------------------------------+
> > >  + TestCase [ 2] : test_crypto_adapter_stats succeeded Segmentation
> > > fault (core dumped)
> > >
> > > > Event metadata is copied after private data offset, and this patch
> > > > is
> > > changing
> > > > the offset value.
> > > >
> > > > You changed the value of len = iv_off + max_iv_len +
> > > > metadata_size, but metadata is copied after this 'len'. See this
> > > > rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
> > > Op_mpool is created with element of priv_size =
> DEFAULT_NUM_XFORMS
> > *
> > > sizeof(struct rte_crypto_sym_xform) + MAXIMUM_IV_LENGTH.
> > > Whereas for the "sessionless" length is set to " uint32_t len =
> > > IV_OFFSET + MAXIMUM_IV_LENGTH + (sizeof(struct
> > rte_crypto_sym_xform) * 2)"
> > > Whereas, IV_OFFSET  = (sizeof(struct rte_crypto_op) + sizeof(struct
> > > rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> > > rte_crypto_sym_xform)).
> > >
> > > So substituting IV_OFFSET, len = (sizeof(struct rte_crypto_op) +
> > > sizeof(struct
> > > rte_crypto_sym_op) + DEFAULT_NUM_XFORMS * sizeof(struct
> > > rte_crypto_sym_xform)) + MAXIMUM_IV_LENGTH + (sizeof(struct
> > > rte_crypto_sym_xform) * 2).
> > > Which is a way ahead of the boundary which causes buffer overflow.
> > >
> > > When memcpy is executed -> rte_memcpy((uint8_t *)op + len,
> &m_data,
> > > sizeof(m_data)); The m_data will overwrite the beyond the boundary.
> > > Hope this clarifies the need for fix.
> >
> > You are setting len = sizeof(rte_crypto_op) +
> > sizeof(rte_crypto_sym_op) + 2
> > *(sizeof(xform)) + IV_LEN + m_data_len And then copying mdata at end
> > of 'len', which is not correct. Here, len already include mdata and
> > you are copying mdata after its designated space. Right?
> > IMO, len should be set as IV_OFFSET+IV_LEN only.
> Agree. I will update the changes in the next patch.
Along with above changes, ops mempool has to be updated with sizeof(union rte_event_crypto_metadata) as below: 

	params.op_mpool = rte_crypto_op_pool_create(
			"EVENT_CRYPTO_SYM_OP_POOL",
			RTE_CRYPTO_OP_TYPE_SYMMETRIC,
			NUM_MBUFS, MBUF_CACHE_SIZE,
			DEFAULT_NUM_XFORMS *
			sizeof(struct rte_crypto_sym_xform) +
			MAXIMUM_IV_LENGTH +
+			sizeof(union rte_event_crypto_metadata),
			rte_socket_id());

Do you agree?

> 
> >
> > > >
> > > > I do not agree with this patch, am I missing something?
> > > >
> > > > >  		/* Fill in private data information */
> > > > >  		rte_memcpy(&m_data.response_info,
> &response_info, @@
> > > -
> > > > 424,7 +424,7
> > > > > @@ test_op_new_mode(uint8_t session_less)
> > > > >  		first_xform = &cipher_xform;
> > > > >  		sym_op->xform = first_xform;
> > > > >  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
> > > > > -				(sizeof(struct
> rte_crypto_sym_xform) * 2);
> > > > > +				(sizeof(union
> rte_event_crypto_metadata));
> > > > >  		op->private_data_offset = len;
> > > > >  		/* Fill in private data information */
> > > > >  		rte_memcpy(&m_data.response_info,
> &response_info,
> > > > > --
> > > > > 2.25.1
Akhil Goyal July 18, 2021, 9:25 a.m. UTC | #10 
> Along with above changes, ops mempool has to be updated with
> sizeof(union rte_event_crypto_metadata) as below:
> 
> 	params.op_mpool = rte_crypto_op_pool_create(
> 			"EVENT_CRYPTO_SYM_OP_POOL",
> 			RTE_CRYPTO_OP_TYPE_SYMMETRIC,
> 			NUM_MBUFS, MBUF_CACHE_SIZE,
> 			DEFAULT_NUM_XFORMS *
> 			sizeof(struct rte_crypto_sym_xform) +
> 			MAXIMUM_IV_LENGTH +
> +			sizeof(union rte_event_crypto_metadata),
> 			rte_socket_id());
> 
> Do you agree?
Ack
diff mbox series

Patch

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index f689bc1f2..688ac0b2f 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -229,7 +229,7 @@  test_op_forward_mode(uint8_t session_less)
 		first_xform = &cipher_xform;
 		sym_op->xform = first_xform;
 		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
-				(sizeof(struct rte_crypto_sym_xform) * 2);
+				(sizeof(union rte_event_crypto_metadata));
 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info, &response_info,
@@ -424,7 +424,7 @@  test_op_new_mode(uint8_t session_less)
 		first_xform = &cipher_xform;
 		sym_op->xform = first_xform;
 		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH +
-				(sizeof(struct rte_crypto_sym_xform) * 2);
+				(sizeof(union rte_event_crypto_metadata));
 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info, &response_info,