[PATCH 0/3] Enhancements to the secvar interface in static key management mode

[PATCH 0/3] Enhancements to the secvar interface in static key management mode

[Srish Srinivasan]

The PLPKS enabled Power LPAR sysfs exposes all of the secure boot secure
variables irrespective of the key management mode. There is support for
both static and dynamic key management and the key management mode can
be updated using the management console. The user can modify the secure
boot secvars db, dbx, grubdb, grubdbx, and sbat only in the dynamic key
mode. But the sysfs interface exposes these secvars even in static key
mode. This could lead to errors when reading them or writing to them in
the static key mode.

Update the secvar format property based on the key management mode and
expose only the secure variables relevant to the key management mode.
Enable loading of signed third-party kernel modules in the static key
mode when the platform keystore is enabled.

Srish Srinivasan (3):
  powerpc/pseries: Correct secvar format representation for static key
    management
  powerpc/secvar: Expose secvars relevant to the key management mode
  integrity/platform_certs: Allow loading of keys in static key
    management mode

 Documentation/ABI/testing/sysfs-secvar        |  9 +-
 arch/powerpc/platforms/pseries/plpks-secvar.c | 98 ++++++++++++-------
 .../integrity/platform_certs/load_powerpc.c   |  5 +-
 3 files changed, 73 insertions(+), 39 deletions(-)

-- 
2.47.1

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

[Srish Srinivasan]

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@imap.linux.ibm.com>
Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Stefan Berger <stefanb@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.
+ */
+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);
+		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);
 }
 
 static int plpks_max_size(u64 *max_size)
-- 
2.47.1

[PATCH 2/3] powerpc/secvar: Expose secvars relevant to the key management mode

[Srish Srinivasan]

The PLPKS enabled PowerVM LPAR sysfs exposes all of the secure boot
secvars irrespective of the key management mode.

The PowerVM LPAR supports static and dynamic key management for secure
boot. The key management option can be updated in the management
console. Only in the dynamic key mode can the user modify the secure
boot secvars db, dbx, grubdb, grubdbx, and sbat, which are exposed via
the sysfs interface. But the sysfs interface exposes these secvars even
in the static key mode. This could lead to errors when reading them or
writing to them in the static key mode.

Expose only PK, trustedcadb, and moduledb in the static key mode to
enable loading of signed third-party kernel modules.

Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 Documentation/ABI/testing/sysfs-secvar        |  9 ++++--
 arch/powerpc/platforms/pseries/plpks-secvar.c | 28 ++++++++++++++++---
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-secvar b/Documentation/ABI/testing/sysfs-secvar
index 857cf12b0904..2bdc7d9c0c10 100644
--- a/Documentation/ABI/testing/sysfs-secvar
+++ b/Documentation/ABI/testing/sysfs-secvar
@@ -22,9 +22,12 @@ Description:	A string indicating which backend is in use by the firmware.
 		and is expected to be "ibm,edk2-compat-v1".
 
 		On pseries/PLPKS, this is generated by the kernel based on the
-		version number in the SB_VERSION variable in the keystore, and
-		has the form "ibm,plpks-sb-v<version>", or
-		"ibm,plpks-sb-unknown" if there is no SB_VERSION variable.
+		existence of the SB_VERSION property in firmware. This string
+		takes the form "ibm,plpks-sb-v1" in the presence of SB_VERSION,
+		indicating the key management mode is dynamic. Otherwise it
+		takes the form "ibm,plpks-sb-v0" in the static key management
+		mode. Only secvars relevant to the key management mode are
+		exposed.
 
 What:		/sys/firmware/secvar/vars/<variable name>
 Date:		August 2019
diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c b/arch/powerpc/platforms/pseries/plpks-secvar.c
index d57067a733ab..cbcb2c356f2a 100644
--- a/arch/powerpc/platforms/pseries/plpks-secvar.c
+++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
@@ -59,7 +59,14 @@ static u32 get_policy(const char *name)
 		return PLPKS_SIGNEDUPDATE;
 }
 
-static const char * const plpks_var_names[] = {
+static const char * const plpks_var_names_static[] = {
+	"PK",
+	"moduledb",
+	"trustedcadb",
+	NULL,
+};
+
+static const char * const plpks_var_names_dynamic[] = {
 	"PK",
 	"KEK",
 	"db",
@@ -207,21 +214,34 @@ static int plpks_max_size(u64 *max_size)
 	return 0;
 }
 
+static const struct secvar_operations plpks_secvar_ops_static = {
+	.get = plpks_get_variable,
+	.set = plpks_set_variable,
+	.format = plpks_secvar_format,
+	.max_size = plpks_max_size,
+	.config_attrs = config_attrs,
+	.var_names = plpks_var_names_static,
+};
 
-static const struct secvar_operations plpks_secvar_ops = {
+static const struct secvar_operations plpks_secvar_ops_dynamic = {
 	.get = plpks_get_variable,
 	.set = plpks_set_variable,
 	.format = plpks_secvar_format,
 	.max_size = plpks_max_size,
 	.config_attrs = config_attrs,
-	.var_names = plpks_var_names,
+	.var_names = plpks_var_names_dynamic,
 };
 
 static int plpks_secvar_init(void)
 {
+	u8 mode;
+
 	if (!plpks_is_available())
 		return -ENODEV;
 
-	return set_secvar_ops(&plpks_secvar_ops);
+	mode = plpks_get_sb_keymgmt_mode();
+	if (mode)
+		return set_secvar_ops(&plpks_secvar_ops_dynamic);
+	return set_secvar_ops(&plpks_secvar_ops_static);
 }
 machine_device_initcall(pseries, plpks_secvar_init);
-- 
2.47.1

[PATCH 3/3] integrity/platform_certs: Allow loading of keys in static key management mode

[Srish Srinivasan]

On PLPKS enabled PowerVM LPAR, there is no provision to load signed
third-party kernel modules when the key management mode is static. This
is because keys from secure boot secvars are only loaded when the key
management mode is dynamic.

Allow loading of the trustedcadb and moduledb keys even in the static
key management mode, where the secvar format string takes the form
"ibm,plpks-sb-v0".

Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 security/integrity/platform_certs/load_powerpc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
index c85febca3343..714c961a00f5 100644
--- a/security/integrity/platform_certs/load_powerpc.c
+++ b/security/integrity/platform_certs/load_powerpc.c
@@ -75,12 +75,13 @@ static int __init load_powerpc_certs(void)
 		return -ENODEV;
 
 	// Check for known secure boot implementations from OPAL or PLPKS
-	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-sb-v1", buf)) {
+	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-sb-v1", buf) &&
+	    strcmp("ibm,plpks-sb-v0", buf)) {
 		pr_err("Unsupported secvar implementation \"%s\", not loading certs\n", buf);
 		return -ENODEV;
 	}
 
-	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
+	if (strcmp("ibm,plpks-sb-v1", buf) == 0 || strcmp("ibm,plpks-sb-v0", buf) == 0)
 		/* PLPKS authenticated variables ESL data is prefixed with 8 bytes of timestamp */
 		offset = 8;
 
-- 
2.47.1

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

[Nayna Jain]

On 4/30/25 5:03 AM, 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.

Reviewed-by: Nayna Jain <nayna@linux.ibm.com>

Thanks & Regards,
    - Nayna

>
> Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@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.
> + */
> +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);
> +		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);
>   }
>   
>   static int plpks_max_size(u64 *max_size)

Re: [PATCH 2/3] powerpc/secvar: Expose secvars relevant to the key management mode

[Nayna Jain]

On 4/30/25 5:03 AM, Srish Srinivasan wrote:
> The PLPKS enabled PowerVM LPAR sysfs exposes all of the secure boot
> secvars irrespective of the key management mode.
>
> The PowerVM LPAR supports static and dynamic key management for secure
> boot. The key management option can be updated in the management
> console. Only in the dynamic key mode can the user modify the secure
> boot secvars db, dbx, grubdb, grubdbx, and sbat, which are exposed via
> the sysfs interface. But the sysfs interface exposes these secvars even
> in the static key mode. This could lead to errors when reading them or
> writing to them in the static key mode.
>
> Expose only PK, trustedcadb, and moduledb in the static key mode to
> enable loading of signed third-party kernel modules.

Reviewed-by: Nayna Jain <nayna@linux.ibm.com>

Thanks & Regards,
    - Nayna

>
> Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   Documentation/ABI/testing/sysfs-secvar        |  9 ++++--
>   arch/powerpc/platforms/pseries/plpks-secvar.c | 28 ++++++++++++++++---
>   2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-secvar b/Documentation/ABI/testing/sysfs-secvar
> index 857cf12b0904..2bdc7d9c0c10 100644
> --- a/Documentation/ABI/testing/sysfs-secvar
> +++ b/Documentation/ABI/testing/sysfs-secvar
> @@ -22,9 +22,12 @@ Description:	A string indicating which backend is in use by the firmware.
>   		and is expected to be "ibm,edk2-compat-v1".
>   
>   		On pseries/PLPKS, this is generated by the kernel based on the
> -		version number in the SB_VERSION variable in the keystore, and
> -		has the form "ibm,plpks-sb-v<version>", or
> -		"ibm,plpks-sb-unknown" if there is no SB_VERSION variable.
> +		existence of the SB_VERSION property in firmware. This string
> +		takes the form "ibm,plpks-sb-v1" in the presence of SB_VERSION,
> +		indicating the key management mode is dynamic. Otherwise it
> +		takes the form "ibm,plpks-sb-v0" in the static key management
> +		mode. Only secvars relevant to the key management mode are
> +		exposed.
>   
>   What:		/sys/firmware/secvar/vars/<variable name>
>   Date:		August 2019
> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c b/arch/powerpc/platforms/pseries/plpks-secvar.c
> index d57067a733ab..cbcb2c356f2a 100644
> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
> @@ -59,7 +59,14 @@ static u32 get_policy(const char *name)
>   		return PLPKS_SIGNEDUPDATE;
>   }
>   
> -static const char * const plpks_var_names[] = {
> +static const char * const plpks_var_names_static[] = {
> +	"PK",
> +	"moduledb",
> +	"trustedcadb",
> +	NULL,
> +};
> +
> +static const char * const plpks_var_names_dynamic[] = {
>   	"PK",
>   	"KEK",
>   	"db",
> @@ -207,21 +214,34 @@ static int plpks_max_size(u64 *max_size)
>   	return 0;
>   }
>   
> +static const struct secvar_operations plpks_secvar_ops_static = {
> +	.get = plpks_get_variable,
> +	.set = plpks_set_variable,
> +	.format = plpks_secvar_format,
> +	.max_size = plpks_max_size,
> +	.config_attrs = config_attrs,
> +	.var_names = plpks_var_names_static,
> +};
>   
> -static const struct secvar_operations plpks_secvar_ops = {
> +static const struct secvar_operations plpks_secvar_ops_dynamic = {
>   	.get = plpks_get_variable,
>   	.set = plpks_set_variable,
>   	.format = plpks_secvar_format,
>   	.max_size = plpks_max_size,
>   	.config_attrs = config_attrs,
> -	.var_names = plpks_var_names,
> +	.var_names = plpks_var_names_dynamic,
>   };
>   
>   static int plpks_secvar_init(void)
>   {
> +	u8 mode;
> +
>   	if (!plpks_is_available())
>   		return -ENODEV;
>   
> -	return set_secvar_ops(&plpks_secvar_ops);
> +	mode = plpks_get_sb_keymgmt_mode();
> +	if (mode)
> +		return set_secvar_ops(&plpks_secvar_ops_dynamic);
> +	return set_secvar_ops(&plpks_secvar_ops_static);
>   }
>   machine_device_initcall(pseries, plpks_secvar_init);

Re: [PATCH 3/3] integrity/platform_certs: Allow loading of keys in static key management mode

[Nayna Jain]

On 4/30/25 5:03 AM, Srish Srinivasan wrote:
> On PLPKS enabled PowerVM LPAR, there is no provision to load signed
> third-party kernel modules when the key management mode is static. This
> is because keys from secure boot secvars are only loaded when the key
> management mode is dynamic.
>
> Allow loading of the trustedcadb and moduledb keys even in the static
> key management mode, where the secvar format string takes the form
> "ibm,plpks-sb-v0".

Reviewed-by: Nayna Jain <nayna@linux.ibm.com>

Thanks & Regards,
    - Nayna

>
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   security/integrity/platform_certs/load_powerpc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> index c85febca3343..714c961a00f5 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -75,12 +75,13 @@ static int __init load_powerpc_certs(void)
>   		return -ENODEV;
>   
>   	// Check for known secure boot implementations from OPAL or PLPKS
> -	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-sb-v1", buf)) {
> +	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-sb-v1", buf) &&
> +	    strcmp("ibm,plpks-sb-v0", buf)) {
>   		pr_err("Unsupported secvar implementation \"%s\", not loading certs\n", buf);
>   		return -ENODEV;
>   	}
>   
> -	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> +	if (strcmp("ibm,plpks-sb-v1", buf) == 0 || strcmp("ibm,plpks-sb-v0", buf) == 0)
>   		/* PLPKS authenticated variables ESL data is prefixed with 8 bytes of timestamp */
>   		offset = 8;
>   

Re: [PATCH 2/3] powerpc/secvar: Expose secvars relevant to the key management mode

[Andrew Donnellan]

On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
> The PLPKS enabled PowerVM LPAR sysfs exposes all of the secure boot
> secvars irrespective of the key management mode.
> 
> The PowerVM LPAR supports static and dynamic key management for
> secure
> boot. The key management option can be updated in the management
> console. Only in the dynamic key mode can the user modify the secure
> boot secvars db, dbx, grubdb, grubdbx, and sbat, which are exposed
> via
> the sysfs interface. But the sysfs interface exposes these secvars
> even
> in the static key mode. This could lead to errors when reading them
> or
> writing to them in the static key mode.
> 
> Expose only PK, trustedcadb, and moduledb in the static key mode to
> enable loading of signed third-party kernel modules.
> 
> Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

I'm assuming it's been determined that there's no value in letting
userspace see db/dbx/etc in a read-only way in static mode?

With one comment below:

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  Documentation/ABI/testing/sysfs-secvar        |  9 ++++--
>  arch/powerpc/platforms/pseries/plpks-secvar.c | 28 ++++++++++++++++-
> --
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-secvar
> b/Documentation/ABI/testing/sysfs-secvar
> index 857cf12b0904..2bdc7d9c0c10 100644
> --- a/Documentation/ABI/testing/sysfs-secvar
> +++ b/Documentation/ABI/testing/sysfs-secvar
> @@ -22,9 +22,12 @@ Description:	A string indicating which backend is
> in use by the firmware.
>  		and is expected to be "ibm,edk2-compat-v1".
>  
>  		On pseries/PLPKS, this is generated by the kernel
> based on the
> -		version number in the SB_VERSION variable in the
> keystore, and
> -		has the form "ibm,plpks-sb-v<version>", or
> -		"ibm,plpks-sb-unknown" if there is no SB_VERSION
> variable.
> +		existence of the SB_VERSION property in firmware.
> This string
> +		takes the form "ibm,plpks-sb-v1" in the presence of
> SB_VERSION,
> +		indicating the key management mode is dynamic.
> Otherwise it
> +		takes the form "ibm,plpks-sb-v0" in the static key
> management
> +		mode. Only secvars relevant to the key management
> mode are
> +		exposed.

Everything except the last sentence here is relevant to the previous
patch in the series (noting my comments on the previous patch about the
string).

The last sentence is more related to the <variable name> entry than the
format entry, and perhaps worth including a list of what variables are
applicable to each mode.

>  
>  What:		/sys/firmware/secvar/vars/<variable name>
>  Date:		August 2019
> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c
> b/arch/powerpc/platforms/pseries/plpks-secvar.c
> index d57067a733ab..cbcb2c356f2a 100644
> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
> @@ -59,7 +59,14 @@ static u32 get_policy(const char *name)
>  		return PLPKS_SIGNEDUPDATE;
>  }
>  
> -static const char * const plpks_var_names[] = {
> +static const char * const plpks_var_names_static[] = {
> +	"PK",
> +	"moduledb",
> +	"trustedcadb",
> +	NULL,
> +};
> +
> +static const char * const plpks_var_names_dynamic[] = {
>  	"PK",
>  	"KEK",
>  	"db",
> @@ -207,21 +214,34 @@ static int plpks_max_size(u64 *max_size)
>  	return 0;
>  }
>  
> +static const struct secvar_operations plpks_secvar_ops_static = {
> +	.get = plpks_get_variable,
> +	.set = plpks_set_variable,
> +	.format = plpks_secvar_format,
> +	.max_size = plpks_max_size,
> +	.config_attrs = config_attrs,
> +	.var_names = plpks_var_names_static,
> +};
>  
> -static const struct secvar_operations plpks_secvar_ops = {
> +static const struct secvar_operations plpks_secvar_ops_dynamic = {
>  	.get = plpks_get_variable,
>  	.set = plpks_set_variable,
>  	.format = plpks_secvar_format,
>  	.max_size = plpks_max_size,
>  	.config_attrs = config_attrs,
> -	.var_names = plpks_var_names,
> +	.var_names = plpks_var_names_dynamic,
>  };
>  
>  static int plpks_secvar_init(void)
>  {
> +	u8 mode;
> +
>  	if (!plpks_is_available())
>  		return -ENODEV;
>  
> -	return set_secvar_ops(&plpks_secvar_ops);
> +	mode = plpks_get_sb_keymgmt_mode();
> +	if (mode)
> +		return set_secvar_ops(&plpks_secvar_ops_dynamic);
> +	return set_secvar_ops(&plpks_secvar_ops_static);
>  }
>  machine_device_initcall(pseries, plpks_secvar_init);

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

Re: [PATCH 3/3] integrity/platform_certs: Allow loading of keys in static key management mode

[Andrew Donnellan]

On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
> On PLPKS enabled PowerVM LPAR, there is no provision to load signed
> third-party kernel modules when the key management mode is static.
> This
> is because keys from secure boot secvars are only loaded when the key
> management mode is dynamic.
> 
> Allow loading of the trustedcadb and moduledb keys even in the static
> key management mode, where the secvar format string takes the form
> "ibm,plpks-sb-v0".
> 
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  security/integrity/platform_certs/load_powerpc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index c85febca3343..714c961a00f5 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -75,12 +75,13 @@ static int __init load_powerpc_certs(void)
>  		return -ENODEV;
>  
>  	// Check for known secure boot implementations from OPAL or
> PLPKS
> -	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-
> sb-v1", buf)) {
> +	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-
> sb-v1", buf) &&
> +	    strcmp("ibm,plpks-sb-v0", buf)) {
>  		pr_err("Unsupported secvar implementation \"%s\",
> not loading certs\n", buf);
>  		return -ENODEV;
>  	}
>  
> -	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> +	if (strcmp("ibm,plpks-sb-v1", buf) == 0 ||
> strcmp("ibm,plpks-sb-v0", buf) == 0)
>  		/* PLPKS authenticated variables ESL data is
> prefixed with 8 bytes of timestamp */
>  		offset = 8;
>  

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

Re: [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@imap.linux.ibm.com>
> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@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@linux.ibm.com   IBM Australia Limited

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

[Srish Srinivasan]

On 5/5/25 2:06 PM, Andrew Donnellan wrote:
> 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@imap.linux.ibm.com>
>> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@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.
Hi Andrew,
Thanks a lot for your feedback.

Thanks for noticing this. Yes, will fix it.
>
>> +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.
Yes, I agree with your concern. I just want to add that, as per my 
understanding, we need to check for both -ENOENT and -EPERM,
as explained below:

As per H_PKS_READ_OBJECT semantics described in the PAPR v10.60 
(https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8),

* If the object is not world readable, verify that the consumer password 
matches the stored value in the hypervisor. Else return H_AUTHORITY.
* Verify if the object exists, else return H_NOT_FOUND.
* Verify if the policy for the object is met, else return H_AUTHORITY.

So, the hypervisor returns H_NOT_FOUND only for the authenticated 
consumer. For unauthenticated consumers, which is the case here,
it would return H_AUTHORITY.
> 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.
Currently, the other layers in the boot stack assume static key mode for 
any failure in reading SB_VERSION. We added the same interpretation
in the kernel to keep it consistent with the other layers, and represent 
the same to the user. This is the reason for not parsing the error codes
when trying to read SB_VERSION, and defaulting to the static key 
management mode. However, we want the exact error code to be logged
for debugging purposes. And, it does make sense to have logging only for 
error codes other than -ENOENT and -EPERM, as you suggested.
Does this sound okay?
>
>> +		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.
Sure.

Thanks and Regards,
Srish
>
>>   }
>>   
>>   static int plpks_max_size(u64 *max_size)

Re: [PATCH 2/3] powerpc/secvar: Expose secvars relevant to the key management mode

[Srish Srinivasan]

On 5/5/25 12:53 PM, Andrew Donnellan wrote:
> On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
>> The PLPKS enabled PowerVM LPAR sysfs exposes all of the secure boot
>> secvars irrespective of the key management mode.
>>
>> The PowerVM LPAR supports static and dynamic key management for
>> secure
>> boot. The key management option can be updated in the management
>> console. Only in the dynamic key mode can the user modify the secure
>> boot secvars db, dbx, grubdb, grubdbx, and sbat, which are exposed
>> via
>> the sysfs interface. But the sysfs interface exposes these secvars
>> even
>> in the static key mode. This could lead to errors when reading them
>> or
>> writing to them in the static key mode.
>>
>> Expose only PK, trustedcadb, and moduledb in the static key mode to
>> enable loading of signed third-party kernel modules.
>>
>> Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> I'm assuming it's been determined that there's no value in letting
> userspace see db/dbx/etc in a read-only way in static mode?
>
> With one comment below:
>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Hi Andrew,
Thanks a lot for your feedback.

Yes, that is correct.
>> ---
>>   Documentation/ABI/testing/sysfs-secvar        |  9 ++++--
>>   arch/powerpc/platforms/pseries/plpks-secvar.c | 28 ++++++++++++++++-
>> --
>>   2 files changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-secvar
>> b/Documentation/ABI/testing/sysfs-secvar
>> index 857cf12b0904..2bdc7d9c0c10 100644
>> --- a/Documentation/ABI/testing/sysfs-secvar
>> +++ b/Documentation/ABI/testing/sysfs-secvar
>> @@ -22,9 +22,12 @@ Description:	A string indicating which backend is
>> in use by the firmware.
>>   		and is expected to be "ibm,edk2-compat-v1".
>>   
>>   		On pseries/PLPKS, this is generated by the kernel
>> based on the
>> -		version number in the SB_VERSION variable in the
>> keystore, and
>> -		has the form "ibm,plpks-sb-v<version>", or
>> -		"ibm,plpks-sb-unknown" if there is no SB_VERSION
>> variable.
>> +		existence of the SB_VERSION property in firmware.
>> This string
>> +		takes the form "ibm,plpks-sb-v1" in the presence of
>> SB_VERSION,
>> +		indicating the key management mode is dynamic.
>> Otherwise it
>> +		takes the form "ibm,plpks-sb-v0" in the static key
>> management
>> +		mode. Only secvars relevant to the key management
>> mode are
>> +		exposed.
> Everything except the last sentence here is relevant to the previous
> patch in the series (noting my comments on the previous patch about the
> string).
>
> The last sentence is more related to the <variable name> entry than the
> format entry, and perhaps worth including a list of what variables are
> applicable to each mode.
Sure, will fix this.

Thanks and Regards,
Srish
>
>>   
>>   What:		/sys/firmware/secvar/vars/<variable name>
>>   Date:		August 2019
>> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> index d57067a733ab..cbcb2c356f2a 100644
>> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> @@ -59,7 +59,14 @@ static u32 get_policy(const char *name)
>>   		return PLPKS_SIGNEDUPDATE;
>>   }
>>   
>> -static const char * const plpks_var_names[] = {
>> +static const char * const plpks_var_names_static[] = {
>> +	"PK",
>> +	"moduledb",
>> +	"trustedcadb",
>> +	NULL,
>> +};
>> +
>> +static const char * const plpks_var_names_dynamic[] = {
>>   	"PK",
>>   	"KEK",
>>   	"db",
>> @@ -207,21 +214,34 @@ static int plpks_max_size(u64 *max_size)
>>   	return 0;
>>   }
>>   
>> +static const struct secvar_operations plpks_secvar_ops_static = {
>> +	.get = plpks_get_variable,
>> +	.set = plpks_set_variable,
>> +	.format = plpks_secvar_format,
>> +	.max_size = plpks_max_size,
>> +	.config_attrs = config_attrs,
>> +	.var_names = plpks_var_names_static,
>> +};
>>   
>> -static const struct secvar_operations plpks_secvar_ops = {
>> +static const struct secvar_operations plpks_secvar_ops_dynamic = {
>>   	.get = plpks_get_variable,
>>   	.set = plpks_set_variable,
>>   	.format = plpks_secvar_format,
>>   	.max_size = plpks_max_size,
>>   	.config_attrs = config_attrs,
>> -	.var_names = plpks_var_names,
>> +	.var_names = plpks_var_names_dynamic,
>>   };
>>   
>>   static int plpks_secvar_init(void)
>>   {
>> +	u8 mode;
>> +
>>   	if (!plpks_is_available())
>>   		return -ENODEV;
>>   
>> -	return set_secvar_ops(&plpks_secvar_ops);
>> +	mode = plpks_get_sb_keymgmt_mode();
>> +	if (mode)
>> +		return set_secvar_ops(&plpks_secvar_ops_dynamic);
>> +	return set_secvar_ops(&plpks_secvar_ops_static);
>>   }
>>   machine_device_initcall(pseries, plpks_secvar_init);

Re: [PATCH 3/3] integrity/platform_certs: Allow loading of keys in static key management mode

[Srish Srinivasan]

On 5/5/25 1:25 PM, Andrew Donnellan wrote:
> On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
>> On PLPKS enabled PowerVM LPAR, there is no provision to load signed
>> third-party kernel modules when the key management mode is static.
>> This
>> is because keys from secure boot secvars are only loaded when the key
>> management mode is dynamic.
>>
>> Allow loading of the trustedcadb and moduledb keys even in the static
>> key management mode, where the secvar format string takes the form
>> "ibm,plpks-sb-v0".
>>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Hi Andrew,
Thanks a lot for the review.

Thanks and Regards,
Srish
>> ---
>>   security/integrity/platform_certs/load_powerpc.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/integrity/platform_certs/load_powerpc.c
>> b/security/integrity/platform_certs/load_powerpc.c
>> index c85febca3343..714c961a00f5 100644
>> --- a/security/integrity/platform_certs/load_powerpc.c
>> +++ b/security/integrity/platform_certs/load_powerpc.c
>> @@ -75,12 +75,13 @@ static int __init load_powerpc_certs(void)
>>   		return -ENODEV;
>>   
>>   	// Check for known secure boot implementations from OPAL or
>> PLPKS
>> -	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-
>> sb-v1", buf)) {
>> +	if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks-
>> sb-v1", buf) &&
>> +	    strcmp("ibm,plpks-sb-v0", buf)) {
>>   		pr_err("Unsupported secvar implementation \"%s\",
>> not loading certs\n", buf);
>>   		return -ENODEV;
>>   	}
>>   
>> -	if (strcmp("ibm,plpks-sb-v1", buf) == 0)
>> +	if (strcmp("ibm,plpks-sb-v1", buf) == 0 ||
>> strcmp("ibm,plpks-sb-v0", buf) == 0)
>>   		/* PLPKS authenticated variables ESL data is
>> prefixed with 8 bytes of timestamp */
>>   		offset = 8;
>>   

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

[Nayna Jain]

On 5/5/25 4:36 AM, Andrew Donnellan wrote:
> 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@imap.linux.ibm.com>
>> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@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.

Yes, "ibm,plpks-sb-static" is more clear compared to "ibm,plpks-sb-v0".  
However, I am not sure why "static mode doesn't use the same version 
numbering scheme as dynamic mode". Infact, as per my understanding,  it 
is part of same versioning system. "0 represent static, 1 represent 
dynamic and anything beyond 1 would mean dynamic with additional features".

Also, wouldn't having "ibm,pkpks-sb-static" and then "ibm,pkpk-sb-v1" 
for dynamic would be bit confusing? I mean being static is clear, but 
what they relate v1 to? Or did you mean to have "ibm,plpks-sb-static" 
and "ibm,plpks-sb-dynamic"  for the two modes?

Thanks & Regards,
          - Nayna

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

[Andrew Donnellan]

On Tue, 2025-05-06 at 15:27 -0400, Nayna Jain wrote:
> 
> > 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.
> 
> Yes, "ibm,plpks-sb-static" is more clear compared to "ibm,plpks-sb-
> v0".  
> However, I am not sure why "static mode doesn't use the same version 
> numbering scheme as dynamic mode". Infact, as per my understanding, 
> it 
> is part of same versioning system. "0 represent static, 1 represent 
> dynamic and anything beyond 1 would mean dynamic with additional
> features".
> 
> Also, wouldn't having "ibm,pkpks-sb-static" and then "ibm,pkpk-sb-v1"
> for dynamic would be bit confusing? I mean being static is clear, but
> what they relate v1 to? Or did you mean to have "ibm,plpks-sb-static"
> and "ibm,plpks-sb-dynamic"  for the two modes?
> 

I don't feel strongly about this, as long as it's well documented.

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

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

[Andrew Donnellan]

On Wed, 2025-05-07 at 00:29 +0530, Srish Srinivasan wrote:
> > > +	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.
> Yes, I agree with your concern. I just want to add that, as per my 
> understanding, we need to check for both -ENOENT and -EPERM,
> as explained below:
> 
> As per H_PKS_READ_OBJECT semantics described in the PAPR v10.60 
> (https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8),
> 
> * If the object is not world readable, verify that the consumer
> password 
> matches the stored value in the hypervisor. Else return H_AUTHORITY.
> * Verify if the object exists, else return H_NOT_FOUND.
> * Verify if the policy for the object is met, else return
> H_AUTHORITY.
> 
> So, the hypervisor returns H_NOT_FOUND only for the authenticated 
> consumer. For unauthenticated consumers, which is the case here,
> it would return H_AUTHORITY.

We expect SB_VERSION to always be world-readable, I think? In which
case it shouldn't return H_AUTHORITY / -EPERM, ever, and if it does
that's an error which should be handled as an error. Or am I
misinterpreting the spec here?


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

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

[Srish Srinivasan]

On 5/7/25 11:47 AM, Andrew Donnellan wrote:
> On Wed, 2025-05-07 at 00:29 +0530, Srish Srinivasan wrote:
>>>> +	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.
>> Yes, I agree with your concern. I just want to add that, as per my
>> understanding, we need to check for both -ENOENT and -EPERM,
>> as explained below:
>>
>> As per H_PKS_READ_OBJECT semantics described in the PAPR v10.60
>> (https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8),
>>
>> * If the object is not world readable, verify that the consumer
>> password
>> matches the stored value in the hypervisor. Else return H_AUTHORITY.
>> * Verify if the object exists, else return H_NOT_FOUND.
>> * Verify if the policy for the object is met, else return
>> H_AUTHORITY.
>>
>> So, the hypervisor returns H_NOT_FOUND only for the authenticated
>> consumer. For unauthenticated consumers, which is the case here,
>> it would return H_AUTHORITY.
> We expect SB_VERSION to always be world-readable, I think? In which
> case it shouldn't return H_AUTHORITY / -EPERM, ever, and if it does
> that's an error which should be handled as an error. Or am I
> misinterpreting the spec here?

Yes, SB_VERSION is world-readable and should not return H_AUTHORITY in 
the case of dynamic key management mode. However, in
the case of static key management mode, when SB_VERSION does not exist, 
the hypervisor tries to authenticate the consumer. If the
authentication is successful, H_NOT_FOUND is returned, else H_AUTHORITY 
is returned. The intention behind authenticating the
consumer when the object is not found is to ensure that a 
non-authenticated consumer is unable to conclude on the absence of
the object. Here, when the kernel tries to read the non-existent 
SB_VERSION, it fails the authentication check and therefore,
gets the H_AUTHORITY error code.

>
>

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

[Andrew Donnellan]

On Wed, 2025-05-07 at 21:18 +0530, Srish Srinivasan wrote:
> > We expect SB_VERSION to always be world-readable, I think? In which
> > case it shouldn't return H_AUTHORITY / -EPERM, ever, and if it does
> > that's an error which should be handled as an error. Or am I
> > misinterpreting the spec here?
> 
> Yes, SB_VERSION is world-readable and should not return H_AUTHORITY
> in 
> the case of dynamic key management mode. However, in
> the case of static key management mode, when SB_VERSION does not
> exist, 
> the hypervisor tries to authenticate the consumer. If the
> authentication is successful, H_NOT_FOUND is returned, else
> H_AUTHORITY 
> is returned. The intention behind authenticating the
> consumer when the object is not found is to ensure that a 
> non-authenticated consumer is unable to conclude on the absence of
> the object. Here, when the kernel tries to read the non-existent 
> SB_VERSION, it fails the authentication check and therefore,
> gets the H_AUTHORITY error code.

Ah, I see my confusion: if the object *doesn't exist*, then it
obviously can't be a world-readable object, thus triggering the
password verification. In which case, we do need to catch -EPERM.

Thanks for correcting me!

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

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

[Andrew Donnellan]

On Wed, 2025-05-07 at 00:29 +0530, Srish Srinivasan wrote:
> > 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.
> Currently, the other layers in the boot stack assume static key mode
> for 
> any failure in reading SB_VERSION. We added the same interpretation
> in the kernel to keep it consistent with the other layers, and
> represent 
> the same to the user. This is the reason for not parsing the error
> codes
> when trying to read SB_VERSION, and defaulting to the static key 
> management mode. However, we want the exact error code to be logged
> for debugging purposes. And, it does make sense to have logging only
> for 
> error codes other than -ENOENT and -EPERM, as you suggested.
> Does this sound okay?

Okay, maybe document explicitly in a comment that we default to static
mode in the event of any weird errors.

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

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

[Srish Srinivasan]

On 5/12/25 3:25 PM, Andrew Donnellan wrote:
> On Wed, 2025-05-07 at 00:29 +0530, Srish Srinivasan wrote:
>>> 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.
>> Currently, the other layers in the boot stack assume static key mode
>> for
>> any failure in reading SB_VERSION. We added the same interpretation
>> in the kernel to keep it consistent with the other layers, and
>> represent
>> the same to the user. This is the reason for not parsing the error
>> codes
>> when trying to read SB_VERSION, and defaulting to the static key
>> management mode. However, we want the exact error code to be logged
>> for debugging purposes. And, it does make sense to have logging only
>> for
>> error codes other than -ENOENT and -EPERM, as you suggested.
>> Does this sound okay?
> Okay, maybe document explicitly in a comment that we default to static
> mode in the event of any weird errors.
Sure, will do that.
Thank You.
>