DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>
To: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
	"Jerin Jacob Kollanukkaran" <jerinj@marvell.com>
Cc: "Power, Ciara" <ciara.power@intel.com>
Subject: Re: [dpdk-dev] [EXT] [PATCH] test: fix crypto_op length for sessionless case
Date: Thu, 8 Jul 2021 14:12:52 +0000	[thread overview]
Message-ID: <PH0PR11MB4824096972F14B2F18B89205E8199@PH0PR11MB4824.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB4484E1023B25EBDFCC16119DD81A9@CO6PR18MB4484.namprd18.prod.outlook.com>

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


  reply	other threads:[~2021-07-08 14:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-30 12:46 [dpdk-dev] " Abhinandan Gujjar
2021-07-02 17:08 ` Gujjar, Abhinandan S
2021-07-02 23:26   ` Ferruh Yigit
2021-07-05  6:30     ` Gujjar, Abhinandan S
2021-07-06 16:09       ` Brandon Lo
2021-07-07 14:07 ` [dpdk-dev] [EXT] " Akhil Goyal
2021-07-08 14:12   ` Gujjar, Abhinandan S [this message]
2021-07-13  9:11     ` Akhil Goyal
2021-07-18  9:05       ` Gujjar, Abhinandan S
2021-07-18  9:22         ` Gujjar, Abhinandan S
2021-07-18  9:25           ` Akhil Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR11MB4824096972F14B2F18B89205E8199@PH0PR11MB4824.namprd11.prod.outlook.com \
    --to=abhinandan.gujjar@intel.com \
    --cc=ciara.power@intel.com \
    --cc=dev@dpdk.org \
    --cc=gakhil@marvell.com \
    --cc=jerinj@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).