Hi Akhil,

-----Original Message-----
From: Gujjar, Abhinandan S
Sent: Sunday, July 18, 2021 2:36 PM
To: Akhil Goyal <redacted>; dev@dpdk.org; Jerin Jacob
Kollanukkaran [off-list ref]
Cc: Power, Ciara <redacted>
Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless case

Hi Akhil,

quoted

-----Original Message-----
From: Akhil Goyal <redacted>
Sent: Tuesday, July 13, 2021 2:42 PM
To: Gujjar, Abhinandan S <redacted>; dev@dpdk.org;
Jerin Jacob Kollanukkaran [off-list ref]
Cc: Power, Ciara <redacted>
Subject: RE: [EXT] [PATCH] test: fix crypto_op length for sessionless
case

Hi Abhinandan,

quoted

quoted

quoted

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 <redacted>
---
 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);

quoted

quoted

quoted

quoted

+				(sizeof(union

rte_event_crypto_metadata));

quoted

quoted

quoted

quoted

 		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)

quoted

Event metadata is copied after private data offset, and this patch
is

changing

quoted

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

quoted

*

quoted

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

quoted

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,

quoted

quoted

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?

quoted

quoted

quoted

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

quoted

 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info,

&response_info, @@

quoted

quoted

-

quoted

424,7 +424,7

quoted

@@ 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);

quoted

quoted

quoted

quoted

+				(sizeof(union

rte_event_crypto_metadata));

quoted

quoted

quoted

quoted

 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info,

&response_info,

quoted

quoted

quoted

quoted

--
2.25.1