Re: [dpdk-dev] [EXT] [PATCH v3 04/11] test: add cipher field to RSA test

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Shally, Arek,

> > > >
> > > > > -----Original Message-----
> > > > > From: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > > > Sent: Wednesday, July 17, 2019 12:23 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: akhil.goyal@nxp.com; fiona.trahe@intel.com; Shally Verma
> > > > > <shallyv@marvell.com>; Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > > > Subject: [EXT] [PATCH v3 04/11] test: add cipher field to RSA test
> > > > >
> > > > > External Email
> > > > >
> > > > > ------------------------------------------------------------------
> > > > > --
> > > > > -- This patch adds cipher field to RSA test cases
> > > > >
> > > > > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > > > ---
> > > > >  app/test/test_cryptodev_asym.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/app/test/test_cryptodev_asym.c
> > > > > b/app/test/test_cryptodev_asym.c index 4dee164..8391545 100644
> > > > > --- a/app/test/test_cryptodev_asym.c
> > > > > +++ b/app/test/test_cryptodev_asym.c
> > > > > @@ -164,6 +164,7 @@ queue_ops_rsa_enc_dec(struct
> > > > > rte_cryptodev_asym_session *sess)
> > > > >  	uint8_t dev_id = ts_params->valid_devs[0];
> > > > >  	struct rte_crypto_op *op, *result_op;
> > > > >  	struct rte_crypto_asym_op *asym_op;
> > > > > +	uint8_t cipher_buf[TEST_DATA_SIZE] = {0};
> > > > >  	int ret, status = TEST_SUCCESS;
> > > > >
> > > > >  	/* Set up crypto op data structure */ @@ -180,6 +181,8 @@
> > > > > queue_ops_rsa_enc_dec(struct rte_cryptodev_asym_session *sess)
> > > > >  	asym_op->rsa.op_type = RTE_CRYPTO_ASYM_OP_ENCRYPT;
> > > > >
> > > > >  	asym_op->rsa.message.data = rsaplaintext.data;
> > > > > +	asym_op->rsa.cipher.data = cipher_buf;
> > > > > +	asym_op->rsa.cipher.length = 0;
> > > > [Shally] I think this should be initialized to length of buffer
> > > > available i.e. RSA Key size? PMD can override it with length of
> > > > actual data written at output, which has to be less than , equal to
> > RSA_key size.
> > > [AK] - its because API comments are ambiguous in this case and we have
> > > only one field describing array length.
> > > I would suggest to rephrase cipher field API comments from "length in
> > bytes
> > > 	 * of this field needs to be greater or equal to the length of
> > > 	 * corresponding RSA key in bytes"
> > > To "underlying array should have allocated enough memory to hold
> > > cipher output (bigger or equal to RSA key size". Then length could and
> > > I think should be zero or unspecified at this point.
> > > What do you think?
> >
> > [AK2] Something like that:
> > 	 * When RTE_CRYPTO_ASYM_OP_ENCRYPT op_type used underlying
> > array
> > 	 * should have been allocated with enough memory to hold cipher
> > 	 * output (bigger or equal to RSA key size).
> > The same for message field.
> [Shally] This description is okay. But still I would assume app to set length field of cipher buffer to actual
> allocated than 0. But I look forward to more feedback on this from others
[Fiona] I think the important thing is to be clear on when it's an input field and when an output and what the appl or PMD does in each case.
So my understanding is in ENCRYPT case it's an output field and DECRYPT it's an input.
SO how about - combining this with the changes already suggested to avoid repetition in patch 2:
Comment under rte_crypto_rsa_op_param.message:
Pointer to input data
 	 * - to be encrypted for RSA public encrypt.
 	 * - to be signed for RSA sign generation.
 	 * - to be authenticated for RSA sign verification.
Pointer to output data
               * - for RSA private decrypt.
                     In this case the underlying array should have been allocated with
                     enough memory to hold plaintext output (i.e. must be at least RSA key size).
                     The message.length field should be 0 and will be overwritten by the PMD
                     with the decrypted length.
All data is in Octet-string network byte order format.

Note 1: If API allows a length on decrypt, then what would the PMD use it for? Would it have to handle the case where it's less than key-size? In which case the appl is breaking the API and ignoring the previous comment. Or more than key-size - what does the PMD care - it just needs key-size. IF there was a case where PMD could produce more than keysize and would need to know if the buffer is big enough then we should allow this and say it's both an input (buffer-len) and an output (decrypted-message-len). But I don't think there's such a case. 

Note 2 : it's good practice for apps to zero all fields in all API structs, except those explicitly set, to allow for future API extensions without ABI breakage.

Comment under rte_crypto_rsa_op_param.cipher:
Pointer to input data
 	 * - to be decrypted for RSA private decrypt.
 	 
Pointer to output data
               * - for RSA public encrypt.
                     In this case the underlying array should have been allocated with
                     enough memory to hold ciphertext output (i.e. must be at least RSA key size).
                     The message.length field should be 0 and will be overwritten by the PMD
                     with the encrypted length.
All data is in Octet-string network byte order format.

@Shally - does above make sense?
If so we can update patches 2, 3 and 4 based on above.