Re: [PATCH v4 16/24] powerpc/pseries: Implement signed update for PLPKS objects

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Jan 20, 2023 at 5:42 PM AEST, Andrew Donnellan wrote:
> From: Nayna Jain <nayna@linux.ibm.com>
>
> The Platform Keystore provides a signed update interface which can be used
> to create, replace or append to certain variables in the PKS in a secure
> fashion, with the hypervisor requiring that the update be signed using the
> Platform Key.
>
> Implement an interface to the H_PKS_SIGNED_UPDATE hcall in the plpks
> driver to allow signed updates to PKS objects.
>
> (The plpks driver doesn't need to do any cryptography or otherwise handle
> the actual signed variable contents - that will be handled by userspace
> tooling.)
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> [ajd: split patch, add timeout handling and misc cleanups]
> Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
>
> ---
>
> v3: Merge plpks fixes and signed update series with secvar series
>
>     Fix error code handling in plpks_confirm_object_flushed() (ruscur)
>
>     Pass plpks_var struct to plpks_signed_update_var() by reference (mpe)
>
>     Consistent constant naming scheme (ruscur)
>
> v4: Fix MAX_HCALL_OPCODE rebasing issue (npiggin)
> ---
>  arch/powerpc/include/asm/hvcall.h      |  1 +
>  arch/powerpc/include/asm/plpks.h       |  5 ++
>  arch/powerpc/platforms/pseries/plpks.c | 71 ++++++++++++++++++++++++--
>  3 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 95fd7f9485d5..c099780385dd 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -335,6 +335,7 @@
>  #define H_RPT_INVALIDATE	0x448
>  #define H_SCM_FLUSH		0x44C
>  #define H_GET_ENERGY_SCALE_INFO	0x450
> +#define H_PKS_SIGNED_UPDATE	0x454
>  #define H_WATCHDOG		0x45C
>  #define MAX_HCALL_OPCODE	H_WATCHDOG
>  
> diff --git a/arch/powerpc/include/asm/plpks.h b/arch/powerpc/include/asm/plpks.h
> index 7c5f51a9af7c..e7204e6c0ca4 100644
> --- a/arch/powerpc/include/asm/plpks.h
> +++ b/arch/powerpc/include/asm/plpks.h
> @@ -68,6 +68,11 @@ struct plpks_var_name_list {
>  	struct plpks_var_name varlist[];
>  };
>  
> +/**
> + * Updates the authenticated variable. It expects NULL as the component.
> + */
> +int plpks_signed_update_var(struct plpks_var *var, u64 flags);
> +
>  /**
>   * Writes the specified var and its data to PKS.
>   * Any caller of PKS driver should present a valid component type for
> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index 1189246b03dc..796ed5544ee5 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -81,6 +81,12 @@ static int pseries_status_to_err(int rc)
>  		err = -ENOENT;
>  		break;
>  	case H_BUSY:
> +	case H_LONG_BUSY_ORDER_1_MSEC:
> +	case H_LONG_BUSY_ORDER_10_MSEC:
> +	case H_LONG_BUSY_ORDER_100_MSEC:
> +	case H_LONG_BUSY_ORDER_1_SEC:
> +	case H_LONG_BUSY_ORDER_10_SEC:
> +	case H_LONG_BUSY_ORDER_100_SEC:
>  		err = -EBUSY;
>  		break;
>  	case H_AUTHORITY:

This is a bit sad to maintain here. It's duplicating bits with
hvcs_convert, and a bunch of open coded places. Probably not the
series to do anything about. Would be nice if we could standardise
it though.

> @@ -184,14 +190,17 @@ static struct label *construct_label(char *component, u8 varos, u8 *name,
>  				     u16 namelen)
>  {
>  	struct label *label;
> -	size_t slen;
> +	size_t slen = 0;
>  
>  	if (!name || namelen > PLPKS_MAX_NAME_SIZE)
>  		return ERR_PTR(-EINVAL);
>  
> -	slen = strlen(component);
> -	if (component && slen > sizeof(label->attr.prefix))
> -		return ERR_PTR(-EINVAL);
> +	// Support NULL component for signed updates
> +	if (component) {
> +		slen = strlen(component);
> +		if (slen > sizeof(label->attr.prefix))
> +			return ERR_PTR(-EINVAL);
> +	}

Is this already a bug? Code checks for component != NULL but previously
calls strlen which would oops on NULL component AFAIKS. Granted nothing
is actually using any of this these days.

It already seems like it's supposed to be allowed to rad NULL component
with read_var though? Why the differences, why not always allow NULL
component? (I assume there is some reason, I just don't know anything
about secvar or secure boot).

>  
>  	// The label structure must not cross a page boundary, so we align to the next power of 2
>  	label = kzalloc(roundup_pow_of_two(sizeof(*label)), GFP_KERNEL);
> @@ -397,6 +406,58 @@ static int plpks_confirm_object_flushed(struct label *label,
>  	return pseries_status_to_err(rc);
>  }
>  
> +int plpks_signed_update_var(struct plpks_var *var, u64 flags)
> +{
> +	unsigned long retbuf[PLPAR_HCALL9_BUFSIZE] = {0};
> +	int rc;
> +	struct label *label;
> +	struct plpks_auth *auth;
> +	u64 continuetoken = 0;
> +	u64 timeout = 0;
> +
> +	if (!var->data || var->datalen <= 0 || var->namelen > PLPKS_MAX_NAME_SIZE)
> +		return -EINVAL;
> +
> +	if (!(var->policy & PLPKS_SIGNEDUPDATE))
> +		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;
> +	}
> +
> +	do {
> +		rc = plpar_hcall9(H_PKS_SIGNED_UPDATE, retbuf,
> +				  virt_to_phys(auth), virt_to_phys(label),
> +				  label->size, var->policy, flags,
> +				  virt_to_phys(var->data), var->datalen,
> +				  continuetoken);
> +
> +		continuetoken = retbuf[0];
> +		if (pseries_status_to_err(rc) == -EBUSY) {
> +			int delay_ms = get_longbusy_msecs(rc);
> +			mdelay(delay_ms);
> +			timeout += delay_ms;
> +		}
> +		rc = pseries_status_to_err(rc);
> +	} while (rc == -EBUSY && timeout < PLPKS_MAX_TIMEOUT);
> +
> +	if (!rc)
> +		rc = plpks_confirm_object_flushed(label, auth);
> +
> +	kfree(label);
> +out:
> +	kfree(auth);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(plpks_signed_update_var);

Sorry I missed it before -- can this be a _GPL export?

Thanks,
Nick