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

["Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>]

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