Extend PKWM to support user-created wrapping keys

plpks_gen_wrapping_key() returns a negative errno when default wrapping key generation fails. However, trusted_pkwm_init() returns -EINVAL for all such failures, discarding the original error.

Propagate the error returned by plpks_gen_wrapping_key() unchanged.

Fixes c99fcb0d735b ("keys/trusted_keys: establish PKWM as a trusted source")

Cc [email protected] # 7.0

Signed-off-by Srish Srinivasan <[email protected]>

Tested-by R Nageswara Sastry <[email protected]>

---
 security/keys/trusted-keys/trusted_pkwm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/keys/trusted-keys/trusted_pkwm.c +1 -1

index bf42c6679245..422f752598e1 100644
@@ -170,7 +170,7 @@ static int trusted_pkwm_init(void)
 	ret = plpks_gen_wrapping_key();
 	if (ret) {
 		pr_err("Failed to generate default wrapping key\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	return register_key_type(&key_type_trusted);

Signature · 1 line

2.52.0

Quoted 4 lines · Srish Srinivasan

plpks_gen_wrapping_key() returns a negative errno when default wrapping key generation fails. However, trusted_pkwm_init() returns -EINVAL for all such failures, discarding the original error.

Quoted 6 lines

Propagate the error returned by plpks_gen_wrapping_key() unchanged.

Fixes c99fcb0d735b ("keys/trusted_keys: establish PKWM as a trusted source")

Cc [email protected] # 7.0

Signed-off-by Srish Srinivasan <[email protected]>

Tested-by R Nageswara Sastry <[email protected]>

Nit: I'd put the SOB as last.

Quoted 17 lines

---
  security/keys/trusted-keys/trusted_pkwm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

security/keys/trusted-keys/trusted_pkwm.c +1 -1

index bf42c6679245..422f752598e1 100644
@@ -170,7 +170,7 @@ static int trusted_pkwm_init(void)
  	ret = plpks_gen_wrapping_key();
  	if (ret) {
  		pr_err("Failed to generate default wrapping key\n");
-		return -EINVAL;
+		return ret;
  	}
 

Quoted 4 lines

security/keys/trusted-keys/trusted_pkwm.c +0 -0

  	return register_key_type(&key_type_trusted);

Signature · 1 line

2.52.0

Reviewed-by Jarkko Sakkinen <[email protected]>

Br, Jarkko

The PKWM seal and unseal paths unconditionally free their output buffers, but the PLPKS helpers can return without initializing those pointers.

Initialize the output buffer pointers to NULL to make cleanup safe on early failure paths.

Fixes c99fcb0d735b ("keys/trusted_keys: establish PKWM as a trusted source")

Cc [email protected] # 7.0

Signed-off-by Srish Srinivasan <[email protected]>

---
 security/keys/trusted-keys/trusted_pkwm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

security/keys/trusted-keys/trusted_pkwm.c +2 -2

index 422f752598e1..97ffec0ce1b7 100644
@@ -82,7 +82,7 @@ static int trusted_pkwm_seal(struct trusted_key_payload *p, char *datablob)
 {
 	struct trusted_key_options *options = NULL;
 	struct trusted_pkwm_options *pkwm = NULL;
-	u8 *input_buf, *output_buf;
+	u8 *input_buf, *output_buf = NULL;
 	u32 output_len, input_len;
 	int rc;
 

@@ -129,7 +129,7 @@ static int trusted_pkwm_seal(struct trusted_key_payload *p, char *datablob)
 
 static int trusted_pkwm_unseal(struct trusted_key_payload *p, char *datablob)
 {
-	u8 *input_buf, *output_buf;
+	u8 *input_buf, *output_buf = NULL;
 	u32 input_len, output_len;
 	int rc;
 

Signature · 1 line

2.52.0

Quoted 35 lines · Srish Srinivasan

The PKWM seal and unseal paths unconditionally free their output buffers, but the PLPKS helpers can return without initializing those pointers.

Initialize the output buffer pointers to NULL to make cleanup safe on early failure paths.

Fixes c99fcb0d735b ("keys/trusted_keys: establish PKWM as a trusted source")

Cc [email protected] # 7.0

Signed-off-by Srish Srinivasan <[email protected]>

---
  security/keys/trusted-keys/trusted_pkwm.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

security/keys/trusted-keys/trusted_pkwm.c +2 -2

index 422f752598e1..97ffec0ce1b7 100644
@@ -82,7 +82,7 @@ static int trusted_pkwm_seal(struct trusted_key_payload *p, char *datablob)
  {
  	struct trusted_key_options *options = NULL;
  	struct trusted_pkwm_options *pkwm = NULL;
-	u8 *input_buf, *output_buf;
+	u8 *input_buf, *output_buf = NULL;
  	u32 output_len, input_len;
  	int rc;
 

@@ -129,7 +129,7 @@ static int trusted_pkwm_seal(struct trusted_key_payload *p, char *datablob)
 
  static int trusted_pkwm_unseal(struct trusted_key_payload *p, char *datablob)
  {
-	u8 *input_buf, *output_buf;
+	u8 *input_buf, *output_buf = NULL;
  	u32 input_len, output_len;
  	int rc;
 

Quoted 3 lines

Signature · 1 line

2.52.0

Reviewed-by Jarkko Sakkinen <[email protected]>

Br, Jarkko

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pseries/plpks: output_buf leak and -EAGAIN return in plpks_get_object_labels() --

commit 41ef08b282e00765eba284dad461f1985057d371
Author: Srish Srinivasan <[email protected]>

pseries/plpks: add HCALLs for PKWM wrapping key life cycle management

This commit adds support for the H_PKS_REVOKE_OBJECT, H_PKS_UNREVOKE_OBJECT, and H_PKS_GET_OBJECTLABELS hypercalls to manage the life cycle of PKWM wrapping keys and retrieve wrapping key labels in the Platform Keystore.

Quoted 4 lines

arch/powerpc/platforms/pseries/plpks.c +0 -0

index d5adc563f71f0..6e10bd6a9a481 100644

[ ... ]

Quoted 1 line

arch/powerpc/platforms/pseries/plpks.c +0 -0

@@ -1205,6 +1237,457 @@ int plpks_unwrap_object(u8 **input_buf, u64 input_len, u8 **output_buf,

[ ... ]

Quoted 133 lines

arch/powerpc/platforms/pseries/plpks.c +133 -0

+int plpks_get_object_labels(u8 **output_buf, u64 *output_len,
+			    char *comp_prefix)
+{
+	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
+	u8 *labels_buf = NULL;
+	u8 *tmp_buf = NULL;
+	struct plpks_auth *auth;
+	struct label_attr *metadata = NULL;
+	u8 prefix[PLPKS_OBJLABEL_PREFIX_LEN];
+	u16 label_len;
+	u64 labels_count;
+	u64 continuetoken = 0, output_buf_len = 0;
+	int rc, pseries_status;
+	size_t labels_buf_offset = 0, output_buf_offset = 0;
+	size_t obj_label_entry_size, i;
+
+	if (!output_buf || !output_len || !comp_prefix)
+		return -EINVAL;
+
+	*output_buf = NULL;
+	*output_len = 0;
+
+	memset(prefix, 0, sizeof(prefix));
+	memcpy(prefix, comp_prefix, strnlen(comp_prefix, sizeof(prefix)));
+
+	auth = construct_auth(PLPKS_OS_OWNER);
+	if (IS_ERR(auth))
+		return PTR_ERR(auth);
+
+	do {
+		labels_buf =
+			kzalloc(PLPKS_OBJLABEL_BUF_SIZE, GFP_KERNEL);
+
+		if (!labels_buf) {
+			rc = -ENOMEM;
+			goto out_free_output_buf;
+		}
+
+		rc = plpar_hcall(H_PKS_GET_OBJECTLABELS, retbuf,
+				 virt_to_phys(auth), continuetoken,
+				 virt_to_phys(labels_buf),
+				 PLPKS_OBJLABEL_BUF_SIZE);
+
+		pseries_status = rc;
+		rc = pseries_status_to_err(rc);
+
+		if (rc && rc != -EAGAIN) {
+			pr_err("H_PKS_GET_OBJECTLABELS failed. pseries_status=%d rc=%d\n",
+			       pseries_status, rc);
+			goto out_free_labels_buf;
+		} else {
+			/*
+			 * Setting an incorrect countinuetoken upon
+			 * receiving H_CONTINUE would result in H_P2. Since
+			 * the continuetoken is being set to the expected
+			 * value from the previous call, H_P2 must not be
+			 * returned.
+			 */
+			continuetoken = retbuf[1];
+
+			labels_count = retbuf[0];
+			if (!labels_count) {
+				kfree(labels_buf);
+				labels_buf = NULL;
+				goto out;
+			}
+
+			/*
+			 * Filter out object labels that don't have the provided
+			 * component prefix.
+			 */
+
+			output_buf_len += PLPKS_OBJLABEL_BUF_SIZE;
+
+			tmp_buf = krealloc(*output_buf, output_buf_len,
+					   GFP_KERNEL);
+
+			if (!tmp_buf) {
+				rc = -ENOMEM;
+				goto out_free_labels_buf;
+			}
+
+			*output_buf = tmp_buf;
+
+			for (i = 0; i < labels_count; ++i) {
+				if (labels_buf_offset +
+				    PLPKS_OBJLABEL_LEN_FIELD_SIZE >
+				    PLPKS_OBJLABEL_BUF_SIZE) {
+					rc = -EIO;
+					goto out_free_labels_buf;
+				}
+
+				label_len =
+					get_unaligned_be16(labels_buf +
+							   labels_buf_offset);
+
+				obj_label_entry_size =
+					PLPKS_OBJLABEL_LEN_FIELD_SIZE +
+					label_len;
+
+				if (label_len < sizeof(struct label_attr) ||
+				    labels_buf_offset + obj_label_entry_size >
+				    PLPKS_OBJLABEL_BUF_SIZE) {
+					rc = -EIO;
+					goto out_free_labels_buf;
+				}
+
+				metadata =
+					(struct label_attr *)(labels_buf +
+					labels_buf_offset +
+					PLPKS_OBJLABEL_LEN_FIELD_SIZE);
+
+				if (!memcmp(metadata->prefix, prefix,
+					    PLPKS_OBJLABEL_PREFIX_LEN)) {
+					memcpy(*output_buf + output_buf_offset,
+					       labels_buf + labels_buf_offset,
+					       obj_label_entry_size);
+
+					output_buf_offset +=
+						obj_label_entry_size;
+					(*output_len) += 1;
+				}
+				labels_buf_offset += obj_label_entry_size;
+			}
+			kfree(labels_buf);
+			labels_buf = NULL;
+			labels_buf_offset = 0;
+		}
+	} while (rc == -EAGAIN);
+
+	goto out;
+
+out_free_labels_buf:

arch/powerpc/platforms/pseries/plpks.c int plpks_unwrap_object(u8 **input_buf, u64 input_len, u8 **output_buf, · line 1688

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks.c +10 -0

+	kfree(labels_buf);
+	labels_buf = NULL;
+out_free_output_buf:
+	kfree(*output_buf);
+	*output_buf = NULL;
+	*output_len = 0;
+out:
+	kfree_sensitive(auth);
+	return rc;
+}

[Severity: Medium]
Can plpks_get_object_labels() leak *output_buf or return -EAGAIN to the caller?

First, inside the do-while loop, when labels_count > 0, *output_buf is allocated or grown via krealloc() before checking whether any of the returned labels match comp_prefix. If PLPKS contains objects for other components so labels_count > 0, but none of those labels match comp_prefix, the loop completes with rc == 0 and *output_len == 0 and jumps to out without freeing *output_buf. Unlike the case where PLPKS has 0 total objects on the first call (which returns 0 with *output_len == 0 and *output_buf == NULL), this returns 0 with *output_len == 0 while *output_buf points to an allocated buffer of uninitialized heap memory. Callers checking *output_len == 0 will leak *output_buf, while callers checking *output_buf != NULL will read uninitialized memory.

Second, if !labels_count (retbuf[0] == 0) is encountered when H_PKS_GET_OBJECTLABELS returns H_CONTINUE (converted by pseries_status_to_err() to rc = -EAGAIN) or on a subsequent continuation iteration after *output_buf was already allocated, jumping directly to out bypasses the loop condition and out_free_output_buf. Could that return the internal continuation code -EAGAIN to the caller and leak any *output_buf allocated in prior iterations?

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] pseries/plpks: label truncation and undocumented format in list_key_labels()
- [Medium] pseries/plpks: TOCTOU race between delete_store() and unrevoke_store()
- [Low] pseries/plpks: duplicated sysfs store/show logic in plpks-wrapkey-sysfs.c --

commit 58203a02dbc210e21378df2545831a7ea49bbca4
Author: Srish Srinivasan <[email protected]>

pseries/plpks/wrapkey: expose PKWM wrapping key management to userspace via sysfs

This commit adds a sysfs interface under /sys/firmware/plpks/wrapkey to allow userspace to create, revoke, unrevoke, delete, and list PKWM wrapping keys.

Quoted 5 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +0 -0

new file mode 100644
index 0000000000000..c293196cc9adf

[ ... ]

Quoted 23 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +23 -0

+static ssize_t list_key_labels(char *buf, u8 *obj_labels_buf,
+			       u64 obj_labels_count, int revoked)
+{
+	struct plpks_var var = { 0 };
+	u8 *obj_labels_buf_ptr, *comp_prefix;
+	u8 *key_label_ptr;
+	u16 obj_label_len;
+	int len = 0;
+	int is_revoked;
+	u64 i;
+
+	var.os = PLPKS_VAR_LINUX;
+	var.component = PLPKS_WRAPKEY_COMPONENT;
+	obj_labels_buf_ptr = obj_labels_buf;
+
+	for (i = 0; i < obj_labels_count; ++i) {
+		obj_label_len = get_unaligned_be16(obj_labels_buf_ptr);
+		comp_prefix = obj_labels_buf_ptr +
+			      PLPKS_OBJLABEL_LEN_FIELD_SIZE;
+
+		var.namelen = obj_label_len - PLPKS_MAX_LABEL_ATTR_SIZE;
+		var.name = kzalloc(var.namelen + 1, GFP_KERNEL);
+

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c line 71

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +10 -0

+		if (!var.name)
+			return -ENOMEM;
+
+		key_label_ptr = comp_prefix + PLPKS_MAX_LABEL_ATTR_SIZE;
+		memcpy(var.name, key_label_ptr, var.namelen);
+
+		is_revoked = plpks_is_wrapping_key_revoked(&var);
+		if (is_revoked == revoked)
+			len += sysfs_emit_at(buf, len, "<%s>\n",
+					     (char *)var.name);

[Severity: Medium]
Can list_key_labels() silently truncate wrapping key labels when reading /sys/firmware/plpks/wrapkey/list_active or list_revoked?

plpks_get_object_labels() can retrieve labels across multiple 4KB buffers via H_CONTINUE, and each label can be up to PLPKS_MAX_NAME_SIZE (239) bytes. However, a standard kobj_attribute show buffer is limited to PAGE_SIZE - 1 bytes. Once len reaches PAGE_SIZE - 1, sysfs_emit_at() returns 0 and any remaining wrapping key labels are silently omitted without any error or indication to userspace.

Also, list_key_labels() formats each label wrapped in angle brackets ("<%s>\n"), whereas Documentation/ABI/testing/sysfs-firmware-plpks does not mention the angle-bracket format for list_active and list_revoked. Should the output format and buffer limit be addressed or documented?

Quoted 58 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +58 -0

+		else if (is_revoked < 0)
+			pr_warn("Failed to get revocation status for <%s>\n",
+				(char *)var.name);
+
+		kfree(var.name);
+		obj_labels_buf_ptr = key_label_ptr + var.namelen;
+	}
+
+	return len;
+}
+
+static ssize_t list_active_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	u8 *obj_labels_buf = NULL;
+	u64 obj_labels_count = 0;
+	int rc;
+
+	rc = plpks_get_object_labels(&obj_labels_buf, &obj_labels_count,
+				     PLPKS_WRAPKEY_COMPONENT);
+	if (rc) {
+		pr_err("Retrieving object labels failed. rc=%d\n", rc);
+		goto out;
+	}
+
+	rc = list_key_labels(buf, obj_labels_buf, obj_labels_count,
+			     PLPKS_WRAPKEY_UNREVOKED);
+
+out:
+	kfree(obj_labels_buf);
+	return rc;
+}
+
+static ssize_t list_revoked_show(struct kobject *kobj,
+				 struct kobj_attribute *attr, char *buf)
+{
+	u8 *obj_labels_buf = NULL;
+	u64 obj_labels_count = 0;
+	int rc;
+
+	rc = plpks_get_object_labels(&obj_labels_buf, &obj_labels_count,
+				     PLPKS_WRAPKEY_COMPONENT);
+	if (rc) {
+		pr_err("Retrieving object labels failed. rc=%d\n", rc);
+		goto out;
+	}
+
+	rc = list_key_labels(buf, obj_labels_buf, obj_labels_count,
+			     PLPKS_WRAPKEY_REVOKED);
+
+out:
+	kfree(obj_labels_buf);
+	return rc;
+}
+
+static ssize_t create_store(struct kobject *kobj, struct kobj_attribute *attr,
+			    const char *buf, size_t count)
+{

[ ... ]

Quoted 44 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +44 -0

+static ssize_t unrevoke_store(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct plpks_var var = { 0 };
+	int rc;
+
+	if (!capable(CAP_SYS_ADMIN)) {
+		rc = -EPERM;
+		goto out;
+	}
+
+	if (!count) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (count > PLPKS_MAX_NAME_SIZE) {
+		rc = -ENAMETOOLONG;
+		goto out;
+	}
+
+	if (!strcmp(buf, PLPKS_DEFAULT_WRAPKEY_LABEL)) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (!is_wrapkey_label_alnum(buf, count)) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	var.name = kstrndup(buf, count, GFP_KERNEL);
+	if (!var.name) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	var.namelen = count;
+	var.os = PLPKS_VAR_LINUX;
+	var.component = PLPKS_WRAPKEY_COMPONENT;
+
+	rc = plpks_unrevoke_wrapping_key(&var);
+
+	if (rc) {

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c line 290

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +10 -0

+		pr_err("un-revocation of wrapping key <%s> failed. rc = %d\n",
+		       (char *)var.name, rc);
+		goto out;
+	}
+
+	rc = count;
+out:
+	kfree(var.name);
+	return rc;
+}

[Severity: Low]
This isn't a bug, but create_store(), revoke_store(), unrevoke_store(), and delete_store() all duplicate the same CAP_SYS_ADMIN check, count validation, PLPKS_DEFAULT_WRAPKEY_LABEL check, is_wrapkey_label_alnum() validation, kstrndup() allocation, and struct plpks_var initialization (with unrevoke_store() returning -EINVAL instead of -EPERM for PLPKS_DEFAULT_WRAPKEY_LABEL).

Similarly, list_active_show() and list_revoked_show() duplicate the plpks_get_object_labels() call and cleanup, differing only in passing PLPKS_WRAPKEY_UNREVOKED vs PLPKS_WRAPKEY_REVOKED to list_key_labels().

Could the shared store validation/setup and show label retrieval logic be factored into common helpers?

Quoted 4 lines

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +4 -0

+
+static ssize_t delete_store(struct kobject *kobj, struct kobj_attribute *attr,
+			    const char *buf, size_t count)
+{

[ ... ]

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c line 333

Quoted 1 line

arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c new file +1 -0

+	rc = plpks_del_wrapping_key(&var);

[Severity: Medium]
What prevents a TOCTOU race if delete_store() and unrevoke_store() are called concurrently on the same revoked wrapping key?

In arch/powerpc/platforms/pseries/plpks.c, plpks_del_wrapping_key() enforces that a key is revoked before deletion across two separate hypercalls without any lock:

plpks_del_wrapping_key()

     rc = plpks_is_wrapping_key_revoked(var);
     if (rc == 1) {
         ...
         rc = plpks_remove_var(PLPKS_WRAPKEY_COMPONENT, var->os, vname);

Because delete and unrevoke are separate sysfs attributes, kernfs only holds its per-open-file mutex separately for each attribute. If CPU 0 enters delete_store() -> plpks_del_wrapping_key() and sees plpks_is_wrapping_key_revoked() == 1, and CPU 1 concurrently runs unrevoke_store() -> plpks_unrevoke_wrapping_key() (H_PKS_UNREVOKE_OBJECT) before CPU 0 calls plpks_remove_var() (H_PKS_REMOVE_OBJECT), won't CPU 0 delete the now-active wrapping key?