[PATCH v5 0/1] Rework pkey ioctl functions error paths

[PATCH v5 0/1] Rework pkey ioctl functions error paths

[Harald Freudenberger]

Slight rework for all the pkey ioctl functions, especially related to
the error paths with regard to free memory. This patch removes all
this duplicated code and introduces one code block at the end of the
functions which is jumped into via goto out or executed on regular
exit.

Changelog:
v1 - first version
v2 - just a rebase of the v1 patch as the original patch did not apply
     any more.
v3 - Fix paths where a error ptr returned from a helper function may
     be used as input for kfree(). Fixed typo pathes -> paths
v4 - Added yet another slight rework covering the possible use of an
     uninitialized variable in an error path.
v5 - Resend without any code changes, just Finn's reviewed by added.

Harald Freudenberger (1):
  s390/pkey: Rework ioctl functions error paths

 drivers/s390/crypto/pkey_api.c | 288 ++++++++++++++++++---------------
 1 file changed, 157 insertions(+), 131 deletions(-)

--
2.43.0

[PATCH v5 1/1] s390/pkey: Rework ioctl functions error paths

[Harald Freudenberger]

With the pkey rework there was the suggestion to rework the error and
free paths of the pkey ioctl functions. The complain was especially to
rewrite the failure handling with goto instead of all repeat the
nearly same code (kfree(), kfree_sensitive(), memzero_explicit()) for
each path. This patch removes all this duplicated code and introduces
one code block at the end of the functions which is jumped into via
goto out or executed on regular exit. As some helper functions return
an error pointer value (which is NOT NULL) make sure on the error path
there is not by accident kfree() or similar called on such ptr values.

Suggested-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
---
 drivers/s390/crypto/pkey_api.c | 288 ++++++++++++++++++---------------
 1 file changed, 157 insertions(+), 131 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 5d8f63f390a8..0530b1ce228e 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -169,8 +169,8 @@ static int pkey_ioctl_clr2protk(struct pkey_clr2protk __user *ucp)
 {
 	struct pkey_clr2protk kcp;
 	struct clearkeytoken *t;
+	u8 *tmpbuf = NULL;
 	u32 keylen;
-	u8 *tmpbuf;
 	int rc;
 
 	if (copy_from_user(&kcp, ucp, sizeof(kcp)))
@@ -181,13 +181,13 @@ static int pkey_ioctl_clr2protk(struct pkey_clr2protk __user *ucp)
 	if (!keylen) {
 		PKEY_DBF_ERR("%s unknown/unsupported keytype %u\n",
 			     __func__, kcp.keytype);
-		memzero_explicit(&kcp, sizeof(kcp));
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 	tmpbuf = kzalloc(sizeof(*t) + keylen, GFP_KERNEL);
 	if (!tmpbuf) {
-		memzero_explicit(&kcp, sizeof(kcp));
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
 	}
 	t = (struct clearkeytoken *)tmpbuf;
 	t->type = TOKTYPE_NON_CCA;
@@ -202,20 +202,22 @@ static int pkey_ioctl_clr2protk(struct pkey_clr2protk __user *ucp)
 			 kcp.protkey.protkey,
 			 &kcp.protkey.len, &kcp.protkey.type, 0);
 	pr_debug("key2protkey()=%d\n", rc);
+	if (rc)
+		goto out;
 
-	kfree_sensitive(tmpbuf);
-
-	if (!rc && copy_to_user(ucp, &kcp, sizeof(kcp)))
+	if (copy_to_user(ucp, &kcp, sizeof(kcp)))
 		rc = -EFAULT;
-	memzero_explicit(&kcp, sizeof(kcp));
 
+out:
+	memzero_explicit(&kcp, sizeof(kcp));
+	kfree_sensitive(tmpbuf);
 	return rc;
 }
 
 static int pkey_ioctl_findcard(struct pkey_findcard __user *ufc)
 {
+	struct pkey_apqn *apqns = NULL;
 	struct pkey_findcard kfc;
-	struct pkey_apqn *apqns;
 	size_t nr_apqns;
 	int rc;
 
@@ -224,8 +226,10 @@ static int pkey_ioctl_findcard(struct pkey_findcard __user *ufc)
 
 	nr_apqns = MAXAPQNSINLIST;
 	apqns = kmalloc_objs(struct pkey_apqn, nr_apqns);
-	if (!apqns)
-		return -ENOMEM;
+	if (!apqns) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	rc = pkey_handler_apqns_for_key(kfc.seckey.seckey,
 					sizeof(kfc.seckey.seckey),
@@ -237,17 +241,18 @@ static int pkey_ioctl_findcard(struct pkey_findcard __user *ufc)
 						PKEY_FLAGS_MATCH_ALT_MKVP,
 						apqns, &nr_apqns, 0);
 	pr_debug("apqns_for_key()=%d\n", rc);
-	if (rc) {
-		kfree(apqns);
-		return rc;
-	}
+	if (rc)
+		goto out;
+
 	kfc.cardnr = apqns[0].card;
 	kfc.domain = apqns[0].domain;
-	kfree(apqns);
+
 	if (copy_to_user(ufc, &kfc, sizeof(kfc)))
-		return -EFAULT;
+		rc = -EFAULT;
 
-	return 0;
+out:
+	kfree(apqns);
+	return rc;
 }
 
 static int pkey_ioctl_skey2pkey(struct pkey_skey2pkey __user *usp)
@@ -327,7 +332,7 @@ static int pkey_ioctl_verifyprotk(struct pkey_verifyprotk __user *uvp)
 {
 	struct pkey_verifyprotk kvp;
 	struct protaeskeytoken *t;
-	u8 *tmpbuf;
+	u8 *tmpbuf = NULL;
 	int rc;
 
 	if (copy_from_user(&kvp, uvp, sizeof(kvp)))
@@ -336,15 +341,15 @@ static int pkey_ioctl_verifyprotk(struct pkey_verifyprotk __user *uvp)
 	if (kvp.protkey.len > sizeof(kvp.protkey.protkey)) {
 		PKEY_DBF_ERR("%s protkey length %u exceeds protkey buffer size\n",
 			     __func__, kvp.protkey.len);
-		memzero_explicit(&kvp, sizeof(kvp));
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 
 	/* build a 'protected key token' from the raw protected key */
 	tmpbuf = kzalloc(sizeof(*t), GFP_KERNEL);
 	if (!tmpbuf) {
-		memzero_explicit(&kvp, sizeof(kvp));
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
 	}
 	t = (struct protaeskeytoken *)tmpbuf;
 	t->type = TOKTYPE_NON_CCA;
@@ -357,42 +362,48 @@ static int pkey_ioctl_verifyprotk(struct pkey_verifyprotk __user *uvp)
 				     NULL, NULL, NULL, NULL, NULL, 0);
 	pr_debug("verify_key()=%d\n", rc);
 
-	kfree_sensitive(tmpbuf);
+out:
 	memzero_explicit(&kvp, sizeof(kvp));
-
+	kfree_sensitive(tmpbuf);
 	return rc;
 }
 
 static int pkey_ioctl_kblob2protk(struct pkey_kblob2pkey __user *utp)
 {
 	struct pkey_kblob2pkey ktp;
-	u8 *kkey;
+	u8 *kkey = NULL;
 	int rc;
 
 	if (copy_from_user(&ktp, utp, sizeof(ktp)))
 		return -EFAULT;
 	kkey = _copy_key_from_user(ktp.key, ktp.keylen);
-	if (IS_ERR(kkey))
-		return PTR_ERR(kkey);
+	if (IS_ERR(kkey)) {
+		rc = PTR_ERR(kkey);
+		kkey = NULL;
+		goto out;
+	}
 	ktp.protkey.len = sizeof(ktp.protkey.protkey);
 	rc = key2protkey(NULL, 0, kkey, ktp.keylen,
 			 ktp.protkey.protkey, &ktp.protkey.len,
 			 &ktp.protkey.type, 0);
 	pr_debug("key2protkey()=%d\n", rc);
-	kfree_sensitive(kkey);
-	if (!rc && copy_to_user(utp, &ktp, sizeof(ktp)))
+	if (rc)
+		goto out;
+	if (copy_to_user(utp, &ktp, sizeof(ktp)))
 		rc = -EFAULT;
-	memzero_explicit(&ktp, sizeof(ktp));
 
+out:
+	memzero_explicit(&ktp, sizeof(ktp));
+	kfree_sensitive(kkey);
 	return rc;
 }
 
 static int pkey_ioctl_genseck2(struct pkey_genseck2 __user *ugs)
 {
+	struct pkey_apqn *apqns = NULL;
 	u32 klen = KEYBLOBBUFSIZE;
 	struct pkey_genseck2 kgs;
-	struct pkey_apqn *apqns;
-	u8 *kkey;
+	u8 *kkey = NULL;
 	int rc;
 	u32 u;
 
@@ -405,46 +416,48 @@ static int pkey_ioctl_genseck2(struct pkey_genseck2 __user *ugs)
 		return -EINVAL;
 	}
 	apqns = _copy_apqns_from_user(kgs.apqns, kgs.apqn_entries);
-	if (IS_ERR(apqns))
-		return PTR_ERR(apqns);
+	if (IS_ERR(apqns)) {
+		rc = PTR_ERR(apqns);
+		apqns = NULL;
+		goto out;
+	}
 	kkey = kzalloc(klen, GFP_KERNEL);
 	if (!kkey) {
-		kfree(apqns);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
 	}
 	rc = pkey_handler_gen_key(apqns, kgs.apqn_entries,
 				  u, kgs.type, kgs.size, kgs.keygenflags,
 				  kkey, &klen, NULL, 0);
 	pr_debug("gen_key()=%d\n", rc);
-	kfree(apqns);
-	if (rc) {
-		kfree_sensitive(kkey);
-		return rc;
-	}
+	if (rc)
+		goto out;
 	if (kgs.key) {
 		if (kgs.keylen < klen) {
-			kfree_sensitive(kkey);
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 		if (copy_to_user(kgs.key, kkey, klen)) {
-			kfree_sensitive(kkey);
-			return -EFAULT;
+			rc = -EFAULT;
+			goto out;
 		}
 	}
 	kgs.keylen = klen;
 	if (copy_to_user(ugs, &kgs, sizeof(kgs)))
 		rc = -EFAULT;
-	kfree_sensitive(kkey);
 
+out:
+	kfree_sensitive(kkey);
+	kfree(apqns);
 	return rc;
 }
 
 static int pkey_ioctl_clr2seck2(struct pkey_clr2seck2 __user *ucs)
 {
+	struct pkey_apqn *apqns = NULL;
 	u32 klen = KEYBLOBBUFSIZE;
 	struct pkey_clr2seck2 kcs;
-	struct pkey_apqn *apqns;
-	u8 *kkey;
+	u8 *kkey = NULL;
 	int rc;
 	u32 u;
 
@@ -454,49 +467,45 @@ static int pkey_ioctl_clr2seck2(struct pkey_clr2seck2 __user *ucs)
 	if (!u) {
 		PKEY_DBF_ERR("%s unknown/unsupported keybitsize %d\n",
 			     __func__, kcs.size);
-		memzero_explicit(&kcs, sizeof(kcs));
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 	apqns = _copy_apqns_from_user(kcs.apqns, kcs.apqn_entries);
 	if (IS_ERR(apqns)) {
-		memzero_explicit(&kcs, sizeof(kcs));
-		return PTR_ERR(apqns);
+		rc = PTR_ERR(apqns);
+		apqns = NULL;
+		goto out;
 	}
 	kkey = kzalloc(klen, GFP_KERNEL);
 	if (!kkey) {
-		kfree(apqns);
-		memzero_explicit(&kcs, sizeof(kcs));
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
 	}
 	rc = pkey_handler_clr_to_key(apqns, kcs.apqn_entries,
 				     u, kcs.type, kcs.size, kcs.keygenflags,
 				     kcs.clrkey.clrkey, kcs.size / 8,
 				     kkey, &klen, NULL, 0);
 	pr_debug("clr_to_key()=%d\n", rc);
-	kfree(apqns);
-	if (rc) {
-		kfree_sensitive(kkey);
-		memzero_explicit(&kcs, sizeof(kcs));
-		return rc;
-	}
+	if (rc)
+		goto out;
 	if (kcs.key) {
 		if (kcs.keylen < klen) {
-			kfree_sensitive(kkey);
-			memzero_explicit(&kcs, sizeof(kcs));
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 		if (copy_to_user(kcs.key, kkey, klen)) {
-			kfree_sensitive(kkey);
-			memzero_explicit(&kcs, sizeof(kcs));
-			return -EFAULT;
+			rc = -EFAULT;
+			goto out;
 		}
 	}
 	kcs.keylen = klen;
 	if (copy_to_user(ucs, &kcs, sizeof(kcs)))
 		rc = -EFAULT;
+
+out:
 	memzero_explicit(&kcs, sizeof(kcs));
 	kfree_sensitive(kkey);
-
+	kfree(apqns);
 	return rc;
 }
 
@@ -509,18 +518,23 @@ static int pkey_ioctl_verifykey2(struct pkey_verifykey2 __user *uvk)
 	if (copy_from_user(&kvk, uvk, sizeof(kvk)))
 		return -EFAULT;
 	kkey = _copy_key_from_user(kvk.key, kvk.keylen);
-	if (IS_ERR(kkey))
-		return PTR_ERR(kkey);
+	if (IS_ERR(kkey)) {
+		rc = PTR_ERR(kkey);
+		kkey = NULL;
+		goto out;
+	}
 
 	rc = pkey_handler_verify_key(kkey, kvk.keylen,
 				     &kvk.cardnr, &kvk.domain,
 				     &kvk.type, &kvk.size, &kvk.flags, 0);
 	pr_debug("verify_key()=%d\n", rc);
+	if (rc)
+		goto out;
+	if (copy_to_user(uvk, &kvk, sizeof(kvk)))
+		rc = -EFAULT;
 
+out:
 	kfree_sensitive(kkey);
-	if (!rc && copy_to_user(uvk, &kvk, sizeof(kvk)))
-		return -EFAULT;
-
 	return rc;
 }
 
@@ -528,30 +542,37 @@ static int pkey_ioctl_kblob2protk2(struct pkey_kblob2pkey2 __user *utp)
 {
 	struct pkey_apqn *apqns = NULL;
 	struct pkey_kblob2pkey2 ktp;
-	u8 *kkey;
+	u8 *kkey = NULL;
 	int rc;
 
 	if (copy_from_user(&ktp, utp, sizeof(ktp)))
 		return -EFAULT;
 	apqns = _copy_apqns_from_user(ktp.apqns, ktp.apqn_entries);
-	if (IS_ERR(apqns))
-		return PTR_ERR(apqns);
+	if (IS_ERR(apqns)) {
+		rc = PTR_ERR(apqns);
+		apqns = NULL;
+		goto out;
+	}
 	kkey = _copy_key_from_user(ktp.key, ktp.keylen);
 	if (IS_ERR(kkey)) {
-		kfree(apqns);
-		return PTR_ERR(kkey);
+		rc = PTR_ERR(kkey);
+		kkey = NULL;
+		goto out;
 	}
 	ktp.protkey.len = sizeof(ktp.protkey.protkey);
 	rc = key2protkey(apqns, ktp.apqn_entries, kkey, ktp.keylen,
 			 ktp.protkey.protkey, &ktp.protkey.len,
 			 &ktp.protkey.type, 0);
 	pr_debug("key2protkey()=%d\n", rc);
-	kfree(apqns);
-	kfree_sensitive(kkey);
-	if (!rc && copy_to_user(utp, &ktp, sizeof(ktp)))
+	if (rc)
+		goto out;
+	if (copy_to_user(utp, &ktp, sizeof(ktp)))
 		rc = -EFAULT;
-	memzero_explicit(&ktp, sizeof(ktp));
 
+out:
+	memzero_explicit(&ktp, sizeof(ktp));
+	kfree_sensitive(kkey);
+	kfree(apqns);
 	return rc;
 }
 
@@ -560,7 +581,7 @@ static int pkey_ioctl_apqns4k(struct pkey_apqns4key __user *uak)
 	struct pkey_apqn *apqns = NULL;
 	struct pkey_apqns4key kak;
 	size_t nr_apqns, len;
-	u8 *kkey;
+	u8 *kkey = NULL;
 	int rc;
 
 	if (copy_from_user(&kak, uak, sizeof(kak)))
@@ -568,40 +589,42 @@ static int pkey_ioctl_apqns4k(struct pkey_apqns4key __user *uak)
 	nr_apqns = kak.apqn_entries;
 	if (nr_apqns) {
 		apqns = kmalloc_objs(struct pkey_apqn, nr_apqns);
-		if (!apqns)
-			return -ENOMEM;
+		if (!apqns) {
+			rc = -ENOMEM;
+			goto out;
+		}
 	}
 	kkey = _copy_key_from_user(kak.key, kak.keylen);
 	if (IS_ERR(kkey)) {
-		kfree(apqns);
-		return PTR_ERR(kkey);
+		rc = PTR_ERR(kkey);
+		kkey = NULL;
+		goto out;
 	}
 	rc = pkey_handler_apqns_for_key(kkey, kak.keylen, kak.flags,
 					apqns, &nr_apqns, 0);
 	pr_debug("apqns_for_key()=%d\n", rc);
-	kfree_sensitive(kkey);
-	if (rc && rc != -ENOSPC) {
-		kfree(apqns);
-		return rc;
-	}
+	if (rc && rc != -ENOSPC)
+		goto out;
 	if (!rc && kak.apqns) {
 		if (nr_apqns > kak.apqn_entries) {
-			kfree(apqns);
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 		len = nr_apqns * sizeof(struct pkey_apqn);
 		if (len) {
 			if (copy_to_user(kak.apqns, apqns, len)) {
-				kfree(apqns);
-				return -EFAULT;
+				rc = -EFAULT;
+				goto out;
 			}
 		}
 	}
 	kak.apqn_entries = nr_apqns;
 	if (copy_to_user(uak, &kak, sizeof(kak)))
 		rc = -EFAULT;
-	kfree(apqns);
 
+out:
+	kfree_sensitive(kkey);
+	kfree(apqns);
 	return rc;
 }
 
@@ -617,87 +640,90 @@ static int pkey_ioctl_apqns4kt(struct pkey_apqns4keytype __user *uat)
 	nr_apqns = kat.apqn_entries;
 	if (nr_apqns) {
 		apqns = kmalloc_objs(struct pkey_apqn, nr_apqns);
-		if (!apqns)
-			return -ENOMEM;
+		if (!apqns) {
+			rc = -ENOMEM;
+			goto out;
+		}
 	}
 	rc = pkey_handler_apqns_for_keytype(kat.type,
 					    kat.cur_mkvp, kat.alt_mkvp,
 					    kat.flags, apqns, &nr_apqns, 0);
 	pr_debug("apqns_for_keytype()=%d\n", rc);
-	if (rc && rc != -ENOSPC) {
-		kfree(apqns);
-		return rc;
-	}
+	if (rc && rc != -ENOSPC)
+		goto out;
 	if (!rc && kat.apqns) {
 		if (nr_apqns > kat.apqn_entries) {
-			kfree(apqns);
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 		len = nr_apqns * sizeof(struct pkey_apqn);
 		if (len) {
 			if (copy_to_user(kat.apqns, apqns, len)) {
-				kfree(apqns);
-				return -EFAULT;
+				rc = -EFAULT;
+				goto out;
 			}
 		}
 	}
 	kat.apqn_entries = nr_apqns;
 	if (copy_to_user(uat, &kat, sizeof(kat)))
 		rc = -EFAULT;
-	kfree(apqns);
 
+out:
+	kfree(apqns);
 	return rc;
 }
 
 static int pkey_ioctl_kblob2protk3(struct pkey_kblob2pkey3 __user *utp)
 {
 	u32 protkeylen = PROTKEYBLOBBUFSIZE;
+	u8 *kkey = NULL, *protkey = NULL;
 	struct pkey_apqn *apqns = NULL;
 	struct pkey_kblob2pkey3 ktp;
-	u8 *kkey, *protkey;
 	int rc;
 
 	if (copy_from_user(&ktp, utp, sizeof(ktp)))
 		return -EFAULT;
 	apqns = _copy_apqns_from_user(ktp.apqns, ktp.apqn_entries);
-	if (IS_ERR(apqns))
-		return PTR_ERR(apqns);
+	if (IS_ERR(apqns)) {
+		rc = PTR_ERR(apqns);
+		apqns = NULL;
+		goto out;
+	}
 	kkey = _copy_key_from_user(ktp.key, ktp.keylen);
 	if (IS_ERR(kkey)) {
-		kfree(apqns);
-		return PTR_ERR(kkey);
+		rc = PTR_ERR(kkey);
+		kkey = NULL;
+		goto out;
 	}
 	protkey = kmalloc(protkeylen, GFP_KERNEL);
 	if (!protkey) {
-		kfree(apqns);
-		kfree_sensitive(kkey);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto out;
 	}
 	rc = key2protkey(apqns, ktp.apqn_entries, kkey, ktp.keylen,
 			 protkey, &protkeylen, &ktp.pkeytype, 0);
 	pr_debug("key2protkey()=%d\n", rc);
-	kfree(apqns);
-	kfree_sensitive(kkey);
-	if (rc) {
-		kfree_sensitive(protkey);
-		return rc;
-	}
+	if (rc)
+		goto out;
 	if (ktp.pkey && ktp.pkeylen) {
 		if (protkeylen > ktp.pkeylen) {
-			kfree_sensitive(protkey);
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 		if (copy_to_user(ktp.pkey, protkey, protkeylen)) {
-			kfree_sensitive(protkey);
-			return -EFAULT;
+			rc = -EFAULT;
+			goto out;
 		}
 	}
-	kfree_sensitive(protkey);
 	ktp.pkeylen = protkeylen;
 	if (copy_to_user(utp, &ktp, sizeof(ktp)))
-		return -EFAULT;
+		rc = -EFAULT;
 
-	return 0;
+out:
+	kfree_sensitive(protkey);
+	kfree_sensitive(kkey);
+	kfree(apqns);
+	return rc;
 }
 
 static long pkey_unlocked_ioctl(struct file *filp, unsigned int cmd,
-- 
2.43.0

Re: [PATCH v5 1/1] s390/pkey: Rework ioctl functions error paths

[sashiko-bot]

> With the pkey rework there was the suggestion to rework the error and
> free paths of the pkey ioctl functions. The complain was especially to
> rewrite the failure handling with goto instead of all repeat the
> nearly same code (kfree(), kfree_sensitive(), memzero_explicit()) for
> each path. This patch removes all this duplicated code and introduces
> one code block at the end of the functions which is jumped into via
> goto out or executed on regular exit. As some helper functions return
> an error pointer value (which is NOT NULL) make sure on the error path
> there is not by accident kfree() or similar called on such ptr values.
> 
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722142947.109588-2-freude@linux.ibm.com?part=1