diff mbox

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

Message ID 1516024409-7659-1-git-send-email-tomaszx.jozwiak@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Pablo de Lara Guarch
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Tomasz Jozwiak Jan. 15, 2018, 1:53 p.m. UTC 
  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(-)

Comments

Fiona Trahe Jan. 15, 2018, 2:30 p.m. UTC | #1 
> -----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>
De Lara Guarch, Pablo Jan. 17, 2018, 5:23 p.m. UTC | #2 
> -----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
Tomasz Jozwiak Jan. 22, 2018, 4:28 p.m. UTC | #3 
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(-)
De Lara Guarch, Pablo Jan. 23, 2018, 11:50 a.m. UTC | #4 
> -----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
diff mbox

Patch

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;