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

[Srish Srinivasan <ssrish@linux.ibm.com>]

Hi

On 11/5/26 8:32 PM, sashiko-bot@kernel.org wrote:
> 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
> --


Yes, these are valid findings and I will fix these in my next version.

Thanks,
Srish


>
> 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?
>