Re: [PATCH v3 5/6] keys/trusted_keys: enable PKWM wrapping key selection by label

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] keys/trusted_keys: uninitialized output_buf freed in trusted_pkwm_seal()
- [Medium] powerpc/pseries/plpks: double status conversion in plpks_gen_wrapping_key()
--

commit 333e996cc70a34c8f3a006ec261f3d6b0dcd0e1f
Author: Srish Srinivasan <ssrish@linux.ibm.com>

keys/trusted_keys: enable PKWM wrapping key selection by label

This commit updates the PKWM trusted keys backend and pseries PLPKS wrapping
interface to allow callers to specify a wrapping key label via the
wrapping_key=<label> option when sealing trusted keys, falling back to the
default wrapping key if none is specified.

> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index edd189cfcc85c..a4fb5dbac4061 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c

[ ... ]

> @@ -960,25 +958,22 @@ EXPORT_SYMBOL_GPL(plpks_revoke_is_supported);
>   *
>   * Returns: On success 0 is returned, a negative errno if not.
>   */
> -int plpks_gen_wrapping_key(void)
> +int plpks_gen_wrapping_key(struct plpks_var *var)
>  {
>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
>  	struct plpks_auth *auth;
>  	struct label *label;
>  	int rc = 0, pseries_status = 0;
> -	struct plpks_var var = {
> -		.name = PLPKS_DEFAULT_WRAPKEY_LABEL,
> -		.namelen = sizeof(PLPKS_DEFAULT_WRAPKEY_LABEL) - 1,
> -		.policy = PLPKS_WRAPPINGKEY,
> -		.os = PLPKS_VAR_LINUX,
> -		.component = PLPKS_WRAPKEY_COMPONENT
> -	};
> +
> +	if (!var || !var->name || !*var->name)
> +		return -EINVAL;
>  
>  	auth = construct_auth(PLPKS_OS_OWNER);
>  	if (IS_ERR(auth))
>  		return PTR_ERR(auth);
>  
> -	label = construct_label(var.component, var.os, var.name, var.namelen);
> +	label = construct_label(var->component, var->os, var->name,
> +				var->namelen);
>  	if (IS_ERR(label)) {
>  		rc = PTR_ERR(label);
>  		goto out;
> @@ -986,7 +981,7 @@ int plpks_gen_wrapping_key(void)
>  
>  	rc = plpar_hcall(H_PKS_GEN_KEY, retbuf,
>  			 virt_to_phys(auth), virt_to_phys(label),
> -			 label->size, var.policy,
> +			 label->size, var->policy,
>  			 NULL, PLPKS_WRAPPING_KEY_LENGTH);
>  
>  	if (!rc)
> @@ -995,12 +990,9 @@ int plpks_gen_wrapping_key(void)
>  	pseries_status = rc;
>  	rc = pseries_status_to_err(rc);
>  
> -	if (rc && rc != -EEXIST) {
> -		pr_err("H_PKS_GEN_KEY failed. pseries_status=%d, rc=%d",
> +	if (rc && rc != -EEXIST)
> +		pr_err("H_PKS_GEN_KEY failed. pseries_status=%d, rc=%d\n",
>  		       pseries_status, rc);
> -	} else {
> -		rc = 0;
> -	}

[Severity: Medium]
When plpar_hcall(H_PKS_GEN_KEY, ...) returns 0, plpks_gen_wrapping_key()
calls plpks_confirm_object_flushed(label, auth):

arch/powerpc/platforms/pseries/plpks.c:plpks_confirm_object_flushed() {
	...
	if (timed_out)
		return -ETIMEDOUT;

	return pseries_status_to_err(rc);
}

Since plpks_confirm_object_flushed() already converts the hypervisor status
code into a negative Linux errno (or returns -ETIMEDOUT), passing its return
value through pseries_status_to_err(rc) a second time in
plpks_gen_wrapping_key() re-maps the Linux errno as if it were a hypervisor
return code.

Because H_RESOURCE is -16 and -EBUSY is also -16, a -EBUSY return from
plpks_confirm_object_flushed() gets converted into -EEXIST. With this patch
Propagating -EEXIST to callers and trusted_pkwm_init() treating -EEXIST as
success, won't a -EBUSY flush failure be silently ignored in both
plpks_gen_wrapping_key() and trusted_pkwm_init(), while other flush errors
like -ETIMEDOUT or -EIO are corrupted into -EINVAL or -ENXIO?

>  
>  	kfree(label);
>  out:

[ ... ]

> diff --git a/security/keys/trusted-keys/trusted_pkwm.c b/security/keys/trusted-keys/trusted_pkwm.c
> index 6774557f818a1..ba7717b474b27 100644
> --- a/security/keys/trusted-keys/trusted_pkwm.c
> +++ b/security/keys/trusted-keys/trusted_pkwm.c

[ ... ]

> @@ -82,6 +90,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;
> +	struct plpks_var var = {0};
>  	u8 *input_buf, *output_buf;
>  	u64 output_len, input_len;
>  	int rc;
> @@ -108,8 +117,18 @@ static int trusted_pkwm_seal(struct trusted_key_payload *p, char *datablob)
>  
>  	pkwm = options->private;
>  
> +	if (pkwm->wrapping_key_label[0]) {
> +		var.name = (u8 *)pkwm->wrapping_key_label;
> +		var.namelen = strlen(pkwm->wrapping_key_label);
> +	} else {
> +		var.name = (u8 *)PLPKS_DEFAULT_WRAPKEY_LABEL;
> +		var.namelen = strlen(PLPKS_DEFAULT_WRAPKEY_LABEL);
> +	}
> +	var.os = PLPKS_VAR_LINUX;
> +	var.component = PLPKS_WRAPKEY_COMPONENT;
> +
>  	rc = plpks_wrap_object(&input_buf, input_len, pkwm->wrap_flags,
> -			       &output_buf, &output_len);
> +			       &output_buf, &output_len, &var);
>  	if (!rc) {
>  		memcpy(p->blob, output_buf, output_len);
>  		p->blob_len = output_len;

[Severity: High]
Can kfree(output_buf) be called on an uninitialized pointer in
trusted_pkwm_seal()?

In trusted_pkwm_seal(), output_buf is declared on the stack without being
initialized to NULL:

security/keys/trusted-keys/trusted_pkwm.c:trusted_pkwm_seal() {
	...
	u8 *input_buf, *output_buf;
	...
	rc = plpks_wrap_object(&input_buf, input_len, pkwm->wrap_flags,
			       &output_buf, &output_len, &var);
	if (!rc) {
		...
	} else {
		pr_err("Wrapping of payload key failed: %d\n", rc);
	}

	kfree_sensitive(input_buf);
	kfree(output_buf);
}

In plpks_wrap_object(), *output_buf is only assigned when kzalloc() is
called:

arch/powerpc/platforms/pseries/plpks.c:plpks_wrap_object() {
	...
	if (!var || !var->name || !*var->name)
		return -EINVAL;

	auth = construct_auth(PLPKS_OS_OWNER);
	if (IS_ERR(auth))
		return PTR_ERR(auth);

	label = construct_label(var->component, var->os, var->name,
				var->namelen);
	if (IS_ERR(label)) {
		rc = PTR_ERR(label);
		goto out;
	}
	...
	if (sb_audit_or_enforce_bit & sb_enforce_bit) {
		pr_err("Cannot set both audit/enforce and enforce bits.");
		rc = -EINVAL;
		goto out_free_label;
	}
	...
	*output_buf = kzalloc(ALIGN(*output_len, PLPKS_WRAPPING_BUF_ALIGN),
			      GFP_KERNEL);
}

If plpks_wrap_object() returns early via any of those error paths before
reaching kzalloc() (for example if construct_label() fails on the user-
supplied var->name or var->namelen, or if construct_auth() fails with
-ENOMEM), *output_buf is never written and trusted_pkwm_seal() passes the
uninitialized stack value to kfree(output_buf).

The same pattern also exists in trusted_pkwm_unseal(), where output_buf is
uninitialized on the stack and passed to kfree_sensitive(output_buf) if
construct_auth() fails in plpks_unwrap_object() before *output_buf is
allocated. Should output_buf be initialized to NULL in both
trusted_pkwm_seal() and trusted_pkwm_unseal()?

[ ... ]

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