Re: [dpdk-dev] [EXT] [PATCH] test: fix crypto_op length for sessionless case

[Akhil Goyal <gakhil@marvell.com>]

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