[dpdk-dev] [PATCH v1 0/2] Changes for RSA verify operation in OpenSSL PMD and unit tests

[dpdk-dev] [PATCH v1 0/2] Changes for RSA verify operation in OpenSSL PMD and unit tests

[Akash Saxena]

In lib cryptodev, RSA verify operation inputs plain message text and
corresponding signature and expected to return 
RTE_CRYPTO_OP_STATUS_SUCCESS/FAILURE on a signature match/mismatch. 
Current OpenSSL PMD RSA verify implementation overrides application passed
sign input by decrypted output which isn't expected.

This patch addresses this issue in OpenSSL PMD. Now, OpenSSL PMD use tmp 
buffer to store sign operation decrypted output and test application to 
only check for STATUS_SUCCESS/FAILURE.

Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
Signed-off-by: Akash Saxena <akash.saxena@caviumnetworks.com>
Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>

---
Akash Saxena (2):
  crypto/openssl: changes for RSA verify operation
  test/crypto: check for operation status only at output of RSA verify
    crypto operation

 drivers/crypto/openssl/rte_openssl_pmd.c | 20 +++++++++++++-------
 test/test/test_cryptodev_asym.c          |  9 ++++++---
 2 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v1 1/2] crypto/openssl: changes for RSA verify operation

[Akash Saxena]

Add tmp buffer to pass to OpenSSL sign API and memcmp output with
original plain text to verify signature match.
Set op->status = RTE_CRYPO_OP_STATUS_ERROR on signature mismatch.

Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
Signed-off-by: Akash Saxena <akash.saxena@caviumnetworks.com>
Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
---
 drivers/crypto/openssl/rte_openssl_pmd.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 003116d..cc33fca 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -1843,6 +1843,9 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
 	struct rte_crypto_asym_op *op = cop->asym;
 	RSA *rsa = sess->u.r.rsa;
 	uint32_t pad = (op->rsa.pad);
+	uint8_t *tmp;
+
+	cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
 
 	switch (pad) {
 	case RTE_CRYPTO_RSA_PKCS1_V1_5_BT0:
@@ -1895,9 +1898,10 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
 		break;
 
 	case RTE_CRYPTO_ASYM_OP_VERIFY:
+		tmp = rte_malloc(NULL, op->rsa.sign.length, 0);
 		ret = RSA_public_decrypt(op->rsa.sign.length,
 				op->rsa.sign.data,
-				op->rsa.sign.data,
+				tmp,
 				rsa,
 				pad);
 
@@ -1905,13 +1909,15 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
 				"Length of public_decrypt %d "
 				"length of message %zd\n",
 				ret, op->rsa.message.length);
-
-		if (memcmp(op->rsa.sign.data, op->rsa.message.data,
-					op->rsa.message.length)) {
-			OPENSSL_LOG(ERR,
-					"RSA sign Verification failed");
-			return -1;
+		if (ret > 0) {
+			if (memcmp(tmp, op->rsa.message.data,
+						op->rsa.message.length)) {
+				OPENSSL_LOG(ERR,
+						"RSA sign Verification failed");
+				cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
+			}
 		}
+		rte_free(tmp);
 		break;
 
 	default:
-- 
2.7.4

[dpdk-dev] [PATCH v1 2/2] test/crypto: remove data verification at rsa verify operation

[Akash Saxena]

Change unit test app to check only for op->status =
RTE_CRYPTO_OP_STATUS_SUCCESS/ERROR instead of calling rsa_verify().

Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
---
 test/test/test_cryptodev_asym.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/test/test/test_cryptodev_asym.c b/test/test/test_cryptodev_asym.c
index 2fdfc1d..a899f99 100644
--- a/test/test/test_cryptodev_asym.c
+++ b/test/test/test_cryptodev_asym.c
@@ -153,10 +153,13 @@ test_rsa_sign_verify(void)
 		goto error_exit;
 	}
 	status = TEST_SUCCESS;
-	int ret = 0;
-	ret = rsa_verify(&rsaplaintext, result_op);
-	if (ret)
+	if (result_op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
+		RTE_LOG(ERR, USER1,
+				"line %u FAILED: %s",
+				__LINE__, "Failed to process asym crypto op");
 		status = TEST_FAILED;
+		goto error_exit;
+	}
 
 error_exit:
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v1 1/2] crypto/openssl: changes for RSA verify operation

[Akhil Goyal]

On 10/11/2018 7:13 PM, Akash Saxena wrote:
> Add tmp buffer to pass to OpenSSL sign API and memcmp output with
> original plain text to verify signature match.
> Set op->status = RTE_CRYPO_OP_STATUS_ERROR on signature mismatch.
>
> Signed-off-by: Ayuj Verma<ayuj.verma@caviumnetworks.com>
> Signed-off-by: Akash Saxena<akash.saxena@caviumnetworks.com>
> Signed-off-by: Shally Verma<shally.verma@caviumnetworks.com>
> ---
>   drivers/crypto/openssl/rte_openssl_pmd.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Re: [dpdk-dev] [PATCH v1 1/2] crypto/openssl: changes for RSA verify operation

[Akhil Goyal]

On 10/11/2018 7:13 PM, Akash Saxena wrote:
> Add tmp buffer to pass to OpenSSL sign API and memcmp output with
> original plain text to verify signature match.
> Set op->status = RTE_CRYPO_OP_STATUS_ERROR on signature mismatch.
>
> Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
> Signed-off-by: Akash Saxena <akash.saxena@caviumnetworks.com>
> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
> ---
>   drivers/crypto/openssl/rte_openssl_pmd.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
> index 003116d..cc33fca 100644
> --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> @@ -1843,6 +1843,9 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
>   	struct rte_crypto_asym_op *op = cop->asym;
>   	RSA *rsa = sess->u.r.rsa;
>   	uint32_t pad = (op->rsa.pad);
> +	uint8_t *tmp;
> +
> +	cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
>   
>   	switch (pad) {
>   	case RTE_CRYPTO_RSA_PKCS1_V1_5_BT0:
> @@ -1895,9 +1898,10 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
>   		break;
>   
>   	case RTE_CRYPTO_ASYM_OP_VERIFY:
> +		tmp = rte_malloc(NULL, op->rsa.sign.length, 0);
>   		ret = RSA_public_decrypt(op->rsa.sign.length,
>   				op->rsa.sign.data,
> -				op->rsa.sign.data,
> +				tmp,
>   				rsa,
>   				pad);
Null checking of tmp is not done if rte_malloc fails. Please correct it.
>   
> @@ -1905,13 +1909,15 @@ process_openssl_rsa_op(struct rte_crypto_op *cop,
>   				"Length of public_decrypt %d "
>   				"length of message %zd\n",
>   				ret, op->rsa.message.length);
> -
> -		if (memcmp(op->rsa.sign.data, op->rsa.message.data,
> -					op->rsa.message.length)) {
> -			OPENSSL_LOG(ERR,
> -					"RSA sign Verification failed");
> -			return -1;
> +		if (ret > 0) {
> +			if (memcmp(tmp, op->rsa.message.data,
> +						op->rsa.message.length)) {
> +				OPENSSL_LOG(ERR,
> +						"RSA sign Verification failed");
> +				cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> +			}
>   		}
> +		rte_free(tmp);
>   		break;
>   
>   	default:

Re: [dpdk-dev] [PATCH v1 2/2] test/crypto: remove data verification at rsa verify operation

[Akhil Goyal]

On 10/11/2018 7:13 PM, Akash Saxena wrote:
> Change unit test app to check only for op->status =
> RTE_CRYPTO_OP_STATUS_SUCCESS/ERROR instead of calling rsa_verify().
>
> Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
> ---
>   test/test/test_cryptodev_asym.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/test/test/test_cryptodev_asym.c b/test/test/test_cryptodev_asym.c
> index 2fdfc1d..a899f99 100644
> --- a/test/test/test_cryptodev_asym.c
> +++ b/test/test/test_cryptodev_asym.c
> @@ -153,10 +153,13 @@ test_rsa_sign_verify(void)
>   		goto error_exit;
>   	}
>   	status = TEST_SUCCESS;
> -	int ret = 0;
> -	ret = rsa_verify(&rsaplaintext, result_op);
> -	if (ret)
> +	if (result_op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> +		RTE_LOG(ERR, USER1,
> +				"line %u FAILED: %s",
> +				__LINE__, "Failed to process asym crypto op");
>   		status = TEST_FAILED;
> +		goto error_exit;
> +	}
>   
>   error_exit:
>   
What about test_rsa_enc_dec? Do you need to remove verify from that as well?

Re: [dpdk-dev] [PATCH v1 2/2] test/crypto: remove data verification at rsa verify operation

[Verma, Shally]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Akhil Goyal
>Sent: 16 October 2018 18:51
>To: Saxena, Akash <Akash.Saxena@cavium.com>; pablo.de.lara.guarch@intel.com
>Cc: dev@dpdk.org; Verma, Shally <Shally.Verma@cavium.com>; Verma, Ayuj <Ayuj.Verma@cavium.com>
>Subject: Re: [dpdk-dev] [PATCH v1 2/2] test/crypto: remove data verification at rsa verify operation
>
>External Email
>
>On 10/11/2018 7:13 PM, Akash Saxena wrote:
>> Change unit test app to check only for op->status =
>> RTE_CRYPTO_OP_STATUS_SUCCESS/ERROR instead of calling rsa_verify().
>>
>> Signed-off-by: Ayuj Verma <ayuj.verma@caviumnetworks.com>
>> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
>> ---
>>   test/test/test_cryptodev_asym.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/test/test/test_cryptodev_asym.c b/test/test/test_cryptodev_asym.c
>> index 2fdfc1d..a899f99 100644
>> --- a/test/test/test_cryptodev_asym.c
>> +++ b/test/test/test_cryptodev_asym.c
>> @@ -153,10 +153,13 @@ test_rsa_sign_verify(void)
>>               goto error_exit;
>>       }
>>       status = TEST_SUCCESS;
>> -     int ret = 0;
>> -     ret = rsa_verify(&rsaplaintext, result_op);
>> -     if (ret)
>> +     if (result_op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
>> +             RTE_LOG(ERR, USER1,
>> +                             "line %u FAILED: %s",
>> +                             __LINE__, "Failed to process asym crypto op");
>>               status = TEST_FAILED;
>> +             goto error_exit;
>> +     }
>>
>>   error_exit:
>>
>What about test_rsa_enc_dec? Do you need to remove verify from that as well?
No. in case of enc_dec, PMD just return decrypted o/p so, app need to do cross-verify data.
In case of RSA_VERIFY OP PMD internally does memcmp of plaintext to decrypted o/p and ensure
Pass / fail. So, doing it again at app level is redundant.

Thanks
Shally

Re: [dpdk-dev] [PATCH v1 2/2] test/crypto: remove data verification at rsa verify operation

[Akhil Goyal]

>>> @@ -153,10 +153,13 @@ test_rsa_sign_verify(void)
>>>                goto error_exit;
>>>        }
>>>        status = TEST_SUCCESS;
>>> -     int ret = 0;
>>> -     ret = rsa_verify(&rsaplaintext, result_op);
>>> -     if (ret)
>>> +     if (result_op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
>>> +             RTE_LOG(ERR, USER1,
>>> +                             "line %u FAILED: %s",
>>> +                             __LINE__, "Failed to process asym crypto op");
>>>                status = TEST_FAILED;
>>> +             goto error_exit;
>>> +     }
>>>
>>>    error_exit:
>>>
>> What about test_rsa_enc_dec? Do you need to remove verify from that as well?
> No. in case of enc_dec, PMD just return decrypted o/p so, app need to do cross-verify data.
> In case of RSA_VERIFY OP PMD internally does memcmp of plaintext to decrypted o/p and ensure
> Pass / fail. So, doing it again at app level is redundant.
Ok
> Thanks
> Shally