[PATCH 1/3] powerpc/pseries: Correct secvar format representation for static key management

[Andrew Donnellan]

On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
> On a PLPKS enabled PowerVM LPAR, the secvar format property for
> static
> key management is misrepresented as "ibm,plpks-sb-unknown", creating
> reason for confusion.
> 
> Static key management mode uses fixed, built-in keys. Dynamic key
> management mode allows keys to be updated in production to handle
> security updates without firmware rebuilds.
> 
> Define a function named plpks_get_sb_keymgmt_mode() to retrieve the
> key management mode based on the existence of the SB_VERSION property
> in the firmware.
> 
> Set the secvar format property to either "ibm,plpks-sb-v1" or
> "ibm,plpks-sb-v0" based on the key management mode, and return the
> length of the secvar format property.
> 
> Co-developed-by: Souradeep <soura at imap.linux.ibm.com>
> Signed-off-by: Souradeep <soura at imap.linux.ibm.com>
> Signed-off-by: Srish Srinivasan <ssrish at linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar at linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb at linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/plpks-secvar.c | 70 +++++++++++------
> --
>  1 file changed, 40 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c
> b/arch/powerpc/platforms/pseries/plpks-secvar.c
> index 257fd1f8bc19..d57067a733ab 100644
> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
> @@ -152,39 +152,49 @@ static int plpks_set_variable(const char *key,
> u64 key_len, u8 *data,
>  	return rc;
>  }
>  
> -// PLPKS dynamic secure boot doesn't give us a format string in the
> same way OPAL does.
> -// Instead, report the format using the SB_VERSION variable in the
> keystore.
> -// The string is made up by us, and takes the form "ibm,plpks-sb-
> v<n>" (or "ibm,plpks-sb-unknown"
> -// if the SB_VERSION variable doesn't exist). Hypervisor defines the
> SB_VERSION variable as a
> -// "1 byte unsigned integer value".
> -static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
> +/*
> + * Return the key management mode.
> + *
> + * SB_VERSION is defined as a "1 byte unsigned integer value". It is
> owned by
> + * the Partition Firmware and its presence indicates that the key
> management
> + * mode is dynamic. Only signed variables have null bytes in their
> names.
> + * SB_VERSION does not.
> + *
> + * Return 1 to indicate that the key management mode is dynamic.
> Otherwise
> + * return 0, indicating that the key management mode is static.
> + */

This description isn't accurate.

For dynamic mode, it doesn't return 1, it returns whatever version is
defined in SB_VERSION, which could be 1, or could at some later point be
a higher version.

Which makes the function name a bit of a misnomer too - it is returning
the version number, just the version number can now be zero.

> +static u8 plpks_get_sb_keymgmt_mode(void)
>  {
> -	struct plpks_var var = {0};
> -	ssize_t ret;
> -	u8 version;
> -
> -	var.component = NULL;
> -	// Only the signed variables have null bytes in their names,
> this one doesn't
> -	var.name = "SB_VERSION";
> -	var.namelen = strlen(var.name);
> -	var.datalen = 1;
> -	var.data = &version;
> -
> -	// Unlike the other vars, SB_VERSION is owned by firmware
> instead of the OS
> -	ret = plpks_read_fw_var(&var);
> -	if (ret) {
> -		if (ret == -ENOENT) {
> -			ret = snprintf(buf, bufsize, "ibm,plpks-sb-
> unknown");
> -		} else {
> -			pr_err("Error %ld reading SB_VERSION from
> firmware\n", ret);
> -			ret = -EIO;
> -		}
> -		goto err;
> +	u8 mode;
> +	ssize_t rc;
> +	struct plpks_var var = {
> +		.component = NULL,
> +		.name = "SB_VERSION",
> +		.namelen = 10,
> +		.datalen = 1,
> +		.data = &mode,
> +	};
> +
> +	rc = plpks_read_fw_var(&var);
> +	if (rc) {
> +		pr_info("Error %ld reading SB_VERSION from
> firmware\n", rc);

We need to check for -ENOENT, otherwise this message is going to be
printed every time you boot a machine in static mode.

I think you should handle this as the existing code does: if it's
ENOENT, return 0, and for other codes print an error and return -EIO.

> +		mode = 0;
>  	}
> +	return mode;
> +}
>  
> -	ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", version);
> -err:
> -	return ret;
> +// PLPKS dynamic secure boot doesn't give us a format string in the
> same way
> +// OPAL does. Instead, report the format using the SB_VERSION
> variable in the
> +// keystore. The string, made up by us, takes the form "ibm,plpks-
> sb-v<n>".Set
> +// the secvar format property to either "ibm,plpks-sb-v1" or
> "ibm,plpks-sb-v0",
> +// based on the key management mode, and return the length of the
> secvar format
> +// property.
> +static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
> +{
> +	u8 mode;
> +
> +	mode = plpks_get_sb_keymgmt_mode();
> +	return snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", mode);

It might be better to use something like "ibm,plpks-sb-static" in place
of "ibm,plpks-sb-v0" to make it instantly clear that static mode
doesn't use the same version numbering scheme as dynamic mode.

>  }
>  
>  static int plpks_max_size(u64 *max_size)

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd at linux.ibm.com   IBM Australia Limited