From 6290de2c3f7f6eab31ea60f842fe0dd43b7af491 Mon Sep 17 00:00:00 2001 From: Lukasz Wojciechowski Date: Tue, 5 May 2020 23:41:04 +0200 Subject: [PATCH] crypto/dpaa_sec: improve memory freeing 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 Signed-off-by: Lukasz Wojciechowski Acked-by: Akhil Goyal --- 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 a11b17bda7..021a5639da 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.20.1