[dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Lukasz Wojciechowski]

This patch fixes management of memory for authentication
and encryption keys.
There were two issues with former state of implementation:

1) Invalid access to dpaa_sec_session union members
    The dpaa_sec_session structure includes an anonymous union:
    union {
        struct {...} aead_key;
        struct {
            struct {...} cipher_key;
            struct {...} auth_key;
        };
    };
    Depending on the used algorithm a rte_zmalloc() function
    allocated memory that was kept in aead_key, cipher_key
    or auth_key. However every time the memory was released,
    rte_free() was called only on cipher and auth keys, even
    if pointer to allocated memory was stored in aead_key.

    The C language specification defines such behavior as undefined.
    As the cipher_key and aead_key are similar, have same sizes and
    alignment, it has worked, but it's directly against C specification.

    This patch fixes this, providing a free_session_data() function
    to free the keys data. It verifies which algorithm was used
    (aead or auth+cipher) and frees proper part of the union.

2) Some keys might have been freed multiple times
    In functions like: dpaa_sec_cipher_init(), dpaa_sec_auth_init(),
    dpaa_sec_chain_init(), dpaa_sec_aead_init() keys data were freed
    before returning due to some error conditions. However the pointers
    were not zeroed causing another calls to ret_free from higher
    layers of code. This causes an error log about invalid memory address
    to be printed.

    This patch fixes it by making only one layer responsible for freeing
    memory:
    * dpaa_sec_set_session_parameters() for allocations made in:
        - dpaa_sec_cipher_init()
        - dpaa_sec_auth_init()
        - dpaa_sec_chain_init()
        - dpaa_sec_aead_init()
    * dpaa_sec_set_ipsec_session() for allocations made in:
        - dpaa_sec_ipsec_aead_init()
        - dpaa_sec_ipsec_proto_init()
    * dpaa_sec_set_pdcp_session() for allocations made in:
        - dpaa_sec_set_pdcp_session() /*only one layer in case of pdcp*/

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 drivers/crypto/dpaa_sec/dpaa_sec.c | 45 ++++++++++++++++--------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index a11b17bda..021a5639d 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -219,6 +219,13 @@ dpaa_sec_init_tx(struct qman_fq *fq)
 	return ret;
 }
 
+static inline int is_aead(dpaa_sec_session *ses)
+{
+	return ((ses->cipher_alg == 0) &&
+		(ses->auth_alg == 0) &&
+		(ses->aead_alg != 0));
+}
+
 static inline int is_encode(dpaa_sec_session *ses)
 {
 	return ses->dir == DIR_ENC;
@@ -2040,7 +2047,6 @@ dpaa_sec_cipher_init(struct rte_cryptodev *dev __rte_unused,
 	default:
 		DPAA_SEC_ERR("Crypto: Undefined Cipher specified %u",
 			      xform->cipher.algo);
-		rte_free(session->cipher_key.data);
 		return -1;
 	}
 	session->dir = (xform->cipher.op == RTE_CRYPTO_CIPHER_OP_ENCRYPT) ?
@@ -2108,7 +2114,6 @@ dpaa_sec_auth_init(struct rte_cryptodev *dev __rte_unused,
 	default:
 		DPAA_SEC_ERR("Crypto: Unsupported Auth specified %u",
 			      xform->auth.algo);
-		rte_free(session->auth_key.data);
 		return -1;
 	}
 
@@ -2151,7 +2156,6 @@ dpaa_sec_chain_init(struct rte_cryptodev *dev __rte_unused,
 					     RTE_CACHE_LINE_SIZE);
 	if (session->auth_key.data == NULL && auth_xform->key.length > 0) {
 		DPAA_SEC_ERR("No Memory for auth key");
-		rte_free(session->cipher_key.data);
 		return -ENOMEM;
 	}
 	session->auth_key.length = auth_xform->key.length;
@@ -2191,7 +2195,7 @@ dpaa_sec_chain_init(struct rte_cryptodev *dev __rte_unused,
 	default:
 		DPAA_SEC_ERR("Crypto: Unsupported Auth specified %u",
 			      auth_xform->algo);
-		goto error_out;
+		return -1;
 	}
 
 	session->cipher_alg = cipher_xform->algo;
@@ -2212,16 +2216,11 @@ dpaa_sec_chain_init(struct rte_cryptodev *dev __rte_unused,
 	default:
 		DPAA_SEC_ERR("Crypto: Undefined Cipher specified %u",
 			      cipher_xform->algo);
-		goto error_out;
+		return -1;
 	}
 	session->dir = (cipher_xform->op == RTE_CRYPTO_CIPHER_OP_ENCRYPT) ?
 				DIR_ENC : DIR_DEC;
 	return 0;
-
-error_out:
-	rte_free(session->cipher_key.data);
-	rte_free(session->auth_key.data);
-	return -1;
 }
 
 static int
@@ -2253,7 +2252,6 @@ dpaa_sec_aead_init(struct rte_cryptodev *dev __rte_unused,
 		break;
 	default:
 		DPAA_SEC_ERR("unsupported AEAD alg %d", session->aead_alg);
-		rte_free(session->aead_key.data);
 		return -ENOMEM;
 	}
 
@@ -2323,6 +2321,18 @@ dpaa_sec_attach_sess_q(struct dpaa_sec_qp *qp, dpaa_sec_session *sess)
 	return ret;
 }
 
+static inline void
+free_session_data(dpaa_sec_session *s)
+{
+	if (is_aead(s))
+		rte_free(s->aead_key.data);
+	else {
+		rte_free(s->auth_key.data);
+		rte_free(s->cipher_key.data);
+	}
+	memset(s, 0, sizeof(dpaa_sec_session));
+}
+
 static int
 dpaa_sec_set_session_parameters(struct rte_cryptodev *dev,
 			    struct rte_crypto_sym_xform *xform,	void *sess)
@@ -2415,10 +2425,7 @@ dpaa_sec_set_session_parameters(struct rte_cryptodev *dev,
 	return 0;
 
 err1:
-	rte_free(session->cipher_key.data);
-	rte_free(session->auth_key.data);
-	memset(session, 0, sizeof(dpaa_sec_session));
-
+	free_session_data(session);
 	return -EINVAL;
 }
 
@@ -2467,9 +2474,7 @@ free_session_memory(struct rte_cryptodev *dev, dpaa_sec_session *s)
 		s->inq[i] = NULL;
 		s->qp[i] = NULL;
 	}
-	rte_free(s->cipher_key.data);
-	rte_free(s->auth_key.data);
-	memset(s, 0, sizeof(dpaa_sec_session));
+	free_session_data(s);
 	rte_mempool_put(sess_mp, (void *)s);
 }
 
@@ -2836,9 +2841,7 @@ dpaa_sec_set_ipsec_session(__rte_unused struct rte_cryptodev *dev,
 
 	return 0;
 out:
-	rte_free(session->auth_key.data);
-	rte_free(session->cipher_key.data);
-	memset(session, 0, sizeof(dpaa_sec_session));
+	free_session_data(session);
 	return -1;
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] crypto/dpaa_sec: repair memory allocations

[Lukasz Wojciechowski]

This patch repairs 2 memory allocations issues:

1) possible leak of memory
    In cryptodev_dpaa_sec_probe() function in case of portal
    initialization failure, function exited without cleanup.

    The patch redirects flow to out label, which provides
    proper cleanup in case of error: freeing cryptodevice private
    data and releasing cryptodevice.

2) double free of cryptodev private data
    The function dpaa_sec_dev_init() in case of failure called
    dpaa_sec_uninit() which freed both private data and security
    context. However one layer above in cryptodev_dpaa_sec_probe()
    function, the private data were freed one more time.

    The patch limits cleanup of the dpaa_sec_dev_init() function
    to freeing only the security context.

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 drivers/crypto/dpaa_sec/dpaa_sec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index 021a5639d..097ab8de9 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -3410,7 +3410,7 @@ dpaa_sec_dev_init(struct rte_cryptodev *cryptodev)
 init_error:
 	DPAA_SEC_ERR("driver %s: create failed\n", cryptodev->data->name);
 
-	dpaa_sec_uninit(cryptodev);
+	rte_free(cryptodev->security_ctx);
 	return -EFAULT;
 }
 
@@ -3467,7 +3467,7 @@ cryptodev_dpaa_sec_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused,
 		retval = rte_dpaa_portal_init((void *)1);
 		if (retval) {
 			DPAA_SEC_ERR("Unable to initialize portal");
-			return retval;
+			goto out;
 		}
 	}
 
@@ -3476,13 +3476,15 @@ cryptodev_dpaa_sec_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused,
 	if (retval == 0)
 		return 0;
 
+	retval = -ENXIO;
+out:
 	/* In case of error, cleanup is done */
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		rte_free(cryptodev->data->dev_private);
 
 	rte_cryptodev_pmd_release_device(cryptodev);
 
-	return -ENXIO;
+	return retval;
 }
 
 static int
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Akhil Goyal]

Hi Lukasz,

Thanks for the detailed analysis.
Series
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Applied to dpdk-next-crypto

Thanks.
> 
> This patch fixes management of memory for authentication
> and encryption keys.
> There were two issues with former state of implementation:
> 
> 1) Invalid access to dpaa_sec_session union members
>     The dpaa_sec_session structure includes an anonymous union:
>     union {
>         struct {...} aead_key;
>         struct {
>             struct {...} cipher_key;
>             struct {...} auth_key;
>         };
>     };
>     Depending on the used algorithm a rte_zmalloc() function
>     allocated memory that was kept in aead_key, cipher_key
>     or auth_key. However every time the memory was released,
>     rte_free() was called only on cipher and auth keys, even
>     if pointer to allocated memory was stored in aead_key.
> 
>     The C language specification defines such behavior as undefined.
>     As the cipher_key and aead_key are similar, have same sizes and
>     alignment, it has worked, but it's directly against C specification.
> 
>     This patch fixes this, providing a free_session_data() function
>     to free the keys data. It verifies which algorithm was used
>     (aead or auth+cipher) and frees proper part of the union.
> 
> 2) Some keys might have been freed multiple times
>     In functions like: dpaa_sec_cipher_init(), dpaa_sec_auth_init(),
>     dpaa_sec_chain_init(), dpaa_sec_aead_init() keys data were freed
>     before returning due to some error conditions. However the pointers
>     were not zeroed causing another calls to ret_free from higher
>     layers of code. This causes an error log about invalid memory address
>     to be printed.
> 
>     This patch fixes it by making only one layer responsible for freeing
>     memory:
>     * dpaa_sec_set_session_parameters() for allocations made in:
>         - dpaa_sec_cipher_init()
>         - dpaa_sec_auth_init()
>         - dpaa_sec_chain_init()
>         - dpaa_sec_aead_init()
>     * dpaa_sec_set_ipsec_session() for allocations made in:
>         - dpaa_sec_ipsec_aead_init()
>         - dpaa_sec_ipsec_proto_init()
>     * dpaa_sec_set_pdcp_session() for allocations made in:
>         - dpaa_sec_set_pdcp_session() /*only one layer in case of pdcp*/
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---

Re: [dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Lukasz Wojciechowski]

W dniu 09.05.2020 o 23:58, Akhil Goyal pisze:
> Hi Lukasz,
>
> Thanks for the detailed analysis.
> Series
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
>
> Applied to dpdk-next-crypto
>
> Thanks.
I am a fan of big commit messages. Just let me know when I'll cross the 
line withe these novels ;)
Thank you
>> This patch fixes management of memory for authentication
>> and encryption keys.
>> There were two issues with former state of implementation:
>>
>> 1) Invalid access to dpaa_sec_session union members
>>      The dpaa_sec_session structure includes an anonymous union:
>>      union {
>>          struct {...} aead_key;
>>          struct {
>>              struct {...} cipher_key;
>>              struct {...} auth_key;
>>          };
>>      };
>>      Depending on the used algorithm a rte_zmalloc() function
>>      allocated memory that was kept in aead_key, cipher_key
>>      or auth_key. However every time the memory was released,
>>      rte_free() was called only on cipher and auth keys, even
>>      if pointer to allocated memory was stored in aead_key.
>>
>>      The C language specification defines such behavior as undefined.
>>      As the cipher_key and aead_key are similar, have same sizes and
>>      alignment, it has worked, but it's directly against C specification.
>>
>>      This patch fixes this, providing a free_session_data() function
>>      to free the keys data. It verifies which algorithm was used
>>      (aead or auth+cipher) and frees proper part of the union.
>>
>> 2) Some keys might have been freed multiple times
>>      In functions like: dpaa_sec_cipher_init(), dpaa_sec_auth_init(),
>>      dpaa_sec_chain_init(), dpaa_sec_aead_init() keys data were freed
>>      before returning due to some error conditions. However the pointers
>>      were not zeroed causing another calls to ret_free from higher
>>      layers of code. This causes an error log about invalid memory address
>>      to be printed.
>>
>>      This patch fixes it by making only one layer responsible for freeing
>>      memory:
>>      * dpaa_sec_set_session_parameters() for allocations made in:
>>          - dpaa_sec_cipher_init()
>>          - dpaa_sec_auth_init()
>>          - dpaa_sec_chain_init()
>>          - dpaa_sec_aead_init()
>>      * dpaa_sec_set_ipsec_session() for allocations made in:
>>          - dpaa_sec_ipsec_aead_init()
>>          - dpaa_sec_ipsec_proto_init()
>>      * dpaa_sec_set_pdcp_session() for allocations made in:
>>          - dpaa_sec_set_pdcp_session() /*only one layer in case of pdcp*/
>>
>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> ---

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Akhil Goyal]

> 
> W dniu 09.05.2020 o 23:58, Akhil Goyal pisze:
> > Hi Lukasz,
> >
> > Thanks for the detailed analysis.
> > Series
> > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > Applied to dpdk-next-crypto
> >
> > Thanks.
> I am a fan of big commit messages. Just let me know when I'll cross the
> line withe these novels ;)
> Thank you

Commit messages should be descriptive enough but should not be very long.
I trimmed few lines in the message as they were not necessary. 
Please add only relevant information in limited words. 😊

Regards,
Akhil

Re: [dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Lukasz Wojciechowski]

W dniu 11.05.2020 o 12:20, Akhil Goyal pisze:
>> W dniu 09.05.2020 o 23:58, Akhil Goyal pisze:
>>> Hi Lukasz,
>>>
>>> Thanks for the detailed analysis.
>>> Series
>>> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
>>>
>>> Applied to dpdk-next-crypto
>>>
>>> Thanks.
>> I am a fan of big commit messages. Just let me know when I'll cross the
>> line withe these novels ;)
>> Thank you
> Commit messages should be descriptive enough but should not be very long.
> I trimmed few lines in the message as they were not necessary.
> Please add only relevant information in limited words. 😊
I'll try ;)
Thanks
> Regards,
> Akhil

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com