Re: [PATCH 1/3] powerpc/pseries: Correct secvar format representation for static key management
From: Andrew Donnellan <hidden>
Date: 2025-05-05 08:38:07
Also in:
linux-integrity, lkml
On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Signed-off-by: Souradeep <redacted> 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.cb/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