Re: [PATCH v3 6/6] 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: missing release() in wrapkey_ktype leaks wrapkey_kobj
- [Medium] pseries/plpks: stale var.policy across iterations in list_key_labels()
- [Low] powerpc/pseries: wrapkey_sysfs_init() runs on non-pseries platforms
--

commit 7bc0ceea6c99f35ed770f462297146f04fc44fd3
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..efae08e899b66
> --- /dev/null
> +++ b/arch/powerpc/platforms/pseries/plpks-wrapkey-sysfs.c
> @@ -0,0 +1,399 @@

[ ... ]

> +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);
> +		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;
> +}

[Severity: Medium]
Can a stale var.policy value carry over across iterations of the loop in
list_key_labels()?

In list_key_labels(), struct plpks_var var is zero-initialized once before
the loop, and each iteration calls plpks_is_wrapping_key_revoked(&var) ->
plpks_read_var(PLPKS_OS_OWNER, &var) without resetting var.policy.

When plpks_read_var() succeeds in retrieving a wrapping key's policy on
one iteration, it sets var->policy (including PLPKS_WRAPPINGKEY and
possibly PLPKS_REVOKED). If a subsequent iteration fails with -EPERM and
retbuf[1] == 0:

arch/powerpc/platforms/pseries/plpks.c:plpks_read_var() {
	if (rc != H_SUCCESS) {
		rc = pseries_status_to_err(rc);
		if (rc != -EPERM || !retbuf[1])
			goto out_free_output;
		goto out_copy_policy;
	}
}

var->policy is not overwritten. Then plpks_is_wrapping_key_revoked()
inspects the stale var->policy from the previous key:

arch/powerpc/platforms/pseries/plpks.c:plpks_is_wrapping_key_revoked() {
	} else if (rc == -EPERM) {
		if (var->policy & PLPKS_WRAPPINGKEY) {
			if (var->policy & PLPKS_REVOKED)
				rc = 1;
			else
				rc = 0;
		}
	}
}

Would this cause plpks_is_wrapping_key_revoked() to return the previous
key's revocation status (0 or 1) instead of -EPERM?

[ ... ]

> +static const struct kobj_type wrapkey_ktype = {
> +	.sysfs_ops	= &kobj_sysfs_ops,
> +	.default_groups = wrapkey_attr_groups,
> +};
> +
> +static __init int wrapkey_sysfs_init(void)
> +{
> +	int rc;
> +
> +	if (!plpks_revoke_is_supported()) {
> +		pr_err("H_PKS_{UN}REVOKE_OBJECT interface not supported\n");
> +		return -ENODEV;
> +	}
> +
> +	wrapkey_kobj = kzalloc_obj(*wrapkey_kobj);
> +	if (!wrapkey_kobj)
> +		return -ENOMEM;
> +
> +	rc = plpks_init_child_kobj(wrapkey_kobj, &wrapkey_ktype, "wrapkey");
> +
> +	if (rc)
> +		kobject_put(wrapkey_kobj);
> +
> +	return rc;
> +}

[Severity: Medium]
Does calling kobject_put(wrapkey_kobj) leak wrapkey_kobj and trigger a
kobject warning because wrapkey_ktype has no .release callback?

In wrapkey_sysfs_init(), wrapkey_kobj is dynamically allocated with
kzalloc_obj(*wrapkey_kobj) and initialized via plpks_init_child_kobj() ->
kobject_init(kobj, &wrapkey_ktype).

If plpks_init_child_kobj() fails (for example, when plpks_kobj is NULL or
kobject_add() returns an error), wrapkey_sysfs_init() calls
kobject_put(wrapkey_kobj). Because wrapkey_ktype omits .release,
kobject_cleanup() will not free wrapkey_kobj, and wrapkey_kobj also
remains non-NULL.

> +
> +late_initcall(wrapkey_sysfs_init);

[Severity: Low]
Should this use machine_late_initcall(pseries, wrapkey_sysfs_init) instead
of late_initcall(wrapkey_sysfs_init)?

Other PLPKS initcalls (such as pseries_plpks_init() in plpks.c and
plpks_sysfs_init() in plpks-sysfs.c) use machine_*_initcall(pseries, ...).
With a bare late_initcall(), booting a multi-platform powerpc kernel on a
non-pseries machine will run wrapkey_sysfs_init() and log:

  wrapkey-sysfs: H_PKS_{UN}REVOKE_OBJECT interface not supported

[]

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