Re: [PATCH v6 9/9] pseries/plpks/wrapkey: expose PKWM wrapping key management to userspace via sysfs

[sashiko-bot@kernel.org]

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 <ssrish@linux.ibm.com>

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.

> diff --git a/arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c b/arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c
> new file mode 100644
> index 0000000000000..c293196cc9adf
> --- /dev/null
> +++ b/arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c
[ ... ]
> +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);
> +
> +		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?

> +		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)
> +{
[ ... ]
> +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) {
> +		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?

> +
> +static ssize_t delete_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			    const char *buf, size_t count)
> +{
[ ... ]
> +	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105144320.357260-1-ssrish@linux.ibm.com?part=9