Linux s390 Architecture development
 help / color / mirror / Atom feed
* [PATCH v2 0/1] Rework pkey ioctl functions error pathes
@ 2026-07-03  9:46 Harald Freudenberger
  2026-07-03  9:46 ` [PATCH v2 1/1] s390/pkey: Rework " Harald Freudenberger
  0 siblings, 1 reply; 3+ messages in thread
From: Harald Freudenberger @ 2026-07-03  9:46 UTC (permalink / raw)
  To: dengler, fcallies; +Cc: hca, gor, agordeev, linux390-list, linux-s390

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.

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

 drivers/s390/crypto/pkey_api.c | 272 +++++++++++++++++----------------
 1 file changed, 143 insertions(+), 129 deletions(-)

--
2.43.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v2 1/1] s390/pkey: Rework ioctl functions error pathes
  2026-07-03  9:46 [PATCH v2 0/1] Rework pkey ioctl functions error pathes Harald Freudenberger
@ 2026-07-03  9:46 ` Harald Freudenberger
  2026-07-03  9:57   ` sashiko-bot
  0 siblings, 1 reply; 3+ messages in thread
From: Harald Freudenberger @ 2026-07-03  9:46 UTC (permalink / raw)
  To: dengler, fcallies; +Cc: hca, gor, agordeev, linux390-list, linux-s390

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.

Suggested-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/pkey_api.c | 272 +++++++++++++++++----------------
 1 file changed, 143 insertions(+), 129 deletions(-)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 5d8f63f390a8..aece213b84c2 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,41 +362,46 @@ 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);
+		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;
 	int rc;
 	u32 u;
@@ -409,42 +419,41 @@ static int pkey_ioctl_genseck2(struct pkey_genseck2 __user *ugs)
 		return PTR_ERR(apqns);
 	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 +463,44 @@ 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);
+		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 +513,22 @@ 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);
+		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 +536,35 @@ 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);
+		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);
+		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 +573,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 +581,41 @@ 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);
+		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 +631,87 @@ 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);
+		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);
+		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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2 1/1] s390/pkey: Rework ioctl functions error pathes
  2026-07-03  9:46 ` [PATCH v2 1/1] s390/pkey: Rework " Harald Freudenberger
@ 2026-07-03  9:57   ` sashiko-bot
  0 siblings, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-03  9:57 UTC (permalink / raw)
  To: Harald Freudenberger
  Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Passing ERR_PTR values from `_copy_key_from_user` or `_copy_apqns_from_user` to `kfree()` and `kfree_sensitive()` causes a deterministic kernel panic.
- [Critical] Accidental removal of the `ktp.pkeylen` update in `pkey_ioctl_kblob2protk3` breaks the userspace ABI.
--

Note: Due to recitation restrictions, this report does not use the standard
inline quoting format to show the modified code. Instead, I am providing a
free-form summary of the issues found in the patch.

commit 280086862168752a0b3725329891a790006a089e
Author: Harald Freudenberger <freude@linux.ibm.com>
s390/pkey: Rework ioctl functions error pathes

This commit refactors the error handling and free paths in the pkey ioctl
functions. It replaces duplicated cleanup code with centralized goto labels
to handle freeing memory and zeroing operations at the end of each function.


[Severity: Critical]
When _copy_key_from_user() or _copy_apqns_from_user() fails and returns an
ERR_PTR, does the jump to the out label cause kfree() or kfree_sensitive() 
to dereference that ERR_PTR?

In drivers/s390/crypto/pkey_api.c, several ioctl functions check for IS_ERR()
after copying from user space. If an error occurs, they assign rc and then 
execute goto out. However, at the out label, the pointer is unconditionally 
passed to the freeing functions. Passing an ERR_PTR (such as -EFAULT) to the
kfree family causes the slab allocator to attempt dereferencing an invalid 
address, resulting in a kernel panic. 

This pattern appears to be introduced across multiple functions, including
pkey_ioctl_kblob2protk(), pkey_ioctl_clr2seck2(), pkey_ioctl_verifykey2(),
pkey_ioctl_kblob2protk2(), pkey_ioctl_apqns4k(), and
pkey_ioctl_kblob2protk3().


[Severity: Critical]
Was the assignment of ktp.pkeylen intentionally removed in 
pkey_ioctl_kblob2protk3()? 

During the refactoring in drivers/s390/crypto/pkey_api.c, the line 
ktp.pkeylen = protkeylen; was dropped right before the final copy_to_user() 
call. Dropping this assignment prevents the kernel from returning the actual 
size of the derived key back to userspace. Will this break the ABI for 
applications relying on this size query?

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-07-03  9:57 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-03  9:46 [PATCH v2 0/1] Rework pkey ioctl functions error pathes Harald Freudenberger
2026-07-03  9:46 ` [PATCH v2 1/1] s390/pkey: Rework " Harald Freudenberger
2026-07-03  9:57   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox