add omitted release in qat_common

PATCH Superseded Quentin Lambert 3 participants 12 messages Sep 2, 2016 – Sep 13, 2016

Subsystems (2)

Kernel janitors Crypto API

lore

Share & tools

lore

Quentin Lambert

author

2016-09-02 14:37 UTC #0 lore raw

cover add omitted release in qat_common

The first patch introduces a variable to handle different error codes and be able to reuse the same clean up code. The second add an omitted release by jumping to the clean code having set the returned value to the proper error code.

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

---

Quentin Lambert

author

2016-09-02 14:37 UTC #1 lore raw

PATCH 1/2 crypto: qat - introduces a variable to handle error codes

Most error code used to jump to a label that lead to a "return -EFAULT" statement. This patch introduces a variable that stores the error code so that other error branches can use the same label to exit.

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/crypto/qat/qat_common/qat_uclo.c +10 -3

@@ -966,6 +966,7 @@ static int qat_uclo_parse_uof_obj(struct
 {
 	struct icp_qat_uclo_objhandle *obj_handle = handle->obj_handle;
 	unsigned int ae;
+	int ret;
 
 	obj_handle->uword_buf = kcalloc(UWORD_CPYBUF_SIZE, sizeof(uint64_t),
 					GFP_KERNEL);

@@ -987,29 +988,35 @@ static int qat_uclo_parse_uof_obj(struct
 	    !qat_uclo_map_str_table(obj_handle->obj_hdr, ICP_QAT_UOF_STRT,
 				    &obj_handle->str_table)) {
 		pr_err("QAT: UOF doesn't have effective images\n");
+		ret = -EFAULT;
 		goto out_err;
 	}
 	obj_handle->uimage_num  		qat_uclo_map_uimage(obj_handle, obj_handle->ae_uimage,
 				    ICP_QAT_UCLO_MAX_AE * ICP_QAT_UCLO_MAX_CTX);
-	if (!obj_handle->uimage_num)
+	if (!obj_handle->uimage_num) {
+		ret = -EFAULT;
 		goto out_err;
+	}
 	if (qat_uclo_map_ae(handle, handle->hal_handle->ae_max_num)) {
 		pr_err("QAT: Bad object\n");
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
 	}
 	qat_uclo_init_uword_num(handle);
 	qat_uclo_map_initmem_table(&obj_handle->encap_uof_obj,
 				   &obj_handle->init_mem_tab);
-	if (qat_uclo_set_ae_mode(handle))
+	if (qat_uclo_set_ae_mode(handle)) {
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
+	}
 	return 0;
 out_check_uof_aemask_err:
 	for (ae = 0; ae < obj_handle->uimage_num; ae++)
 		kfree(obj_handle->ae_uimage[ae].page);
 out_err:
 	kfree(obj_handle->uword_buf);
-	return -EFAULT;
+	return ret;
 }
 
 static int qat_uclo_map_suof_file_hdr(struct icp_qat_fw_loader_handle *handle,

Quentin Lambert

author

2016-09-02 14:37 UTC #2 lore raw

PATCH 2/2 crypto: qat - fix resource release omissions

In reply to Quentin Lambert

This issue was found with Hector.

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/crypto/qat/qat_common/qat_uclo.c +2 -1

@@ -981,7 +981,8 @@ static int qat_uclo_parse_uof_obj(struct
 			(PID_MINOR_REV & handle->hal_handle->revision_id);
 	if (qat_uclo_check_uof_compat(obj_handle)) {
 		pr_err("QAT: UOF incompatible\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_err;
 	}
 	obj_handle->ustore_phy_size = ICP_QAT_UCLO_MAX_USTORE;
 	if (!obj_handle->obj_hdr->file_buff ||

Quentin Lambert

author

2016-09-02 14:43 UTC #3 lore raw

cover add omitted release in qat_common

In reply to Quentin Lambert

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

---

Quentin Lambert

author

2016-09-02 14:43 UTC #4 lore raw

PATCH RESEND 1/2 crypto: qat - introduces a variable to handle error codes

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/crypto/qat/qat_common/qat_uclo.c +10 -3

@@ -966,6 +966,7 @@ static int qat_uclo_parse_uof_obj(struct
 {
 	struct icp_qat_uclo_objhandle *obj_handle = handle->obj_handle;
 	unsigned int ae;
+	int ret;
 
 	obj_handle->uword_buf = kcalloc(UWORD_CPYBUF_SIZE, sizeof(uint64_t),
 					GFP_KERNEL);

@@ -987,29 +988,35 @@ static int qat_uclo_parse_uof_obj(struct
 	    !qat_uclo_map_str_table(obj_handle->obj_hdr, ICP_QAT_UOF_STRT,
 				    &obj_handle->str_table)) {
 		pr_err("QAT: UOF doesn't have effective images\n");
+		ret = -EFAULT;
 		goto out_err;
 	}
 	obj_handle->uimage_num  		qat_uclo_map_uimage(obj_handle, obj_handle->ae_uimage,
 				    ICP_QAT_UCLO_MAX_AE * ICP_QAT_UCLO_MAX_CTX);
-	if (!obj_handle->uimage_num)
+	if (!obj_handle->uimage_num) {
+		ret = -EFAULT;
 		goto out_err;
+	}
 	if (qat_uclo_map_ae(handle, handle->hal_handle->ae_max_num)) {
 		pr_err("QAT: Bad object\n");
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
 	}
 	qat_uclo_init_uword_num(handle);
 	qat_uclo_map_initmem_table(&obj_handle->encap_uof_obj,
 				   &obj_handle->init_mem_tab);
-	if (qat_uclo_set_ae_mode(handle))
+	if (qat_uclo_set_ae_mode(handle)) {
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
+	}
 	return 0;
 out_check_uof_aemask_err:
 	for (ae = 0; ae < obj_handle->uimage_num; ae++)
 		kfree(obj_handle->ae_uimage[ae].page);
 out_err:
 	kfree(obj_handle->uword_buf);
-	return -EFAULT;
+	return ret;
 }
 
 static int qat_uclo_map_suof_file_hdr(struct icp_qat_fw_loader_handle *handle,

Quentin Lambert

author

2016-09-02 14:43 UTC #5 lore raw

PATCH 2/2 crypto: qat - fix resource release omissions

In reply to Quentin Lambert

In certain cases qat_uclo_parse_uof_obj used to return with an error code before releasing all resources. This patch add a jump to the appropriate label ensuring that the resources are properly released before returning.

This issue was found with Hector.

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/crypto/qat/qat_common/qat_uclo.c +2 -1

@@ -981,7 +981,8 @@ static int qat_uclo_parse_uof_obj(struct
 			(PID_MINOR_REV & handle->hal_handle->revision_id);
 	if (qat_uclo_check_uof_compat(obj_handle)) {
 		pr_err("QAT: UOF incompatible\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_err;
 	}
 	obj_handle->ustore_phy_size = ICP_QAT_UCLO_MAX_USTORE;
 	if (!obj_handle->obj_hdr->file_buff ||

Quentin Lambert

author

2016-09-02 14:47 UTC #6 lore raw

v2 cover add omitted release in qat_common

In reply to Quentin Lambert

-changes since v1
I failed to send the first version properly and it was missing a proper commit message, just ignore it.

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

---

Quentin Lambert

author

2016-09-02 14:47 UTC #7 lore raw

PATCH v2 1/2 crypto: qat - introduces a variable to handle error codes

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/crypto/qat/qat_common/qat_uclo.c +10 -3

@@ -966,6 +966,7 @@ static int qat_uclo_parse_uof_obj(struct
 {
 	struct icp_qat_uclo_objhandle *obj_handle = handle->obj_handle;
 	unsigned int ae;
+	int ret;
 
 	obj_handle->uword_buf = kcalloc(UWORD_CPYBUF_SIZE, sizeof(uint64_t),
 					GFP_KERNEL);

@@ -987,29 +988,35 @@ static int qat_uclo_parse_uof_obj(struct
 	    !qat_uclo_map_str_table(obj_handle->obj_hdr, ICP_QAT_UOF_STRT,
 				    &obj_handle->str_table)) {
 		pr_err("QAT: UOF doesn't have effective images\n");
+		ret = -EFAULT;
 		goto out_err;
 	}
 	obj_handle->uimage_num  		qat_uclo_map_uimage(obj_handle, obj_handle->ae_uimage,
 				    ICP_QAT_UCLO_MAX_AE * ICP_QAT_UCLO_MAX_CTX);
-	if (!obj_handle->uimage_num)
+	if (!obj_handle->uimage_num) {
+		ret = -EFAULT;
 		goto out_err;
+	}
 	if (qat_uclo_map_ae(handle, handle->hal_handle->ae_max_num)) {
 		pr_err("QAT: Bad object\n");
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
 	}
 	qat_uclo_init_uword_num(handle);
 	qat_uclo_map_initmem_table(&obj_handle->encap_uof_obj,
 				   &obj_handle->init_mem_tab);
-	if (qat_uclo_set_ae_mode(handle))
+	if (qat_uclo_set_ae_mode(handle)) {
+		ret = -EFAULT;
 		goto out_check_uof_aemask_err;
+	}
 	return 0;
 out_check_uof_aemask_err:
 	for (ae = 0; ae < obj_handle->uimage_num; ae++)
 		kfree(obj_handle->ae_uimage[ae].page);
 out_err:
 	kfree(obj_handle->uword_buf);
-	return -EFAULT;
+	return ret;
 }
 
 static int qat_uclo_map_suof_file_hdr(struct icp_qat_fw_loader_handle *handle,

Quentin Lambert

author

2016-09-02 14:47 UTC #8 lore raw

PATCH v2 2/2 crypto: qat - fix resource release omissions

In reply to Quentin Lambert

This issue was found with Hector.

Signed-off-by Quentin Lambert <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/crypto/qat/qat_common/qat_uclo.c +2 -1

@@ -981,7 +981,8 @@ static int qat_uclo_parse_uof_obj(struct
 			(PID_MINOR_REV & handle->hal_handle->revision_id);
 	if (qat_uclo_check_uof_compat(obj_handle)) {
 		pr_err("QAT: UOF incompatible\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_err;
 	}
 	obj_handle->ustore_phy_size = ICP_QAT_UCLO_MAX_USTORE;
 	if (!obj_handle->obj_hdr->file_buff ||

Giovanni Cabiddu

2016-09-06 10:18 UTC #9 lore raw

Re: [PATCH v2 2/2] crypto: qat - fix resource release omissions

Hi Lambert,

Quoted 5 lines · Quentin Lambert

In certain cases qat_uclo_parse_uof_obj used to return with an error code before releasing all resources. This patch add a jump to the appropriate label ensuring that the resources are properly released before returning.
This issue was found with Hector.

Thanks for the patches. This can be easily fixed by moving the kcalloc after the compatibility check function. What do you think?

---8<---
Subject: [PATCH] crypto: qat - fix leak on error path

Fix a memory leak in an error path in uc loader.

Signed-off-by Giovanni Cabiddu <[email protected]>

---
 drivers/crypto/qat/qat_common/qat_uclo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/qat/qat_common/qat_uclo.c +4 -4

index 9b961b3..e2454d9 100644
@@ -967,10 +967,6 @@ static int qat_uclo_parse_uof_obj(struct icp_qat_fw_loader_handle *handle)
 	struct icp_qat_uclo_objhandle *obj_handle = handle->obj_handle;
 	unsigned int ae;
 
-	obj_handle->uword_buf = kcalloc(UWORD_CPYBUF_SIZE, sizeof(uint64_t),
-					GFP_KERNEL);
-	if (!obj_handle->uword_buf)
-		return -ENOMEM;
 	obj_handle->encap_uof_obj.beg_uof = obj_handle->obj_hdr->file_buff;
 	obj_handle->encap_uof_obj.obj_hdr = (struct icp_qat_uof_objhdr *)
 					     obj_handle->obj_hdr->file_buff;

@@ -982,6 +978,10 @@ static int qat_uclo_parse_uof_obj(struct icp_qat_fw_loader_handle *handle)
 		pr_err("QAT: UOF incompatible\n");
 		return -EINVAL;
 	}
+	obj_handle->uword_buf = kcalloc(UWORD_CPYBUF_SIZE, sizeof(uint64_t),
+					GFP_KERNEL);
+	if (!obj_handle->uword_buf)
+		return -ENOMEM;
 	obj_handle->ustore_phy_size = ICP_QAT_UCLO_MAX_USTORE;
 	if (!obj_handle->obj_hdr->file_buff ||
 	    !qat_uclo_map_str_table(obj_handle->obj_hdr, ICP_QAT_UOF_STRT,

Herbert Xu

maintainer

2016-09-13 12:40 UTC #10 lore raw

Re: [PATCH v2 2/2] crypto: qat - fix resource release omissions

Quoted 7 lines · Giovanni Cabiddu

---8<---
Subject: [PATCH] crypto: qat - fix leak on error path
Fix a memory leak in an error path in uc loader.
Signed-off-by Giovanni Cabiddu <[email protected]>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Quentin Lambert

author

2016-09-13 12:56 UTC #11 lore raw

Re: [PATCH v2 2/2] crypto: qat - fix resource release omissions

Quoted 8 lines · Herbert Xu

Quoted 6 lines · Giovanni Cabiddu
---8<---
Subject: [PATCH] crypto: qat - fix leak on error path
Fix a memory leak in an error path in uc loader.
Signed-off-by Giovanni Cabiddu <[email protected]>
Patch applied. Thanks.

Sorry, I completly missed Giovanni's message. Good work!

Quentin