[PATCH v4] s390/pkey: Fix memory leak in error case by using memdup_user() rather than open coding

[Markus Elfring]

Date: Mon, 11 Nov 2019 15:20:44 +0100

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

Generated by: scripts/coccinelle/api/memdup_user.cocci

* The function "_copy_apqns_from_user" contained a memory leak
  because of a misssing function call "kfree(kapqns)" for an if branch.
  Link: https://lore.kernel.org/r/833d7d5e-6ede-6bdd-a2cc-2da7f0b03908@xxxxxxxxxx/

  Thus complete the exception handling by this code replacement.

* Delete local variables which became unnecessary with this refactoring
  in two function implementations.

Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
Signed-off-by: Markus Elfring <Markus.Elfring@xxxxxx>
---

v4:
Further changes were requested by Christian BorntrÃger.
https://lore.kernel.org/r/040f3e18-d97a-fc32-b237-20e7553e1733@xxxxxxxxxx/

* An other patch subject was selected.

* An other email address was used for the tag âSigned-off-byâ this time.

v3:
An adjustment was requested by Christian BorntrÃger for the change description.
https://lore.kernel.org/r/c0df9cc8-c41a-1e5d-811c-1ff045c13fcc@xxxxxxxxxx/

v2:
Further changes were requested by Joe Perches.
https://lore.kernel.org/r/6137855bb4170c438c7436cbdb7dfd21639a8855.camel@xxxxxxxxxxx/

* The proposed usage of two conditional operators was replaced by
  an other code structure.

* A sanity check was adjusted for the function â_copy_apqns_from_userâ.


 drivers/s390/crypto/pkey_api.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 9de3d46b3253..ac99fd97569d 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -715,36 +715,18 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype,

 static void *_copy_key_from_user(void __user *ukey, size_t keylen)
 {
-	void *kkey;
-
 	if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE)
 		return ERR_PTR(-EINVAL);
-	kkey = kmalloc(keylen, GFP_KERNEL);
-	if (!kkey)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_user(kkey, ukey, keylen)) {
-		kfree(kkey);
-		return ERR_PTR(-EFAULT);
-	}

-	return kkey;
+	return memdup_user(ukey, keylen);
 }

 static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns)
 {
-	void *kapqns = NULL;
-	size_t nbytes;
-
-	if (uapqns && nr_apqns > 0) {
-		nbytes = nr_apqns * sizeof(struct pkey_apqn);
-		kapqns = kmalloc(nbytes, GFP_KERNEL);
-		if (!kapqns)
-			return ERR_PTR(-ENOMEM);
-		if (copy_from_user(kapqns, uapqns, nbytes))
-			return ERR_PTR(-EFAULT);
-	}
+	if (!uapqns || nr_apqns <= 0)
+		return NULL;

-	return kapqns;
+	return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));
 }

 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
--
2.24.0