Re: [PATCH v3 4/6] pseries/plpks: add HCALLs for PKWM wrapping key life cycle management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pseries/plpks: stale var->policy evaluated in plpks_is_wrapping_key_revoked()
- [Low] powerpc/pseries: papr_hcalls.rst swaps H_PKS_GET_OBJECTLABELS output order
--

commit 73a27f4e02c0030b70181101065300c5db3bc91b
Author: Srish Srinivasan <ssrish@linux.ibm.com>

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

This commit implements the H_PKS_REVOKE_OBJECT, H_PKS_UNREVOKE_OBJECT, and
H_PKS_GET_OBJECTLABELS hypervisor calls in the pseries PLPKS driver to
support revoking, unrevoking, deleting, and listing PKWM wrapping keys.

> diff --git a/Documentation/arch/powerpc/papr_hcalls.rst b/Documentation/arch/powerpc/papr_hcalls.rst
> index 44c9c8b32ae3c..3455b403a0486 100644
> --- a/Documentation/arch/powerpc/papr_hcalls.rst
> +++ b/Documentation/arch/powerpc/papr_hcalls.rst
> @@ -343,6 +343,39 @@ is returned to the caller.
[ ... ]
> +**H_PKS_GET_OBJECTLABELS**
> +
> +| Input: authorization, continueToken, out, outlen
> +| Out: *continue-token, number of object labels in the returned list, object
> +        label list*
> +| Return Value: *H_Success, H_Function, H_State, H_Parameter, H_P2, H_P3, H_P4,
> +                H_Authority, H_Busy, H_Aborted, H_Continue*
> +
> +H_PKS_GET_OBJECTLABELS is used to retrieve a list of object labels owned by the
> +specified consumer.

[Severity: Low]
Are the first two output fields for H_PKS_GET_OBJECTLABELS in
Documentation/arch/powerpc/papr_hcalls.rst swapped relative to the output
registers read in plpks_get_object_labels()?

In Documentation/arch/powerpc/papr_hcalls.rst, the Out field lists output
registers in order starting from r4 (retbuf[0]) and r5 (retbuf[1]), which
here places continue-token before the number of object labels.

However, in arch/powerpc/platforms/pseries/plpks.c:plpks_get_object_labels(),
retbuf[0] is read as labels_count and retbuf[1] is read as continuetoken:

    continuetoken = retbuf[1];

    labels_count = retbuf[0];

[ ... ]
> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index a7953db3f23a6..edd189cfcc85c 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
[ ... ]
> @@ -831,9 +846,6 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
>  	if (var->namelen > PLPKS_MAX_NAME_SIZE)
>  		return -EINVAL;
>  
> -	if (var->policy & PLPKS_WRAPPINGKEY)
> -		return -EPERM;
> -
>  	auth = construct_auth(consumer);
>  	if (IS_ERR(auth))
>  		return PTR_ERR(auth);
[ ... ]
> @@ -1189,6 +1218,419 @@ int plpks_unwrap_object(u8 **input_buf, u64 input_len, u8 **output_buf,
[ ... ]
> +int plpks_is_wrapping_key_revoked(struct plpks_var *var)
> +{
> +	int rc;
> +
> +	if (!var || !var->name || !*var->name)
> +		return -EINVAL;
> +
> +	rc = plpks_read_var(PLPKS_OS_OWNER, var);
> +	if (!rc) {
> +		/*
> +		 * Reading a wrapping key is expected to fail with
> +		 * -EPERM and return its policy.
> +		 */
> +		rc = -EIO;
> +	} else if (rc == -EPERM) {
> +		if (var->policy & PLPKS_WRAPPINGKEY) {
> +			if (var->policy & PLPKS_REVOKED)
> +				rc = 1;
> +			else
> +				rc = 0;
> +		}
> +	}
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(plpks_is_wrapping_key_revoked);

[Severity: Medium]
Can plpks_is_wrapping_key_revoked() evaluate a stale var->policy when
H_PKS_READ_OBJECT fails with H_AUTHORITY and retbuf[1] is 0, or overwrite
var->datalen and var->data if H_PKS_READ_OBJECT succeeds?

In arch/powerpc/platforms/pseries/plpks.c:plpks_read_var(), when
H_PKS_READ_OBJECT returns H_AUTHORITY (mapped to -EPERM), var->policy is only
updated if retbuf[1] is non-zero:

    if (rc != H_SUCCESS) {
        rc = pseries_status_to_err(rc);
        if (rc != -EPERM || !retbuf[1])
            goto out_free_output;
        goto out_copy_policy;
    }

Because plpks_is_wrapping_key_revoked() does not zero var->policy before
calling plpks_read_var(), an H_AUTHORITY failure with retbuf[1] == 0 leaves
var->policy untouched and plpks_is_wrapping_key_revoked() then checks the
pre-existing value in var->policy.

In the subsequent commit "pseries/plpks/wrapkey: expose PKWM wrapping key
management to userspace via sysfs", list_key_labels() defines a single
struct plpks_var var = {0} outside the loop and calls
plpks_is_wrapping_key_revoked(&var) across all labels without resetting
var.policy between iterations. Once an earlier iteration populates var.policy
with PLPKS_WRAPPINGKEY, any later iteration where H_PKS_READ_OBJECT returns
H_AUTHORITY with retbuf[1] == 0 will inspect the previous key's var.policy
and return 0 or 1 instead of -EPERM. The same happens if a caller of
plpks_is_wrapping_key_revoked() or plpks_del_wrapping_key() passes a
struct plpks_var with PLPKS_WRAPPINGKEY already set in var->policy.

Additionally, if H_PKS_READ_OBJECT unexpectedly succeeds on a readable
object, plpks_read_var() will overwrite var->datalen and copy into var->data
(if non-NULL) before plpks_is_wrapping_key_revoked() returns -EIO.

[ ... ]

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