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

Hi Abhinandan,

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);
+				(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)

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 *
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.

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

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);
+				(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