[dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler error

[dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler error

[Tomasz Jozwiak]

This commit fixes
  - bpi_cipher_encrypt to prevent before 'array subscript is
    above array bounds' error
  - bpi_cipher_decrypt to prevent before 'array subscript is
    above array bounds' error
  - right cast from qat_cipher_get_block_size function.
    This function can return -EFAULT in case of any error,
    and that value must be cast to int instead of uint8_t
  - typo in error message

A performance improvement was added to bpi_cipher_encrypt and
bpi_cipher_decrypt as well.

Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")

Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
---
 drivers/crypto/qat/qat_crypto.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index a572967..c63697e 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -69,6 +69,10 @@
 #include "adf_transport_access_macros.h"
 
 #define BYTE_LENGTH    8
+/* bpi is only used for partial blocks of DES and AES
+ * so AES block len can be assumed as max len for iv, src and dst
+ */
+#define BPI_MAX_ENCR_IV_LEN ICP_QAT_HW_AES_BLK_SZ
 
 static int
 qat_is_cipher_alg_supported(enum rte_crypto_cipher_algorithm algo,
@@ -121,16 +125,17 @@ bpi_cipher_encrypt(uint8_t *src, uint8_t *dst,
 {
 	EVP_CIPHER_CTX *ctx = (EVP_CIPHER_CTX *)bpi_ctx;
 	int encrypted_ivlen;
-	uint8_t encrypted_iv[16];
-	int i;
+	uint8_t encrypted_iv[BPI_MAX_ENCR_IV_LEN];
+
+	register uint8_t *encr = encrypted_iv;
 
 	/* ECB method: encrypt the IV, then XOR this with plaintext */
 	if (EVP_EncryptUpdate(ctx, encrypted_iv, &encrypted_ivlen, iv, ivlen)
 								<= 0)
 		goto cipher_encrypt_err;
 
-	for (i = 0; i < srclen; i++)
-		*(dst+i) = *(src+i)^(encrypted_iv[i]);
+	for (; srclen != 0; --srclen, ++dst, ++src, ++encr)
+		*dst = *src ^ *encr;
 
 	return 0;
 
@@ -150,21 +155,22 @@ bpi_cipher_decrypt(uint8_t *src, uint8_t *dst,
 {
 	EVP_CIPHER_CTX *ctx = (EVP_CIPHER_CTX *)bpi_ctx;
 	int encrypted_ivlen;
-	uint8_t encrypted_iv[16];
-	int i;
+	uint8_t encrypted_iv[BPI_MAX_ENCR_IV_LEN];
+
+	register uint8_t *encr = encrypted_iv;
 
 	/* ECB method: encrypt (not decrypt!) the IV, then XOR with plaintext */
 	if (EVP_EncryptUpdate(ctx, encrypted_iv, &encrypted_ivlen, iv, ivlen)
 								<= 0)
 		goto cipher_decrypt_err;
 
-	for (i = 0; i < srclen; i++)
-		*(dst+i) = *(src+i)^(encrypted_iv[i]);
+	for (; srclen != 0; --srclen, ++dst, ++src, ++encr)
+		*dst = *src ^ *encr;
 
 	return 0;
 
 cipher_decrypt_err:
-	PMD_DRV_LOG(ERR, "libcrypto ECB cipher encrypt for BPI IV failed");
+	PMD_DRV_LOG(ERR, "libcrypto ECB cipher decrypt for BPI IV failed");
 	return -EINVAL;
 }
 
@@ -844,7 +850,7 @@ static inline uint32_t
 qat_bpicipher_preprocess(struct qat_session *ctx,
 				struct rte_crypto_op *op)
 {
-	uint8_t block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
+	int block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
 	struct rte_crypto_sym_op *sym_op = op->sym;
 	uint8_t last_block_len = block_len > 0 ?
 			sym_op->cipher.data.length % block_len : 0;
@@ -899,7 +905,7 @@ static inline uint32_t
 qat_bpicipher_postprocess(struct qat_session *ctx,
 				struct rte_crypto_op *op)
 {
-	uint8_t block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
+	int block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
 	struct rte_crypto_sym_op *sym_op = op->sym;
 	uint8_t last_block_len = block_len > 0 ?
 			sym_op->cipher.data.length % block_len : 0;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler error

[Trahe, Fiona]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Jozwiak
> Sent: Monday, January 15, 2018 1:53 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Subject: [dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler error
> 
> This commit fixes
>   - bpi_cipher_encrypt to prevent before 'array subscript is
>     above array bounds' error
>   - bpi_cipher_decrypt to prevent before 'array subscript is
>     above array bounds' error
>   - right cast from qat_cipher_get_block_size function.
>     This function can return -EFAULT in case of any error,
>     and that value must be cast to int instead of uint8_t
>   - typo in error message
> 
> A performance improvement was added to bpi_cipher_encrypt and
> bpi_cipher_decrypt as well.
> 
> Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")
> 
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>

Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Re: [dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler error

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, January 15, 2018 2:31 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; Trahe,
> Fiona <fiona.trahe@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler
> error
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Jozwiak
> > Sent: Monday, January 15, 2018 1:53 PM
> > To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Cc: dev@dpdk.org; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> > Subject: [dpdk-dev] [PATCH] crypto/qat: fix out-of-bounds compiler
> > error
> >
> > This commit fixes
> >   - bpi_cipher_encrypt to prevent before 'array subscript is
> >     above array bounds' error
> >   - bpi_cipher_decrypt to prevent before 'array subscript is
> >     above array bounds' error
> >   - right cast from qat_cipher_get_block_size function.
> >     This function can return -EFAULT in case of any error,
> >     and that value must be cast to int instead of uint8_t
> >   - typo in error message
> >
> > A performance improvement was added to bpi_cipher_encrypt and
> > bpi_cipher_decrypt as well.
> >
> > Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")
> >
> > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> 
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Hi Tomasz,

>From what I see in this patch, it is making various changes,
which are not related (such as the typo in the error message).
Could you split the patch into several ones? You can leave the ack from Fiona,
and make sure that the patch that fixes the out-of-bounds problem also CC stable@dpdk.org.

Thanks,
Pablo

[dpdk-dev] [PATCH v2 0/3] Fixes for QAT PMD

[Tomasz Jozwiak]

v2:
- Previous patch has been split into 3 as below:

Tomasz Jozwiak (3):
  crypto/qat: fix out-of-bounds compiler error
  crypto/qat: fix typo in error message
  crypto/qat: fix parameter type

 drivers/crypto/qat/qat_crypto.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/3] crypto/qat: fix out-of-bounds compiler error

[Tomasz Jozwiak]

This commit fixes
  - bpi_cipher_encrypt to prevent before 'array subscript is
    above array bounds' error
  - bpi_cipher_decrypt to prevent before 'array subscript is
    above array bounds' error

Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")
Cc: stable@dpdk.org

Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_crypto.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index a572967..f85c2c8 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -69,6 +69,10 @@
 #include "adf_transport_access_macros.h"
 
 #define BYTE_LENGTH    8
+/* bpi is only used for partial blocks of DES and AES
+ * so AES block len can be assumed as max len for iv, src and dst
+ */
+#define BPI_MAX_ENCR_IV_LEN ICP_QAT_HW_AES_BLK_SZ
 
 static int
 qat_is_cipher_alg_supported(enum rte_crypto_cipher_algorithm algo,
@@ -121,16 +125,16 @@ bpi_cipher_encrypt(uint8_t *src, uint8_t *dst,
 {
 	EVP_CIPHER_CTX *ctx = (EVP_CIPHER_CTX *)bpi_ctx;
 	int encrypted_ivlen;
-	uint8_t encrypted_iv[16];
-	int i;
+	uint8_t encrypted_iv[BPI_MAX_ENCR_IV_LEN];
+	uint8_t *encr = encrypted_iv;
 
 	/* ECB method: encrypt the IV, then XOR this with plaintext */
 	if (EVP_EncryptUpdate(ctx, encrypted_iv, &encrypted_ivlen, iv, ivlen)
 								<= 0)
 		goto cipher_encrypt_err;
 
-	for (i = 0; i < srclen; i++)
-		*(dst+i) = *(src+i)^(encrypted_iv[i]);
+	for (; srclen != 0; --srclen, ++dst, ++src, ++encr)
+		*dst = *src ^ *encr;
 
 	return 0;
 
@@ -150,16 +154,16 @@ bpi_cipher_decrypt(uint8_t *src, uint8_t *dst,
 {
 	EVP_CIPHER_CTX *ctx = (EVP_CIPHER_CTX *)bpi_ctx;
 	int encrypted_ivlen;
-	uint8_t encrypted_iv[16];
-	int i;
+	uint8_t encrypted_iv[BPI_MAX_ENCR_IV_LEN];
+	uint8_t *encr = encrypted_iv;
 
 	/* ECB method: encrypt (not decrypt!) the IV, then XOR with plaintext */
 	if (EVP_EncryptUpdate(ctx, encrypted_iv, &encrypted_ivlen, iv, ivlen)
 								<= 0)
 		goto cipher_decrypt_err;
 
-	for (i = 0; i < srclen; i++)
-		*(dst+i) = *(src+i)^(encrypted_iv[i]);
+	for (; srclen != 0; --srclen, ++dst, ++src, ++encr)
+		*dst = *src ^ *encr;
 
 	return 0;
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/3] crypto/qat: fix typo in error message

[Tomasz Jozwiak]

This commit fixes typo in bpi_cipher_decrypt error message

Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")
Cc: stable@dpdk.org

Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index f85c2c8..7b577b9 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -168,7 +168,7 @@ bpi_cipher_decrypt(uint8_t *src, uint8_t *dst,
 	return 0;
 
 cipher_decrypt_err:
-	PMD_DRV_LOG(ERR, "libcrypto ECB cipher encrypt for BPI IV failed");
+	PMD_DRV_LOG(ERR, "libcrypto ECB cipher decrypt for BPI IV failed");
 	return -EINVAL;
 }
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/3] crypto/qat: fix parameter type

[Tomasz Jozwiak]

This commit fixes right cast from qat_cipher_get_block_size
function. This function can return -EFAULT in case of
any error, and that value must be cast to int instead of uint8_t

Fixes: d18ab45f7654 ("crypto/qat: support DOCSIS BPI mode")
Cc: stable@dpdk.org

Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_crypto.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
index 7b577b9..e411ae5 100644
--- a/drivers/crypto/qat/qat_crypto.c
+++ b/drivers/crypto/qat/qat_crypto.c
@@ -848,7 +848,7 @@ static inline uint32_t
 qat_bpicipher_preprocess(struct qat_session *ctx,
 				struct rte_crypto_op *op)
 {
-	uint8_t block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
+	int block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
 	struct rte_crypto_sym_op *sym_op = op->sym;
 	uint8_t last_block_len = block_len > 0 ?
 			sym_op->cipher.data.length % block_len : 0;
@@ -903,7 +903,7 @@ static inline uint32_t
 qat_bpicipher_postprocess(struct qat_session *ctx,
 				struct rte_crypto_op *op)
 {
-	uint8_t block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
+	int block_len = qat_cipher_get_block_size(ctx->qat_cipher_alg);
 	struct rte_crypto_sym_op *sym_op = op->sym;
 	uint8_t last_block_len = block_len > 0 ?
 			sym_op->cipher.data.length % block_len : 0;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/3] Fixes for QAT PMD

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Jozwiak
> Sent: Monday, January 22, 2018 4:28 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>; Griffin, John <john.griffin@intel.com>
> Cc: dev@dpdk.org; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Subject: [dpdk-dev] [PATCH v2 0/3] Fixes for QAT PMD
> 
> v2:
> - Previous patch has been split into 3 as below:
> 
> Tomasz Jozwiak (3):
>   crypto/qat: fix out-of-bounds compiler error
>   crypto/qat: fix typo in error message
>   crypto/qat: fix parameter type
> 
>  drivers/crypto/qat/qat_crypto.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> --
> 2.7.4

Applied to dpdk-next-crypto.
Thanks,

Pablo