pSeries dynamic secure boot secvar interface + platform keyring loading

Tested-by Russell Currey <[email protected]>

Reviewed-by Russell Currey <[email protected]>

---

v3: Merge plpks fixes and signed update series with secvar series

Neaten how we return at the end of the function (ruscur)

v4: Move up in series (npiggin)

---
 arch/powerpc/platforms/pseries/plpks.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/powerpc/platforms/pseries/plpks.c +8 -3

index 4edd1585e245..9e85b6d85b0b 100644
@@ -248,6 +248,7 @@ static int plpks_confirm_object_flushed(struct label *label,
 					struct plpks_auth *auth)
 {
 	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
+	bool timed_out = true;
 	u64 timeout = 0;
 	u8 status;
 	int rc;

@@ -259,22 +260,26 @@ static int plpks_confirm_object_flushed(struct label *label,
 
 		status = retbuf[0];
 		if (rc) {
+			timed_out = false;
 			if (rc == H_NOT_FOUND && status == 1)
 				rc = 0;
 			break;
 		}
 
-		if (!rc && status == 1)
+		if (!rc && status == 1) {
+			timed_out = false;
 			break;
+		}
 
 		usleep_range(PKS_FLUSH_SLEEP,
 			     PKS_FLUSH_SLEEP + PKS_FLUSH_SLEEP_RANGE);
 		timeout = timeout + PKS_FLUSH_SLEEP;
 	} while (timeout < PKS_FLUSH_MAX_TIMEOUT);
 
-	rc = pseries_status_to_err(rc);
+	if (timed_out)
+		return -ETIMEDOUT;
 
-	return rc;
+	return pseries_status_to_err(rc);
 }
 
 int plpks_write_var(struct plpks_var var)

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #2 lore raw

PATCH v6 2/26 powerpc/pseries: Fix alignment of PLPKS structures and buffers

A number of structures and buffers passed to PKS hcalls have alignment requirements, which could on occasion cause problems:

- Authorisation structures must be 16-byte aligned and must not cross a page boundary

- Label structures must not cross page boundaries

- Password output buffers must not cross page boundaries

To ensure correct alignment, we adjust the allocation size of each of these structures/buffers to be the closest power of 2 that is at least the size of the structure/buffer (since kmalloc() guarantees that an allocation of a power of 2 size will be aligned to at least that size).

Reported-by Benjamin Gray <[email protected]>

Fixes 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")

Reviewed-by Russell Currey <[email protected]>

---

v3: Merge plpks fixes and signed update series with secvar series

v4: Fix typo in commit message

Move up in series (npiggin)

v5: Reword commit message to better explain alignment guarantee (mpe)

---
 arch/powerpc/platforms/pseries/plpks.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

arch/powerpc/platforms/pseries/plpks.c +7 -3

index 9e85b6d85b0b..a01cf2ff140a 100644
@@ -126,7 +126,8 @@ static int plpks_gen_password(void)
 	u8 *password, consumer = PKS_OS_OWNER;
 	int rc;
 
-	password = kzalloc(maxpwsize, GFP_KERNEL);
+	// The password must not cross a page boundary, so we align to the next power of 2
+	password = kzalloc(roundup_pow_of_two(maxpwsize), GFP_KERNEL);
 	if (!password)
 		return -ENOMEM;

@@ -162,7 +163,9 @@ static struct plpks_auth *construct_auth(u8 consumer)
 	if (consumer > PKS_OS_OWNER)
 		return ERR_PTR(-EINVAL);
 
-	auth = kzalloc(struct_size(auth, password, maxpwsize), GFP_KERNEL);
+	// The auth structure must not cross a page boundary and must be
+	// 16 byte aligned. We align to the next largest power of 2
+	auth = kzalloc(roundup_pow_of_two(struct_size(auth, password, maxpwsize)), GFP_KERNEL);
 	if (!auth)
 		return ERR_PTR(-ENOMEM);

@@ -196,7 +199,8 @@ static struct label *construct_label(char *component, u8 varos, u8 *name,
 	if (component && slen > sizeof(label->attr.prefix))
 		return ERR_PTR(-EINVAL);
 
-	label = kzalloc(sizeof(*label), GFP_KERNEL);
+	// 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);
 	if (!label)
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #3 lore raw

PATCH v6 3/26 powerpc/secvar: Fix incorrect return in secvar_sysfs_load()

From: Russell Currey <[email protected]>

secvar_ops->get_next() returns -ENOENT when there are no more variables to return, which is expected behaviour.

Fix this by returning 0 if get_next() returns -ENOENT.

This fixes an issue introduced in commit bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs"), but the return code of secvar_sysfs_load() was never checked so this issue never mattered.

---

v5: New patch

---
 arch/powerpc/kernel/secvar-sysfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/powerpc/kernel/secvar-sysfs.c +4 -2

index 1ee4640a2641..7fa5f8ed9542 100644
@@ -179,8 +179,10 @@ static int secvar_sysfs_load(void)
 		rc = secvar_ops->get_next(name, &namesize, NAME_MAX_SIZE);
 		if (rc) {
 			if (rc != -ENOENT)
-				pr_err("error getting secvar from firmware %d\n",
-				       rc);
+				pr_err("error getting secvar from firmware %d\n", rc);
+			else
+				rc = 0;
+
 			break;
 		}

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #4 lore raw

PATCH v6 4/26 powerpc/secvar: Use u64 in secvar_operations

From: Michael Ellerman <[email protected]>

There's no reason for secvar_operations to use uint64_t vs the more common kernel type u64.

The types are compatible, but they require different printk format strings which can lead to confusion.

Change all the secvar related routines to use u64.

Signed-off-by Michael Ellerman <[email protected]>

Reviewed-by Russell Currey <[email protected]>

Reviewed-by Andrew Donnellan <[email protected]>

---

v3: Include new patch

---
 arch/powerpc/include/asm/secvar.h                | 9 +++------
 arch/powerpc/kernel/secvar-sysfs.c               | 8 ++++----
 arch/powerpc/platforms/powernv/opal-secvar.c     | 9 +++------
 security/integrity/platform_certs/load_powerpc.c | 4 ++--
 4 files changed, 12 insertions(+), 18 deletions(-)

arch/powerpc/include/asm/secvar.h +3 -6

index 4cc35b58b986..07ba36f868a7 100644
@@ -14,12 +14,9 @@
 extern const struct secvar_operations *secvar_ops;
 
 struct secvar_operations {
-	int (*get)(const char *key, uint64_t key_len, u8 *data,
-		   uint64_t *data_size);
-	int (*get_next)(const char *key, uint64_t *key_len,
-			uint64_t keybufsize);
-	int (*set)(const char *key, uint64_t key_len, u8 *data,
-		   uint64_t data_size);
+	int (*get)(const char *key, u64 key_len, u8 *data, u64 *data_size);
+	int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
+	int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
 };
 
 #ifdef CONFIG_PPC_SECURE_BOOT

arch/powerpc/kernel/secvar-sysfs.c +4 -4

index 7fa5f8ed9542..702044edf14d 100644
@@ -47,7 +47,7 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
 static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
 			 char *buf)
 {
-	uint64_t dsize;
+	u64 dsize;
 	int rc;
 
 	rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);

@@ -64,8 +64,8 @@ static ssize_t data_read(struct file *filep, struct kobject *kobj,
 			 struct bin_attribute *attr, char *buf, loff_t off,
 			 size_t count)
 {
-	uint64_t dsize;
 	char *data;
+	u64 dsize;
 	int rc;
 
 	rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);

@@ -166,9 +166,9 @@ static int update_kobj_size(void)
 
 static int secvar_sysfs_load(void)
 {
-	char *name;
-	uint64_t namesize = 0;
 	struct kobject *kobj;
+	u64 namesize = 0;
+	char *name;
 	int rc;
 
 	name = kzalloc(NAME_MAX_SIZE, GFP_KERNEL);

arch/powerpc/platforms/powernv/opal-secvar.c +3 -6

index 14133e120bdd..ef89861569e0 100644
@@ -54,8 +54,7 @@ static int opal_status_to_err(int rc)
 	return err;
 }
 
-static int opal_get_variable(const char *key, uint64_t ksize,
-			     u8 *data, uint64_t *dsize)
+static int opal_get_variable(const char *key, u64 ksize, u8 *data, u64 *dsize)
 {
 	int rc;

@@ -71,8 +70,7 @@ static int opal_get_variable(const char *key, uint64_t ksize,
 	return opal_status_to_err(rc);
 }
 
-static int opal_get_next_variable(const char *key, uint64_t *keylen,
-				  uint64_t keybufsize)
+static int opal_get_next_variable(const char *key, u64 *keylen, u64 keybufsize)
 {
 	int rc;

@@ -88,8 +86,7 @@ static int opal_get_next_variable(const char *key, uint64_t *keylen,
 	return opal_status_to_err(rc);
 }
 
-static int opal_set_variable(const char *key, uint64_t ksize, u8 *data,
-			     uint64_t dsize)
+static int opal_set_variable(const char *key, u64 ksize, u8 *data, u64 dsize)
 {
 	int rc;

security/integrity/platform_certs/load_powerpc.c +2 -2

index a2900cb85357..1e4f80a4e71c 100644
@@ -18,7 +18,7 @@
 /*
  * Get a certificate list blob from the named secure variable.
  */
-static __init void *get_cert_list(u8 *key, unsigned long keylen, uint64_t *size)
+static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
 {
 	int rc;
 	void *db;

@@ -51,7 +51,7 @@ static __init void *get_cert_list(u8 *key, unsigned long keylen, uint64_t *size)
 static int __init load_powerpc_certs(void)
 {
 	void *db = NULL, *dbx = NULL;
-	uint64_t dbsize = 0, dbxsize = 0;
+	u64 dbsize = 0, dbxsize = 0;
 	int rc = 0;
 	struct device_node *node;

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #5 lore raw

PATCH v6 5/26 powerpc/secvar: Warn and error if multiple secvar ops are set

From: Russell Currey <[email protected]>

The secvar code only supports one consumer at a time.

Multiple consumers aren't possible at this point in time, but we'd want it to be obvious if it ever could happen.

Co-developed-by Andrew Donnellan <[email protected]>

---

v4: Return an error and don't actually try to set secvar_operations if the
warning is triggered (npiggin)

v5: Drop "extern" to fix a checkpatch check (snowpatch)

v6: Return -EBUSY rather than -1 (stefanb)

---
 arch/powerpc/include/asm/secvar.h            |  4 ++--
 arch/powerpc/kernel/secvar-ops.c             | 10 ++++++++--
 arch/powerpc/platforms/powernv/opal-secvar.c |  4 +---
 3 files changed, 11 insertions(+), 7 deletions(-)

arch/powerpc/include/asm/secvar.h +2 -2

index 07ba36f868a7..a2b5f2203dc5 100644
@@ -21,11 +21,11 @@ struct secvar_operations {
 
 #ifdef CONFIG_PPC_SECURE_BOOT
 
-extern void set_secvar_ops(const struct secvar_operations *ops);
+int set_secvar_ops(const struct secvar_operations *ops);
 
 #else
 
-static inline void set_secvar_ops(const struct secvar_operations *ops) { }
+static inline int set_secvar_ops(const struct secvar_operations *ops) { return 0; }
 
 #endif

arch/powerpc/kernel/secvar-ops.c +8 -2

index 6a29777d6a2d..19172a2804f0 100644
@@ -8,10 +8,16 @@
 
 #include <linux/cache.h>
 #include <asm/secvar.h>
+#include <asm/bug.h>
 
-const struct secvar_operations *secvar_ops __ro_after_init;
+const struct secvar_operations *secvar_ops __ro_after_init = NULL;
 
-void set_secvar_ops(const struct secvar_operations *ops)
+int set_secvar_ops(const struct secvar_operations *ops)
 {
+	if (WARN_ON_ONCE(secvar_ops))
+		return -EBUSY;
+
 	secvar_ops = ops;
+
+	return 0;
 }

arch/powerpc/platforms/powernv/opal-secvar.c +1 -3

index ef89861569e0..4c0a3b030fe0 100644
@@ -113,9 +113,7 @@ static int opal_secvar_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	set_secvar_ops(&opal_secvar_ops);
-
-	return 0;
+	return set_secvar_ops(&opal_secvar_ops);
 }
 
 static const struct of_device_id opal_secvar_match[] = {

Signature · 1 line

2.39.1

Stefan Berger

Reviewed-by on v6 5/26

2023-02-10 20:51 UTC #6 lore raw

Re: [PATCH v6 05/26] powerpc/secvar: Warn and error if multiple secvar ops are set

Quoted 71 lines · Andrew Donnellan

From: Russell Currey <[email protected]>
The secvar code only supports one consumer at a time.
Multiple consumers aren't possible at this point in time, but we'd want it to be obvious if it ever could happen.
Signed-off-by Russell Currey <[email protected]>
Co-developed-by Andrew Donnellan <[email protected]>
Signed-off-by Andrew Donnellan <[email protected]>
---
v4: Return an error and don't actually try to set secvar_operations if the
warning is triggered (npiggin)
v5: Drop "extern" to fix a checkpatch check (snowpatch)
v6: Return -EBUSY rather than -1 (stefanb)
---
  arch/powerpc/include/asm/secvar.h            |  4 ++--
  arch/powerpc/kernel/secvar-ops.c             | 10 ++++++++--
  arch/powerpc/platforms/powernv/opal-secvar.c |  4 +---
  3 files changed, 11 insertions(+), 7 deletions(-)
arch/powerpc/include/asm/secvar.h +2 -2
index 07ba36f868a7..a2b5f2203dc5 100644
@@ -21,11 +21,11 @@ struct secvar_operations {
 
  #ifdef CONFIG_PPC_SECURE_BOOT
 
-extern void set_secvar_ops(const struct secvar_operations *ops);
+int set_secvar_ops(const struct secvar_operations *ops);
 
  #else
 
-static inline void set_secvar_ops(const struct secvar_operations *ops) { }
+static inline int set_secvar_ops(const struct secvar_operations *ops) { return 0; }
 
  #endif
 
arch/powerpc/kernel/secvar-ops.c +8 -2
index 6a29777d6a2d..19172a2804f0 100644
@@ -8,10 +8,16 @@
 
  #include <linux/cache.h>
  #include <asm/secvar.h>
+#include <asm/bug.h>
 
-const struct secvar_operations *secvar_ops __ro_after_init;
+const struct secvar_operations *secvar_ops __ro_after_init = NULL;
 
-void set_secvar_ops(const struct secvar_operations *ops)
+int set_secvar_ops(const struct secvar_operations *ops)
  {
+	if (WARN_ON_ONCE(secvar_ops))
+		return -EBUSY;
+
  	secvar_ops = ops;
+
+	return 0;
  }
arch/powerpc/platforms/powernv/opal-secvar.c +0 -0
index ef89861569e0..4c0a3b030fe0 100644

arch/powerpc/platforms/powernv/opal-secvar.c static int opal_secvar_probe(struct platform_device *pdev) · line 119

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-secvar.c +1 -3

@@ -113,9 +113,7 @@ static int opal_secvar_probe(struct platform_device *pdev)
  		return -ENODEV;
  	}
 
-	set_secvar_ops(&opal_secvar_ops);
-
-	return 0;
+	return set_secvar_ops(&opal_secvar_ops);
  }
 
  static const struct of_device_id opal_secvar_match[] = {

Andrew Donnellan

author

2023-02-10 08:04 UTC #7 lore raw

PATCH v6 6/26 powerpc/secvar: Use sysfs_emit() instead of sprintf()

From: Russell Currey <[email protected]>

The secvar format string and object size sysfs files are both ASCII text, and should use sysfs_emit(). No functional change.

---

v2: New patch (gregkh)

---
 arch/powerpc/kernel/secvar-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/powerpc/kernel/secvar-sysfs.c +2 -2

index 702044edf14d..b786d1005027 100644
@@ -35,7 +35,7 @@ static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
 	if (rc)
 		goto out;
 
-	rc = sprintf(buf, "%s\n", format);
+	rc = sysfs_emit(buf, "%s\n", format);
 
 out:
 	of_node_put(node);

@@ -57,7 +57,7 @@ static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
 		return rc;
 	}
 
-	return sprintf(buf, "%llu\n", dsize);
+	return sysfs_emit(buf, "%llu\n", dsize);
 }
 
 static ssize_t data_read(struct file *filep, struct kobject *kobj,

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #8 lore raw

PATCH v6 7/26 powerpc/secvar: Handle format string in the consumer

From: Russell Currey <[email protected]>

The code that handles the format string in secvar-sysfs.c is entirely OPAL specific, so create a new "format" op in secvar_operations to make the secvar code more generic. No functional change.

---

v2: Use sysfs_emit() instead of sprintf() (gregkh)

v3: Enforce format string size limit (ruscur)

v4: Pass the buffer size as an argument, not using a macro (stefanb,
npiggin)

Fix error reporting (npiggin)

---
 arch/powerpc/include/asm/secvar.h            |  1 +
 arch/powerpc/kernel/secvar-sysfs.c           | 27 +++++++-------------
 arch/powerpc/platforms/powernv/opal-secvar.c | 25 ++++++++++++++++++
 3 files changed, 35 insertions(+), 18 deletions(-)

arch/powerpc/include/asm/secvar.h +1 -0

index a2b5f2203dc5..1a2c696a48ad 100644
@@ -17,6 +17,7 @@ struct secvar_operations {
 	int (*get)(const char *key, u64 key_len, u8 *data, u64 *data_size);
 	int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
 	int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
+	ssize_t (*format)(char *buf, size_t bufsize);
 };
 
 #ifdef CONFIG_PPC_SECURE_BOOT

arch/powerpc/kernel/secvar-sysfs.c +9 ~~-18~~

index b786d1005027..e4661559c855 100644
@@ -21,26 +21,17 @@ static struct kset *secvar_kset;
 static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
 			   char *buf)
 {
-	ssize_t rc = 0;
-	struct device_node *node;
-	const char *format;
-
-	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
-	if (!of_device_is_available(node)) {
-		rc = -ENODEV;
-		goto out;
-	}
+	char tmp[32];
+	ssize_t len = secvar_ops->format(tmp, sizeof(tmp));
 
-	rc = of_property_read_string(node, "format", &format);
-	if (rc)
-		goto out;
+	if (len > 0)
+		return sysfs_emit(buf, "%s\n", tmp);
+	else if (len < 0)
+		pr_err("Error %zd reading format string\n", len);
+	else
+		pr_err("Got empty format string from backend\n");
 
-	rc = sysfs_emit(buf, "%s\n", format);
-
-out:
-	of_node_put(node);
-
-	return rc;
+	return -EIO;
 }

arch/powerpc/platforms/powernv/opal-secvar.c +25 -0

index 4c0a3b030fe0..e33bb703ecbc 100644
@@ -98,10 +98,35 @@ static int opal_set_variable(const char *key, u64 ksize, u8 *data, u64 dsize)
 	return opal_status_to_err(rc);
 }
 
+static ssize_t opal_secvar_format(char *buf, size_t bufsize)
+{
+	ssize_t rc = 0;
+	struct device_node *node;
+	const char *format;
+
+	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
+	if (!of_device_is_available(node)) {
+		rc = -ENODEV;
+		goto out;
+	}
+
+	rc = of_property_read_string(node, "format", &format);
+	if (rc)
+		goto out;
+
+	rc = snprintf(buf, bufsize, "%s", format);
+
+out:
+	of_node_put(node);
+
+	return rc;
+}
+
 static const struct secvar_operations opal_secvar_ops = {
 	.get = opal_get_variable,
 	.get_next = opal_get_next_variable,
 	.set = opal_set_variable,
+	.format = opal_secvar_format,
 };
 
 static int opal_secvar_probe(struct platform_device *pdev)

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #9 lore raw

PATCH v6 8/26 powerpc/secvar: Handle max object size in the consumer

From: Russell Currey <[email protected]>

Currently the max object size is handled in the core secvar code with an entirely OPAL-specific implementation, so create a new max_size() op and move the existing implementation into the powernv platform. Should be no functional change.

---

v3: Change uint64_t type to u64 (mpe)

v4: Return immediately if node is NULL (gjoyce)

---
 arch/powerpc/include/asm/secvar.h            |  1 +
 arch/powerpc/kernel/secvar-sysfs.c           | 17 +++------------
 arch/powerpc/platforms/powernv/opal-secvar.c | 22 ++++++++++++++++++++
 3 files changed, 26 insertions(+), 14 deletions(-)

arch/powerpc/include/asm/secvar.h +1 -0

index 1a2c696a48ad..bf396215903d 100644
@@ -18,6 +18,7 @@ struct secvar_operations {
 	int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
 	int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
 	ssize_t (*format)(char *buf, size_t bufsize);
+	int (*max_size)(u64 *max_size);
 };
 
 #ifdef CONFIG_PPC_SECURE_BOOT

arch/powerpc/kernel/secvar-sysfs.c +3 ~~-14~~

index e4661559c855..0966806f28c7 100644
@@ -132,27 +132,16 @@ static struct kobj_type secvar_ktype = {
 static int update_kobj_size(void)
 {
 
-	struct device_node *node;
 	u64 varsize;
-	int rc = 0;
+	int rc = secvar_ops->max_size(&varsize);
 
-	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
-	if (!of_device_is_available(node)) {
-		rc = -ENODEV;
-		goto out;
-	}
-
-	rc = of_property_read_u64(node, "max-var-size", &varsize);
 	if (rc)
-		goto out;
+		return rc;
 
 	data_attr.size = varsize;
 	update_attr.size = varsize;
 
-out:
-	of_node_put(node);
-
-	return rc;
+	return 0;
 }
 
 static int secvar_sysfs_load(void)

arch/powerpc/platforms/powernv/opal-secvar.c +22 -0

index e33bb703ecbc..a8436bf35e2f 100644
@@ -122,11 +122,33 @@ static ssize_t opal_secvar_format(char *buf, size_t bufsize)
 	return rc;
 }
 
+static int opal_secvar_max_size(u64 *max_size)
+{
+	int rc;
+	struct device_node *node;
+
+	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
+	if (!node)
+		return -ENODEV;
+
+	if (!of_device_is_available(node)) {
+		rc = -ENODEV;
+		goto out;
+	}
+
+	rc = of_property_read_u64(node, "max-var-size", max_size);
+
+out:
+	of_node_put(node);
+	return rc;
+}
+
 static const struct secvar_operations opal_secvar_ops = {
 	.get = opal_get_variable,
 	.get_next = opal_get_next_variable,
 	.set = opal_set_variable,
 	.format = opal_secvar_format,
+	.max_size = opal_secvar_max_size,
 };
 
 static int opal_secvar_probe(struct platform_device *pdev)

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #10 lore raw

PATCH v6 9/26 powerpc/secvar: Clean up init error messages

Remove unnecessary prefixes from error messages in secvar_sysfs_init() (the file defines pr_fmt, so putting "secvar:" in every message is unnecessary). Make capitalisation and punctuation more consistent.

---

v3: New patch (ajd)

---
 arch/powerpc/kernel/secvar-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/powerpc/kernel/secvar-sysfs.c +3 -3

index 0966806f28c7..8f3deff94009 100644
@@ -196,13 +196,13 @@ static int secvar_sysfs_init(void)
 	int rc;
 
 	if (!secvar_ops) {
-		pr_warn("secvar: failed to retrieve secvar operations.\n");
+		pr_warn("Failed to retrieve secvar operations\n");
 		return -ENODEV;
 	}
 
 	secvar_kobj = kobject_create_and_add("secvar", firmware_kobj);
 	if (!secvar_kobj) {
-		pr_err("secvar: Failed to create firmware kobj\n");
+		pr_err("Failed to create firmware kobj\n");
 		return -ENOMEM;
 	}

@@ -214,7 +214,7 @@ static int secvar_sysfs_init(void)
 
 	secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
 	if (!secvar_kset) {
-		pr_err("secvar: sysfs kobject registration failed.\n");
+		pr_err("sysfs kobject registration failed\n");
 		kobject_put(secvar_kobj);
 		return -ENOMEM;
 	}

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #11 lore raw

PATCH v6 10/26 powerpc/secvar: Extend sysfs to include config vars

From: Russell Currey <[email protected]>

The forthcoming pseries consumer of the secvar API wants to expose a number of config variables. Allowing secvar implementations to provide their own sysfs attributes makes it easy for consumers to expose what they need to.

This is not being used by the OPAL secvar implementation at present, and the config directory will not be created if no attributes are set.

Co-developed-by Andrew Donnellan <[email protected]>

---

v3: Remove unnecessary "secvar:" prefix from error messages (ajd)

Merge config attributes into secvar_operations (mpe)

---
 arch/powerpc/include/asm/secvar.h  |  2 ++
 arch/powerpc/kernel/secvar-sysfs.c | 33 +++++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 5 deletions(-)

arch/powerpc/include/asm/secvar.h +2 -0

index bf396215903d..011a53a8076c 100644
@@ -10,6 +10,7 @@
 
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/sysfs.h>
 
 extern const struct secvar_operations *secvar_ops;

@@ -19,6 +20,7 @@ struct secvar_operations {
 	int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
 	ssize_t (*format)(char *buf, size_t bufsize);
 	int (*max_size)(u64 *max_size);
+	const struct attribute **config_attrs;
 };
 
 #ifdef CONFIG_PPC_SECURE_BOOT

arch/powerpc/kernel/secvar-sysfs.c +28 -5

index 8f3deff94009..7df32be86507 100644
@@ -144,6 +144,19 @@ static int update_kobj_size(void)
 	return 0;
 }
 
+static int secvar_sysfs_config(struct kobject *kobj)
+{
+	struct attribute_group config_group = {
+		.name = "config",
+		.attrs = (struct attribute **)secvar_ops->config_attrs,
+	};
+
+	if (secvar_ops->config_attrs)
+		return sysfs_create_group(kobj, &config_group);
+
+	return 0;
+}
+
 static int secvar_sysfs_load(void)
 {
 	struct kobject *kobj;

@@ -208,26 +221,36 @@ static int secvar_sysfs_init(void)
 
 	rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
 	if (rc) {
-		kobject_put(secvar_kobj);
-		return -ENOMEM;
+		pr_err("Failed to create format object\n");
+		rc = -ENOMEM;
+		goto err;
 	}
 
 	secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
 	if (!secvar_kset) {
 		pr_err("sysfs kobject registration failed\n");
-		kobject_put(secvar_kobj);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto err;
 	}
 
 	rc = update_kobj_size();
 	if (rc) {
 		pr_err("Cannot read the size of the attribute\n");
-		return rc;
+		goto err;
+	}
+
+	rc = secvar_sysfs_config(secvar_kobj);
+	if (rc) {
+		pr_err("Failed to create config directory\n");
+		goto err;
 	}
 
 	secvar_sysfs_load();
 
 	return 0;
+err:
+	kobject_put(secvar_kobj);
+	return rc;
 }
 
 late_initcall(secvar_sysfs_init);

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #12 lore raw

PATCH v6 11/26 powerpc/secvar: Allow backend to populate static list of variable names

Currently, the list of variables is populated by calling secvar_ops->get_next() repeatedly, which is explicitly modelled on the OPAL API (including the keylen parameter).

For the upcoming PLPKS backend, we have a static list of variable names. It is messy to fit that into get_next(), so instead, let the backend put a NULL-terminated array of variable names into secvar_ops->var_names, which will be used if get_next() is undefined.

---

v3: New patch (ajd/mpe)

v6: Add newlines for better aesthetics (stefanb)

---
 arch/powerpc/include/asm/secvar.h  |  4 ++
 arch/powerpc/kernel/secvar-sysfs.c | 69 +++++++++++++++++++++---------
 2 files changed, 52 insertions(+), 21 deletions(-)

arch/powerpc/include/asm/secvar.h +4 -0

index 011a53a8076c..4828e0ab7e3c 100644
@@ -21,6 +21,10 @@ struct secvar_operations {
 	ssize_t (*format)(char *buf, size_t bufsize);
 	int (*max_size)(u64 *max_size);
 	const struct attribute **config_attrs;
+
+	// NULL-terminated array of fixed variable names
+	// Only used if get_next() isn't provided
+	const char * const *var_names;
 };
 
 #ifdef CONFIG_PPC_SECURE_BOOT

arch/powerpc/kernel/secvar-sysfs.c +48 ~~-21~~

95 lines not on this page · raw message

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:04 UTC #13 lore raw

PATCH v6 12/26 powerpc/secvar: Warn when PAGE_SIZE is smaller than max object size

Due to sysfs constraints, when writing to a variable, we can only handle writes of up to PAGE_SIZE.

It's possible that the maximum object size is larger than PAGE_SIZE, in which case, print a warning on boot so that the user is aware.

---

v3: New patch (ajd)

---
 arch/powerpc/kernel/secvar-sysfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/powerpc/kernel/secvar-sysfs.c +9 -0

index bfb19f22c6ba..6ba23b2bb9da 100644
@@ -225,6 +225,7 @@ static int secvar_sysfs_load_static(void)
 
 static int secvar_sysfs_init(void)
 {
+	u64 max_size;
 	int rc;
 
 	if (!secvar_ops) {

@@ -274,6 +275,14 @@ static int secvar_sysfs_init(void)
 		goto err;
 	}
 
+	// Due to sysfs limitations, we will only ever get a write buffer of
+	// up to 1 page in size. Print a warning if this is potentially going
+	// to cause problems, so that the user is aware.
+	secvar_ops->max_size(&max_size);
+	if (max_size > PAGE_SIZE)
+		pr_warn_ratelimited("PAGE_SIZE (%lu) is smaller than maximum object size (%llu), writes are limited to PAGE_SIZE\n",
+				    PAGE_SIZE, max_size);
+
 	return 0;
 err:
 	kobject_put(secvar_kobj);

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #14 lore raw

PATCH v6 13/26 powerpc/secvar: Don't print error on ENOENT when reading variables

If attempting to read the size or data attributes of a non-existent variable (which will be possible after a later patch to expose the PLPKS via the secvar interface), don't spam the kernel log with error messages. Only print errors for return codes that aren't ENOENT.

Reported-by Sudhakar Kuppusamy <[email protected]>

---

v3: New patch

---
 arch/powerpc/kernel/secvar-sysfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/powerpc/kernel/secvar-sysfs.c +4 -3

index 6ba23b2bb9da..eb3c053f323f 100644
@@ -43,8 +43,8 @@ static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
 
 	rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
 	if (rc) {
-		pr_err("Error retrieving %s variable size %d\n", kobj->name,
-		       rc);
+		if (rc != -ENOENT)
+			pr_err("Error retrieving %s variable size %d\n", kobj->name, rc);
 		return rc;
 	}

@@ -61,7 +61,8 @@ static ssize_t data_read(struct file *filep, struct kobject *kobj,
 
 	rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
 	if (rc) {
-		pr_err("Error getting %s variable size %d\n", kobj->name, rc);
+		if (rc != -ENOENT)
+			pr_err("Error getting %s variable size %d\n", kobj->name, rc);
 		return rc;
 	}
 	pr_debug("dsize is %llu\n", dsize);

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #15 lore raw

PATCH v6 14/26 powerpc/pseries: Move plpks.h to include directory

From: Russell Currey <[email protected]>

Move plpks.h from platforms/pseries/ to include/asm/. This is necessary for later patches to make use of the PLPKS from code in other subsystems.

---

v3: New patch

---
 .../powerpc/{platforms/pseries => include/asm}/plpks.h | 10 +++++++---
 arch/powerpc/platforms/pseries/plpks.c                 |  3 +--
 2 files changed, 8 insertions(+), 5 deletions(-)
 rename arch/powerpc/{platforms/pseries => include/asm}/plpks.h (89%)

arch/powerpc/include/asm/plpks.h renamed from arch/powerpc/platforms/pseries/plpks.h +7 -3

similarity index 89%
rename from arch/powerpc/platforms/pseries/plpks.h
rename to arch/powerpc/include/asm/plpks.h
index 275ccd86bfb5..8295502ee93b 100644
@@ -6,8 +6,10 @@
  * Platform keystore for pseries LPAR(PLPKS).
  */
 
-#ifndef _PSERIES_PLPKS_H
-#define _PSERIES_PLPKS_H
+#ifndef _ASM_POWERPC_PLPKS_H
+#define _ASM_POWERPC_PLPKS_H
+
+#ifdef CONFIG_PSERIES_PLPKS
 
 #include <linux/types.h>
 #include <linux/list.h>

@@ -68,4 +70,6 @@ int plpks_read_fw_var(struct plpks_var *var);
  */
 int plpks_read_bootloader_var(struct plpks_var *var);
 
-#endif
+#endif // CONFIG_PSERIES_PLPKS
+
+#endif // _ASM_POWERPC_PLPKS_H

arch/powerpc/platforms/pseries/plpks.c +1 -2

index a01cf2ff140a..13e6daadb179 100644
@@ -18,8 +18,7 @@
 #include <linux/types.h>
 #include <asm/hvcall.h>
 #include <asm/machdep.h>
-
-#include "plpks.h"
+#include <asm/plpks.h>
 
 #define PKS_FW_OWNER	     0x1
 #define PKS_BOOTLOADER_OWNER 0x2

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #16 lore raw

PATCH v6 15/26 powerpc/pseries: Move PLPKS constants to header file

From: Russell Currey <[email protected]>

Move the constants defined in plpks.c to plpks.h, and standardise their naming, so that PLPKS consumers can make use of them later on.

Co-developed-by Andrew Donnellan <[email protected]>

---

v3: New patch

---
 arch/powerpc/include/asm/plpks.h       | 36 +++++++++++++---
 arch/powerpc/platforms/pseries/plpks.c | 57 ++++++++++----------------
 2 files changed, 53 insertions(+), 40 deletions(-)

arch/powerpc/include/asm/plpks.h +31 -5

index 8295502ee93b..6466aadd7145 100644
@@ -14,14 +14,40 @@
 #include <linux/types.h>
 #include <linux/list.h>
 
-#define OSSECBOOTAUDIT 0x40000000
-#define OSSECBOOTENFORCE 0x20000000
-#define WORLDREADABLE 0x08000000
-#define SIGNEDUPDATE 0x01000000
+// Object policy flags from supported_policies
+#define PLPKS_OSSECBOOTAUDIT	PPC_BIT32(1) // OS secure boot must be audit/enforce
+#define PLPKS_OSSECBOOTENFORCE	PPC_BIT32(2) // OS secure boot must be enforce
+#define PLPKS_PWSET		PPC_BIT32(3) // No access without password set
+#define PLPKS_WORLDREADABLE	PPC_BIT32(4) // Readable without authentication
+#define PLPKS_IMMUTABLE		PPC_BIT32(5) // Once written, object cannot be removed
+#define PLPKS_TRANSIENT		PPC_BIT32(6) // Object does not persist through reboot
+#define PLPKS_SIGNEDUPDATE	PPC_BIT32(7) // Object can only be modified by signed updates
+#define PLPKS_HVPROVISIONED	PPC_BIT32(28) // Hypervisor has provisioned this object
 
-#define PLPKS_VAR_LINUX	0x02
+// Signature algorithm flags from signed_update_algorithms
+#define PLPKS_ALG_RSA2048	PPC_BIT(0)
+#define PLPKS_ALG_RSA4096	PPC_BIT(1)
+
+// Object label OS metadata flags
+#define PLPKS_VAR_LINUX		0x02
 #define PLPKS_VAR_COMMON	0x04
 
+// Flags for which consumer owns an object is owned by
+#define PLPKS_FW_OWNER			0x1
+#define PLPKS_BOOTLOADER_OWNER		0x2
+#define PLPKS_OS_OWNER			0x3
+
+// Flags for label metadata fields
+#define PLPKS_LABEL_VERSION		0
+#define PLPKS_MAX_LABEL_ATTR_SIZE	16
+#define PLPKS_MAX_NAME_SIZE		239
+#define PLPKS_MAX_DATA_SIZE		4000
+
+// Timeouts for PLPKS operations
+#define PLPKS_MAX_TIMEOUT		5000 // msec
+#define PLPKS_FLUSH_SLEEP		10 // msec
+#define PLPKS_FLUSH_SLEEP_RANGE		400
+
 struct plpks_var {
 	char *component;
 	u8 *name;

arch/powerpc/platforms/pseries/plpks.c +22 ~~-35~~

156 lines not on this page · raw message

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #17 lore raw

PATCH v6 16/26 powerpc/pseries: Expose PLPKS config values, support additional fields

From: Nayna Jain <[email protected]>

The plpks driver uses the H_PKS_GET_CONFIG hcall to retrieve configuration and status information about the PKS from the hypervisor.

Update _plpks_get_config() to handle some additional fields. Add getter functions to allow the PKS configuration information to be accessed from other files. Validate that the values we're getting comply with the spec.

While we're here, move the config struct in _plpks_get_config() off the stack - it's getting large and we also need to make sure it doesn't cross a page boundary.

Signed-off-by Nayna Jain <[email protected]>

[ajd: split patch, extend to support additional v3 API fields, minor fixes]

Co-developed-by Andrew Donnellan <[email protected]>

---

v3: Merge plpks fixes and signed update series with secvar series

Refresh config values in plpks_get_usedspace() (ajd)

Validate the config values being returned comply with spec (ruscur)

Return maxobjlabelsize as is (ruscur)

Move plpks.h to include/asm (ruscur)

Fix checkpatch checks (ruscur)

---
 arch/powerpc/include/asm/plpks.h       |  58 ++++++++++
 arch/powerpc/platforms/pseries/plpks.c | 149 +++++++++++++++++++++++--
 2 files changed, 195 insertions(+), 12 deletions(-)

arch/powerpc/include/asm/plpks.h +58 -0

index 6466aadd7145..7c5f51a9af7c 100644
@@ -96,6 +96,64 @@ int plpks_read_fw_var(struct plpks_var *var);
  */
 int plpks_read_bootloader_var(struct plpks_var *var);
 
+/**
+ * Returns if PKS is available on this LPAR.
+ */
+bool plpks_is_available(void);
+
+/**
+ * Returns version of the Platform KeyStore.
+ */
+u8 plpks_get_version(void);
+
+/**
+ * Returns hypervisor storage overhead per object, not including the size of
+ * the object or label. Only valid for config version >= 2
+ */
+u16 plpks_get_objoverhead(void);
+
+/**
+ * Returns maximum password size. Must be >= 32 bytes
+ */
+u16 plpks_get_maxpwsize(void);
+
+/**
+ * Returns maximum object size supported by Platform KeyStore.
+ */
+u16 plpks_get_maxobjectsize(void);
+
+/**
+ * Returns maximum object label size supported by Platform KeyStore.
+ */
+u16 plpks_get_maxobjectlabelsize(void);
+
+/**
+ * Returns total size of the configured Platform KeyStore.
+ */
+u32 plpks_get_totalsize(void);
+
+/**
+ * Returns used space from the total size of the Platform KeyStore.
+ */
+u32 plpks_get_usedspace(void);
+
+/**
+ * Returns bitmask of policies supported by the hypervisor.
+ */
+u32 plpks_get_supportedpolicies(void);
+
+/**
+ * Returns maximum byte size of a single object supported by the hypervisor.
+ * Only valid for config version >= 3
+ */
+u32 plpks_get_maxlargeobjectsize(void);
+
+/**
+ * Returns bitmask of signature algorithms supported for signed updates.
+ * Only valid for config version >= 3
+ */
+u64 plpks_get_signedupdatealgorithms(void);
+
 #endif // CONFIG_PSERIES_PLPKS
 
 #endif // _ASM_POWERPC_PLPKS_H

arch/powerpc/platforms/pseries/plpks.c +137 ~~-12~~

177 lines not on this page · raw message

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #18 lore raw

PATCH v6 17/26 powerpc/pseries: Implement signed update for PLPKS objects

From: Nayna Jain <[email protected]>

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 <[email protected]>

[ajd: split patch, add timeout handling and misc cleanups]

Co-developed-by Andrew Donnellan <[email protected]>

---

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)

v5: Drop the EXPORT_SYMBOL since we don't need it (npiggin)

Return an error if plpks_signed_update_var() is called with non-NULL component (npiggin)

---
 arch/powerpc/include/asm/hvcall.h      |  1 +
 arch/powerpc/include/asm/plpks.h       |  5 ++
 arch/powerpc/platforms/pseries/plpks.c | 74 ++++++++++++++++++++++++--
 3 files changed, 75 insertions(+), 5 deletions(-)

arch/powerpc/include/asm/hvcall.h +1 -0

index 95fd7f9485d5..c099780385dd 100644
@@ -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

arch/powerpc/include/asm/plpks.h +5 -0

index 7c5f51a9af7c..e7204e6c0ca4 100644
@@ -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

arch/powerpc/platforms/pseries/plpks.c +69 -5

102 lines not on this page · raw message

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #19 lore raw

PATCH v6 18/26 powerpc/pseries: Log hcall return codes for PLPKS debug

From: Russell Currey <[email protected]>

The plpks code converts hypervisor return codes into their Linux equivalents so that users can understand them. Having access to the original return codes is really useful for debugging, so add a pr_debug() so we don't lose information from the conversion.

---
 arch/powerpc/platforms/pseries/plpks.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/pseries/plpks.c +2 -0

index cee06fb9a370..e5755443d4a4 100644
@@ -117,6 +117,8 @@ static int pseries_status_to_err(int rc)
 		err = -EINVAL;
 	}
 
+	pr_debug("Converted hypervisor code %d to Linux %d\n", rc, err);
+
 	return err;
 }

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #20 lore raw

PATCH v6 19/26 powerpc/pseries: Make caller pass buffer to plpks_read_var()

Currently, plpks_read_var() allocates a buffer to pass to the H_PKS_READ_OBJECT hcall, then allocates another buffer into which the data is copied, and returns that buffer to the caller.

This is a bit over the top - while we probably still want to allocate a separate buffer to pass to the hypervisor in the hcall, we can let the caller allocate the final buffer and specify the size.

Don't allocate var->data in plpks_read_var(), instead expect the caller to allocate it. If the caller needs to discover the size, it can set var->data to NULL and var->datalen will be populated. Update header file to document this.

---

v3: New patch (mpe)

v6: Reword commit message (stefanb)

---
 arch/powerpc/include/asm/plpks.h       | 12 ++++++++++++
 arch/powerpc/platforms/pseries/plpks.c | 11 ++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

arch/powerpc/include/asm/plpks.h +12 -0

index e7204e6c0ca4..0c49969b0864 100644
@@ -88,16 +88,28 @@ int plpks_remove_var(char *component, u8 varos,
 
 /**
  * Returns the data for the specified os variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
  */
 int plpks_read_os_var(struct plpks_var *var);
 
 /**
  * Returns the data for the specified firmware variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
  */
 int plpks_read_fw_var(struct plpks_var *var);
 
 /**
  * Returns the data for the specified bootloader variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
  */
 int plpks_read_bootloader_var(struct plpks_var *var);

arch/powerpc/platforms/pseries/plpks.c +4 -7

index e5755443d4a4..926b6a927326 100644
@@ -581,17 +581,14 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
 		goto out_free_output;
 	}
 
-	if (var->datalen == 0 || var->datalen > retbuf[0])
+	if (!var->data || var->datalen > retbuf[0])
 		var->datalen = retbuf[0];
 
-	var->data = kzalloc(var->datalen, GFP_KERNEL);
-	if (!var->data) {
-		rc = -ENOMEM;
-		goto out_free_output;
-	}
 	var->policy = retbuf[1];
 
-	memcpy(var->data, output, var->datalen);
+	if (var->data)
+		memcpy(var->data, output, var->datalen);
+
 	rc = 0;
 
 out_free_output:

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:10 UTC #21 lore raw

PATCH v6 20/26 powerpc/pseries: Turn PSERIES_PLPKS into a hidden option

It seems a bit unnecessary for the PLPKS code to have a user-visible config option when it doesn't do anything on its own, and there's existing options for enabling Secure Boot-related features.

It should be enabled by PPC_SECURE_BOOT, which will eventually be what uses PLPKS to populate keyrings.

However, we can't get of the separate option completely, because it will also be used for SED Opal purposes.

Change PSERIES_PLPKS into a hidden option, which is selected by PPC_SECURE_BOOT.

---

v3: New patch

v5: Change the previous description into a comment (npiggin)

---
 arch/powerpc/Kconfig                   |  1 +
 arch/powerpc/platforms/pseries/Kconfig | 19 +++++++++----------
 2 files changed, 10 insertions(+), 10 deletions(-)

arch/powerpc/Kconfig +1 -0

index b8c4ac56bddc..d4ed46101bec 100644
@@ -1029,6 +1029,7 @@ config PPC_SECURE_BOOT
 	depends on PPC_POWERNV || PPC_PSERIES
 	depends on IMA_ARCH_POLICY
 	imply IMA_SECURE_AND_OR_TRUSTED_BOOT
+	select PSERIES_PLPKS if PPC_PSERIES
 	help
 	  Systems with firmware secure boot enabled need to define security
 	  policies to extend secure boot to the OS. This config allows a user

arch/powerpc/platforms/pseries/Kconfig +9 ~~-10~~

index a3b4d99567cb..e51d65969318 100644
@@ -151,16 +151,15 @@ config IBMEBUS
 
 config PSERIES_PLPKS
 	depends on PPC_PSERIES
-	bool "Support for the Platform Key Storage"
-	help
-	  PowerVM provides an isolated Platform Keystore(PKS) storage
-	  allocation for each LPAR with individually managed access
-	  controls to store sensitive information securely. It can be
-	  used to store asymmetric public keys or secrets as required
-	  by different usecases. Select this config to enable
-	  operating system interface to hypervisor to access this space.
-
-	  If unsure, select N.
+	bool
+	# PowerVM provides an isolated Platform Keystore (PKS) storage
+	# allocation for each LPAR with individually managed access
+	# controls to store sensitive information securely. It can be
+	# used to store asymmetric public keys or secrets as required
+	# by different usecases.
+	#
+	# This option is selected by in-kernel consumers that require
+	# access to the PKS.
 
 config PAPR_SCM
 	depends on PPC_PSERIES && MEMORY_HOTPLUG && LIBNVDIMM

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:05 UTC #22 lore raw

PATCH v6 21/26 powerpc/pseries: Clarify warning when PLPKS password already set

When the H_PKS_GEN_PASSWORD hcall returns H_IN_USE, operations that require authentication (i.e. anything other than reading a world-readable variable) will not work.

The current error message doesn't explain this clearly enough. Reword it to emphasise that authenticated operations will fail.

---

v6: New patch

---
 arch/powerpc/platforms/pseries/plpks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/platforms/pseries/plpks.c +1 -1

index 926b6a927326..01ae919b4497 100644
@@ -146,7 +146,7 @@ static int plpks_gen_password(void)
 		memcpy(ospassword, password, ospasswordlength);
 	} else {
 		if (rc == H_IN_USE) {
-			pr_warn("Password is already set for POWER LPAR Platform KeyStore\n");
+			pr_warn("Password already set - authenticated operations will fail\n");
 			rc = 0;
 		} else {
 			goto out;

Signature · 1 line

2.39.1

Stefan Berger

Reviewed-by on v6 21/26

2023-02-10 20:48 UTC #23 lore raw

Re: [PATCH v6 21/26] powerpc/pseries: Clarify warning when PLPKS password already set

Quoted 6 lines · Andrew Donnellan

When the H_PKS_GEN_PASSWORD hcall returns H_IN_USE, operations that require authentication (i.e. anything other than reading a world-readable variable) will not work.
The current error message doesn't explain this clearly enough. Reword it to emphasise that authenticated operations will fail.

typo: -> emphasize

Quoted 15 lines

Signed-off-by Andrew Donnellan <[email protected]>
---
v6: New patch
---
  arch/powerpc/platforms/pseries/plpks.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/plpks.c +0 -0
index 926b6a927326..01ae919b4497 100644

arch/powerpc/platforms/pseries/plpks.c static int plpks_gen_password(void) · line 152

Quoted 8 lines

arch/powerpc/platforms/pseries/plpks.c +1 -1

@@ -146,7 +146,7 @@ static int plpks_gen_password(void)
  		memcpy(ospassword, password, ospasswordlength);
  	} else {
  		if (rc == H_IN_USE) {
-			pr_warn("Password is already set for POWER LPAR Platform KeyStore\n");
+			pr_warn("Password already set - authenticated operations will fail\n");
  			rc = 0;
  		} else {
  			goto out;

Andrew Donnellan

author

2023-02-12 13:27 UTC #24 lore raw

Re: [PATCH v6 21/26] powerpc/pseries: Clarify warning when PLPKS password already set

Quoted 9 lines · Stefan Berger

Quoted 6 lines · Andrew Donnellan
When the H_PKS_GEN_PASSWORD hcall returns H_IN_USE, operations that require
authentication (i.e. anything other than reading a world-readable variable)
will not work.

Quoted 5 lines

Quoted 3 lines
The current error message doesn't explain this clearly enough. Reword it
to emphasise that authenticated operations will fail.
typo: -> emphasize

This commit message was written in en_AU. ;)

Signature · 2 lines

Andrew Donnellan    OzLabs, ADL Canberra
[email protected]   IBM Australia Limited

Andrew Donnellan

author

2023-02-10 08:05 UTC #25 lore raw

PATCH v6 22/26 powerpc/pseries: Add helper to get PLPKS password length

From: Russell Currey <[email protected]>

Add helper function to get the PLPKS password length. This will be used in a later patch to support passing the password between kernels over kexec.

---

v3: New patch

v5: Drop plpks_get_password() since we no longer need to expose it.

---
 arch/powerpc/include/asm/plpks.h       | 5 +++++
 arch/powerpc/platforms/pseries/plpks.c | 5 +++++
 2 files changed, 10 insertions(+)

arch/powerpc/include/asm/plpks.h +5 -0

index 0c49969b0864..757313e00521 100644
@@ -171,6 +171,11 @@ u32 plpks_get_maxlargeobjectsize(void);
  */
 u64 plpks_get_signedupdatealgorithms(void);
 
+/**
+ * Returns the length of the PLPKS password in bytes.
+ */
+u16 plpks_get_passwordlen(void);
+
 #endif // CONFIG_PSERIES_PLPKS
 
 #endif // _ASM_POWERPC_PLPKS_H

arch/powerpc/platforms/pseries/plpks.c +5 -0

index 01ae919b4497..671a10acaebf 100644
@@ -359,6 +359,11 @@ u64 plpks_get_signedupdatealgorithms(void)
 	return signedupdatealgorithms;
 }
 
+u16 plpks_get_passwordlen(void)
+{
+	return ospasswordlength;
+}
+
 bool plpks_is_available(void)
 {
 	int rc;

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #26 lore raw

PATCH v6 23/26 powerpc/pseries: Pass PLPKS password on kexec

From: Russell Currey <[email protected]>

Before interacting with the PLPKS, we ask the hypervisor to generate a password for the current boot, which is then required for most further PLPKS operations.

If we kexec into a new kernel, the new kernel will try and fail to generate a new password, as the password has already been set.

Pass the password through to the new kernel via the device tree, in /chosen/ibm,plpks-pw. Check for the presence of this property before trying to generate a new password - if it exists, use the existing password and remove it from the device tree.

This only works with the kexec_file_load() syscall, not the older kexec_load() syscall, however if you're using Secure Boot then you want to be using kexec_file_load() anyway.

---

v3: New patch

v4: Fix compile when CONFIG_PSERIES_PLPKS=n (snowpatch)

Fix error handling on fdt_path_offset() call (ruscur)

v5: Fix DT property name in commit message (npiggin)

Clear prop in FDT during init to prevent password exposure (mpe)

Rework to remove ifdefs from C code (npiggin)

v6: Rebase on top of 7294194b47e994753a86eee8cf1c61f3f36458a3 and
fc546faa559538fb312c77e055243ece18ab3288

Whitespace (stefanb)

Use more const (stefanb)

Get rid of FDT extra space allocation for node overhead, as it shouldn't be necessary (ruscur)

Note kexec_file_load() restriction in commit message

---
 arch/powerpc/include/asm/plpks.h       | 14 ++++++
 arch/powerpc/kernel/prom.c             |  4 ++
 arch/powerpc/kexec/file_load_64.c      | 18 +++++---
 arch/powerpc/platforms/pseries/plpks.c | 61 ++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 5 deletions(-)

arch/powerpc/include/asm/plpks.h +14 -0

index 757313e00521..23b77027c916 100644
@@ -176,6 +176,20 @@ u64 plpks_get_signedupdatealgorithms(void);
  */
 u16 plpks_get_passwordlen(void);
 
+/**
+ * Called in early init to retrieve and clear the PLPKS password from the DT.
+ */
+void plpks_early_init_devtree(void);
+
+/**
+ * Populates the FDT with the PLPKS password to prepare for kexec.
+ */
+int plpks_populate_fdt(void *fdt);
+#else // CONFIG_PSERIES_PLPKS
+static inline bool plpks_is_available(void) { return false; }
+static inline u16 plpks_get_passwordlen(void) { BUILD_BUG(); }
+static inline void plpks_early_init_devtree(void) { }
+static inline int plpks_populate_fdt(void *fdt) { BUILD_BUG(); }
 #endif // CONFIG_PSERIES_PLPKS
 
 #endif // _ASM_POWERPC_PLPKS_H

arch/powerpc/kernel/prom.c +4 -0

index 4f1c920aa13e..8a13b378770f 100644
@@ -56,6 +56,7 @@
 #include <asm/drmem.h>
 #include <asm/ultravisor.h>
 #include <asm/prom.h>
+#include <asm/plpks.h>
 
 #include <mm/mmu_decl.h>

@@ -893,6 +894,9 @@ void __init early_init_devtree(void *params)
 		powerpc_firmware_features |= FW_FEATURE_PS3_POSSIBLE;
 #endif
 
+	/* If kexec left a PLPKS password in the DT, get it and clear it */
+	plpks_early_init_devtree();
+
 	tm_init();
 
 	DBG(" <- early_init_devtree()\n");

arch/powerpc/kexec/file_load_64.c +13 -5

index 52085751f5f4..8a9469e1ce71 100644
@@ -27,6 +27,7 @@
 #include <asm/kexec_ranges.h>
 #include <asm/crashdump-ppc64.h>
 #include <asm/prom.h>
+#include <asm/plpks.h>
 
 struct umem_info {
 	u64 *buf;		/* data buffer for usable-memory property */

@@ -977,12 +978,17 @@ static unsigned int cpu_node_size(void)
  */
 unsigned int kexec_extra_fdt_size_ppc64(struct kimage *image)
 {
-	unsigned int cpu_nodes, extra_size;
+	unsigned int cpu_nodes, extra_size = 0;
 	struct device_node *dn;
 	u64 usm_entries;
 
+	// Budget some space for the password blob. There's already extra space
+	// for the key name
+	if (plpks_is_available())
+		extra_size += (unsigned int)plpks_get_passwordlen();
+
 	if (image->type != KEXEC_TYPE_CRASH)
-		return 0;
+		return extra_size;
 
 	/*
 	 * For kdump kernel, account for linux,usable-memory and

@@ -992,9 +998,7 @@ unsigned int kexec_extra_fdt_size_ppc64(struct kimage *image)
 	if (drmem_lmb_size()) {
 		usm_entries = ((memory_hotplug_max() / drmem_lmb_size()) +
 			       (2 * (resource_size(&crashk_res) / drmem_lmb_size())));
-		extra_size = (unsigned int)(usm_entries * sizeof(u64));
-	} else {
-		extra_size = 0;
+		extra_size += (unsigned int)(usm_entries * sizeof(u64));
 	}
 
 	/*

@@ -1233,6 +1237,10 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
 		}
 	}
 
+	// If we have PLPKS active, we need to provide the password to the new kernel
+	if (plpks_is_available())
+		ret = plpks_populate_fdt(fdt);
+
 out:
 	kfree(rmem);
 	kfree(umem);

arch/powerpc/platforms/pseries/plpks.c +61 -0

79 lines not on this page · raw message

Signature · 1 line

2.39.1

Stefan Berger

Reviewed-by on v6 23/26

2023-02-10 21:00 UTC #27 lore raw

Re: [PATCH v6 23/26] powerpc/pseries: Pass PLPKS password on kexec

Quoted 230 lines · Andrew Donnellan

From: Russell Currey <[email protected]>

Before interacting with the PLPKS, we ask the hypervisor to generate a password for the current boot, which is then required for most further PLPKS operations.

If we kexec into a new kernel, the new kernel will try and fail to generate a new password, as the password has already been set.

This only works with the kexec_file_load() syscall, not the older kexec_load() syscall, however if you're using Secure Boot then you want to be using kexec_file_load() anyway.

---

v3: New patch

v4: Fix compile when CONFIG_PSERIES_PLPKS=n (snowpatch)

Fix error handling on fdt_path_offset() call (ruscur)

v5: Fix DT property name in commit message (npiggin)

Clear prop in FDT during init to prevent password exposure (mpe)

Rework to remove ifdefs from C code (npiggin)

v6: Rebase on top of 7294194b47e994753a86eee8cf1c61f3f36458a3 and
fc546faa559538fb312c77e055243ece18ab3288

Whitespace (stefanb)

Use more const (stefanb)

Get rid of FDT extra space allocation for node overhead, as it shouldn't be necessary (ruscur)

Note kexec_file_load() restriction in commit message

---
  arch/powerpc/include/asm/plpks.h       | 14 ++++++
  arch/powerpc/kernel/prom.c             |  4 ++
  arch/powerpc/kexec/file_load_64.c      | 18 +++++---
  arch/powerpc/platforms/pseries/plpks.c | 61 ++++++++++++++++++++++++++
  4 files changed, 92 insertions(+), 5 deletions(-)

arch/powerpc/include/asm/plpks.h +14 -0

index 757313e00521..23b77027c916 100644
@@ -176,6 +176,20 @@ u64 plpks_get_signedupdatealgorithms(void);
   */
  u16 plpks_get_passwordlen(void);
 
+/**
+ * Called in early init to retrieve and clear the PLPKS password from the DT.
+ */
+void plpks_early_init_devtree(void);
+
+/**
+ * Populates the FDT with the PLPKS password to prepare for kexec.
+ */
+int plpks_populate_fdt(void *fdt);
+#else // CONFIG_PSERIES_PLPKS
+static inline bool plpks_is_available(void) { return false; }
+static inline u16 plpks_get_passwordlen(void) { BUILD_BUG(); }
+static inline void plpks_early_init_devtree(void) { }
+static inline int plpks_populate_fdt(void *fdt) { BUILD_BUG(); }
  #endif // CONFIG_PSERIES_PLPKS
 
  #endif // _ASM_POWERPC_PLPKS_H

arch/powerpc/kernel/prom.c +4 -0

index 4f1c920aa13e..8a13b378770f 100644
@@ -56,6 +56,7 @@
  #include <asm/drmem.h>
  #include <asm/ultravisor.h>
  #include <asm/prom.h>
+#include <asm/plpks.h>
 
  #include <mm/mmu_decl.h>

@@ -893,6 +894,9 @@ void __init early_init_devtree(void *params)
  		powerpc_firmware_features |= FW_FEATURE_PS3_POSSIBLE;
  #endif
 
+	/* If kexec left a PLPKS password in the DT, get it and clear it */
+	plpks_early_init_devtree();
+
  	tm_init();
 
  	DBG(" <- early_init_devtree()\n");

arch/powerpc/kexec/file_load_64.c +13 -5

index 52085751f5f4..8a9469e1ce71 100644
@@ -27,6 +27,7 @@
  #include <asm/kexec_ranges.h>
  #include <asm/crashdump-ppc64.h>
  #include <asm/prom.h>
+#include <asm/plpks.h>
 
  struct umem_info {
  	u64 *buf;		/* data buffer for usable-memory property */

@@ -977,12 +978,17 @@ static unsigned int cpu_node_size(void)
   */
  unsigned int kexec_extra_fdt_size_ppc64(struct kimage *image)
  {
-	unsigned int cpu_nodes, extra_size;
+	unsigned int cpu_nodes, extra_size = 0;
  	struct device_node *dn;
  	u64 usm_entries;
 
+	// Budget some space for the password blob. There's already extra space
+	// for the key name
+	if (plpks_is_available())
+		extra_size += (unsigned int)plpks_get_passwordlen();
+
  	if (image->type != KEXEC_TYPE_CRASH)
-		return 0;
+		return extra_size;
 
  	/*
  	 * For kdump kernel, account for linux,usable-memory and

@@ -992,9 +998,7 @@ unsigned int kexec_extra_fdt_size_ppc64(struct kimage *image)
  	if (drmem_lmb_size()) {
  		usm_entries = ((memory_hotplug_max() / drmem_lmb_size()) +
  			       (2 * (resource_size(&crashk_res) / drmem_lmb_size())));
-		extra_size = (unsigned int)(usm_entries * sizeof(u64));
-	} else {
-		extra_size = 0;
+		extra_size += (unsigned int)(usm_entries * sizeof(u64));
  	}
 
  	/*

@@ -1233,6 +1237,10 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
  		}
  	}
 
+	// If we have PLPKS active, we need to provide the password to the new kernel
+	if (plpks_is_available())
+		ret = plpks_populate_fdt(fdt);
+
  out:
  	kfree(rmem);
  	kfree(umem);

arch/powerpc/platforms/pseries/plpks.c +54 -0

index 671a10acaebf..cdf09e5bd741 100644
@@ -16,6 +16,9 @@
  #include <linux/slab.h>
  #include <linux/string.h>
  #include <linux/types.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+#include <linux/memblock.h>
  #include <asm/hvcall.h>
  #include <asm/machdep.h>
  #include <asm/plpks.h>

@@ -128,6 +131,12 @@ static int plpks_gen_password(void)
  	u8 *password, consumer = PLPKS_OS_OWNER;
  	int rc;
 
+	// If we booted from kexec, we could be reusing an existing password already
+	if (ospassword) {
+		pr_debug("Password of length %u already in use\n", ospasswordlength);
+		return 0;
+	}
+
  	// The password must not cross a page boundary, so we align to the next power of 2
  	password = kzalloc(roundup_pow_of_two(maxpwsize), GFP_KERNEL);
  	if (!password)

@@ -621,6 +630,58 @@ int plpks_read_bootloader_var(struct plpks_var *var)
  	return plpks_read_var(PLPKS_BOOTLOADER_OWNER, var);
  }
 
+int plpks_populate_fdt(void *fdt)
+{
+	int chosen_offset = fdt_path_offset(fdt, "/chosen");
+
+	if (chosen_offset < 0) {
+		pr_err("Can't find chosen node: %s\n",
+		       fdt_strerror(chosen_offset));
+		return chosen_offset;
+	}
+
+	return fdt_setprop(fdt, chosen_offset, "ibm,plpks-pw", ospassword, ospasswordlength);
+}
+
+// Once a password is registered with the hypervisor it cannot be cleared without
+// rebooting the LPAR, so to keep using the PLPKS across kexec boots we need to
+// recover the previous password from the FDT.
+//
+// There are a few challenges here.  We don't want the password to be visible to
+// users, so we need to clear it from the FDT.  This has to be done in early boot.
+// Clearing it from the FDT would make the FDT's checksum invalid, so we have to
+// manually cause the checksum to be recalculated.
+void __init plpks_early_init_devtree(void)
+{
+	void *fdt = initial_boot_params;
+	int chosen_node = fdt_path_offset(fdt, "/chosen");
+	const u8 *password;
+	int len;
+
+	if (chosen_node < 0)
+		return;
+
+	password = fdt_getprop(fdt, chosen_node, "ibm,plpks-pw", &len);
+	if (len <= 0) {
+		pr_debug("Couldn't find ibm,plpks-pw node.\n");
+		return;
+	}
+
+	ospassword = memblock_alloc_raw(len, SMP_CACHE_BYTES);
+	if (!ospassword) {
+		pr_err("Error allocating memory for password.\n");
+		goto out;
+	}
+
+	memcpy(ospassword, password, len);
+	ospasswordlength = (u16)len;

arch/powerpc/platforms/pseries/plpks.c int plpks_read_bootloader_var(struct plpks_var *var) · line 687

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks.c +7 -0

+
+out:
+	fdt_nop_property(fdt, chosen_node, "ibm,plpks-pw");
+	// Since we've cleared the password, we must update the FDT checksum
+	early_init_dt_verify(fdt);
+}
+
  static __init int pseries_plpks_init(void)
  {
  	int rc;

Andrew Donnellan

author

2023-02-10 08:06 UTC #28 lore raw

PATCH v6 24/26 powerpc/pseries: Implement secvars for dynamic secure boot

From: Russell Currey <[email protected]>

The pseries platform can support dynamic secure boot (i.e. secure boot using user-defined keys) using variables contained with the PowerVM LPAR Platform KeyStore (PLPKS). Using the powerpc secvar API, expose the relevant variables for pseries dynamic secure boot through the existing secvar filesystem layout.

The relevant variables for dynamic secure boot are signed in the keystore, and can only be modified using the H_PKS_SIGNED_UPDATE hcall. Object labels in the keystore are encoded using ucs2 format. With our fixed variable names we don't have to care about encoding outside of the necessary byte padding.

When a user writes to a variable, the first 8 bytes of data must contain the signed update flags as defined by the hypervisor.

When a user reads a variable, the first 4 bytes of data contain the policies defined for the object.

Limitations exist due to the underlying implementation of sysfs binary attributes, as is the case for the OPAL secvar implementation - partial writes are unsupported and writes cannot be larger than PAGE_SIZE. (Even when using bin_attributes, which can be larger than a single page, sysfs only gives us one page's worth of write buffer at a time, and the hypervisor does not expose an interface for partial writes.)

Co-developed-by Nayna Jain <[email protected]>

Signed-off-by Nayna Jain <[email protected]>

Co-developed-by Andrew Donnellan <[email protected]>

---

v2: Remove unnecessary config vars from sysfs and document the others,
thanks to review from Greg. If we end up needing to expose more, we can add them later and update the docs.

Use sysfs_emit() instead of sprintf(), thanks to Greg.

Change the size of the sysfs binary attributes to include the 8-byte flags header, preventing truncation of large writes.

v3: plpks_set_variable(): pass var to plpks_signed_update_var() as a
pointer (mpe)

Update copyright date (ajd)

Consistent comment style (ajd)

Change device_initcall() to machine_arch_initcall(pseries...) so we don't try to load on powernv and kill the machine (mpe)

Add config attributes into plpks_secvar_ops (mpe)

Get rid of PLPKS_SECVAR_COUNT macro (mpe)

Reworded descriptions in ABI documentation (mpe)

Switch to using secvar_ops->var_names rather than secvar_ops->get_next() (ajd/mpe)

Optimise allocation/copying of buffers (mpe)

Elaborate the comment documenting the "format" string (mpe)

Return -EIO on errors in the read case (mpe)

Add "grubdbx" variable (Sudhakar Kuppusamy)

Use utf8s_to_utf16s() rather than our own "UCS-2" conversion code (mpe)

Change uint64_t to u64 (mpe)

Fix SB_VERSION data length (ruscur)

Stop prepending policy data on read (ruscur)

Enforce max format length on format string (not strictly needed, but makes the length limit clear) (ajd)

Update include of plpks.h to reflect new path (ruscur)

Consistent constant naming scheme (ruscur)

v4: Return set_secvar_ops() return code

Pass buffer size to plpks_secvar_format() (stefanb, npiggin)

Add missing null check (stefanb)

Add comment to commit message explaining PAGE_SIZE write limit (joel)

v5: Add comment explaining why we use "key_len - 1" (npiggin)

Use strlen(var.name) instead of hardcoding 10 as length of "SB_VERSION" (npiggin)

Improve comments about use of SB_VERSION and format string (npiggin)

Change "+ 8" to "+ sizeof(u64)" when accounting for flags size in working out file's max size (npiggin)

Compile plpks-secvar.c based on CONFIG_PPC_SECURE_BOOT, not CONFIG_PPC_SECVAR_SYSFS, as the secvar backend is needed for loading keys into keyrings even if the sysfs interface is disabled (ajd)

v6: Update date in ABI docs (stefanb)

Get rid of 1 byte kzalloc (npiggin)

---
 Documentation/ABI/testing/sysfs-secvar        |  75 +++++-
 arch/powerpc/platforms/pseries/Makefile       |   4 +-
 arch/powerpc/platforms/pseries/plpks-secvar.c | 218 ++++++++++++++++++
 3 files changed, 294 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/platforms/pseries/plpks-secvar.c

Documentation/ABI/testing/sysfs-secvar +74 -1

index feebb8c57294..857cf12b0904 100644
@@ -18,6 +18,14 @@ Description:	A string indicating which backend is in use by the firmware.
 		This determines the format of the variable and the accepted
 		format of variable updates.
 
+		On powernv/OPAL, this value is provided by the OPAL 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.
+
 What:		/sys/firmware/secvar/vars/<variable name>
 Date:		August 2019
 Contact:	Nayna Jain <[email protected]>

@@ -34,7 +42,7 @@ Description:	An integer representation of the size of the content of the
 
 What:		/sys/firmware/secvar/vars/<variable_name>/data
 Date:		August 2019
-Contact:	Nayna Jain h<[email protected]>
+Contact:	Nayna Jain <[email protected]>
 Description:	A read-only file containing the value of the variable. The size
 		of the file represents the maximum size of the variable data.

@@ -44,3 +52,68 @@ Contact:	Nayna Jain <[email protected]>
 Description:	A write-only file that is used to submit the new value for the
 		variable. The size of the file represents the maximum size of
 		the variable data that can be written.
+
+What:		/sys/firmware/secvar/config
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	This optional directory contains read-only config attributes as
+		defined by the secure variable implementation.  All data is in
+		ASCII format. The directory is only created if the backing
+		implementation provides variables to populate it, which at
+		present is only PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/version
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Config version as reported by the hypervisor in ASCII decimal
+		format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/max_object_size
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Maximum allowed size of	objects in the keystore in bytes,
+		represented in ASCII decimal format.
+
+		This is not necessarily the same as the max size that can be
+		written to an update file as writes can contain more than
+		object data, you should use the size of the update file for
+		that purpose.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/total_size
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Total size of the PLPKS in bytes, represented in ASCII decimal
+		format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/used_space
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Current space consumed by the key store, in bytes, represented
+		in ASCII decimal format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/supported_policies
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Bitmask of supported policy flags by the hypervisor,
+		represented as an 8 byte hexadecimal ASCII string. Consult the
+		hypervisor documentation for what these flags are.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/signed_update_algorithms
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Bitmask of flags indicating which algorithms the hypervisor
+		supports for signed update of objects, represented as a 16 byte
+		hexadecimal ASCII string. Consult the hypervisor documentation
+		for what these flags mean.
+
+		Currently only provided by PLPKS on the pseries platform.

arch/powerpc/platforms/pseries/Makefile +2 -2

index 92310202bdd7..20a0f3c3fe04 100644
@@ -27,8 +27,8 @@ obj-$(CONFIG_PAPR_SCM)		+= papr_scm.o
 obj-$(CONFIG_PPC_SPLPAR)	+= vphn.o
 obj-$(CONFIG_PPC_SVM)		+= svm.o
 obj-$(CONFIG_FA_DUMP)		+= rtas-fadump.o
-obj-$(CONFIG_PSERIES_PLPKS) += plpks.o
-
+obj-$(CONFIG_PSERIES_PLPKS)	+= plpks.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= plpks-secvar.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_PPC_VAS)		+= vas.o vas-sysfs.o

arch/powerpc/platforms/pseries/plpks-secvar.c new file +218 -0

218 lines not on this page · raw message

Signature · 1 line

2.39.1

Stefan Berger

2023-02-10 21:23 UTC #29 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 364 lines · Andrew Donnellan

From: Russell Currey <[email protected]>

When a user writes to a variable, the first 8 bytes of data must contain the signed update flags as defined by the hypervisor.

When a user reads a variable, the first 4 bytes of data contain the policies defined for the object.

Co-developed-by Nayna Jain <[email protected]>

Signed-off-by Nayna Jain <[email protected]>

Co-developed-by Andrew Donnellan <[email protected]>

In reply to Stefan Berger

---

v2: Remove unnecessary config vars from sysfs and document the others,
thanks to review from Greg. If we end up needing to expose more, we can add them later and update the docs.

Use sysfs_emit() instead of sprintf(), thanks to Greg.

Change the size of the sysfs binary attributes to include the 8-byte flags header, preventing truncation of large writes.

v3: plpks_set_variable(): pass var to plpks_signed_update_var() as a
pointer (mpe)

Update copyright date (ajd)

Consistent comment style (ajd)

Change device_initcall() to machine_arch_initcall(pseries...) so we don't try to load on powernv and kill the machine (mpe)

Add config attributes into plpks_secvar_ops (mpe)

Get rid of PLPKS_SECVAR_COUNT macro (mpe)

Reworded descriptions in ABI documentation (mpe)

Switch to using secvar_ops->var_names rather than secvar_ops->get_next() (ajd/mpe)

Optimise allocation/copying of buffers (mpe)

Elaborate the comment documenting the "format" string (mpe)

Return -EIO on errors in the read case (mpe)

Add "grubdbx" variable (Sudhakar Kuppusamy)

Use utf8s_to_utf16s() rather than our own "UCS-2" conversion code (mpe)

Change uint64_t to u64 (mpe)

Fix SB_VERSION data length (ruscur)

Stop prepending policy data on read (ruscur)

Enforce max format length on format string (not strictly needed, but makes the length limit clear) (ajd)

Update include of plpks.h to reflect new path (ruscur)

Consistent constant naming scheme (ruscur)

v4: Return set_secvar_ops() return code

Pass buffer size to plpks_secvar_format() (stefanb, npiggin)

Add missing null check (stefanb)

Add comment to commit message explaining PAGE_SIZE write limit (joel)

v5: Add comment explaining why we use "key_len - 1" (npiggin)

Use strlen(var.name) instead of hardcoding 10 as length of "SB_VERSION" (npiggin)

Improve comments about use of SB_VERSION and format string (npiggin)

Change "+ 8" to "+ sizeof(u64)" when accounting for flags size in working out file's max size (npiggin)

Compile plpks-secvar.c based on CONFIG_PPC_SECURE_BOOT, not CONFIG_PPC_SECVAR_SYSFS, as the secvar backend is needed for loading keys into keyrings even if the sysfs interface is disabled (ajd)

v6: Update date in ABI docs (stefanb)

Get rid of 1 byte kzalloc (npiggin)

---
  Documentation/ABI/testing/sysfs-secvar        |  75 +++++-
  arch/powerpc/platforms/pseries/Makefile       |   4 +-
  arch/powerpc/platforms/pseries/plpks-secvar.c | 218 ++++++++++++++++++
  3 files changed, 294 insertions(+), 3 deletions(-)

create mode 100644 arch/powerpc/platforms/pseries/plpks-secvar.c

Documentation/ABI/testing/sysfs-secvar +74 -1

index feebb8c57294..857cf12b0904 100644
@@ -18,6 +18,14 @@ Description:	A string indicating which backend is in use by the firmware.
  		This determines the format of the variable and the accepted
  		format of variable updates.
 
+		On powernv/OPAL, this value is provided by the OPAL 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.
+
  What:		/sys/firmware/secvar/vars/<variable name>
  Date:		August 2019
  Contact:	Nayna Jain <[email protected]>

@@ -34,7 +42,7 @@ Description:	An integer representation of the size of the content of the
 
  What:		/sys/firmware/secvar/vars/<variable_name>/data
  Date:		August 2019
-Contact:	Nayna Jain h<[email protected]>
+Contact:	Nayna Jain <[email protected]>
  Description:	A read-only file containing the value of the variable. The size
  		of the file represents the maximum size of the variable data.

@@ -44,3 +52,68 @@ Contact:	Nayna Jain <[email protected]>
  Description:	A write-only file that is used to submit the new value for the
  		variable. The size of the file represents the maximum size of
  		the variable data that can be written.
+
+What:		/sys/firmware/secvar/config
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	This optional directory contains read-only config attributes as
+		defined by the secure variable implementation.  All data is in
+		ASCII format. The directory is only created if the backing
+		implementation provides variables to populate it, which at
+		present is only PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/version
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Config version as reported by the hypervisor in ASCII decimal
+		format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/max_object_size
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Maximum allowed size of	objects in the keystore in bytes,
+		represented in ASCII decimal format.
+
+		This is not necessarily the same as the max size that can be
+		written to an update file as writes can contain more than
+		object data, you should use the size of the update file for
+		that purpose.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/total_size
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Total size of the PLPKS in bytes, represented in ASCII decimal
+		format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/used_space
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Current space consumed by the key store, in bytes, represented
+		in ASCII decimal format.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/supported_policies
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Bitmask of supported policy flags by the hypervisor,
+		represented as an 8 byte hexadecimal ASCII string. Consult the
+		hypervisor documentation for what these flags are.
+
+		Currently only provided by PLPKS on the pseries platform.
+
+What:		/sys/firmware/secvar/config/signed_update_algorithms
+Date:		February 2023
+Contact:	Nayna Jain <[email protected]>
+Description:	Bitmask of flags indicating which algorithms the hypervisor
+		supports for signed update of objects, represented as a 16 byte
+		hexadecimal ASCII string. Consult the hypervisor documentation
+		for what these flags mean.
+
+		Currently only provided by PLPKS on the pseries platform.

arch/powerpc/platforms/pseries/Makefile +2 -2

index 92310202bdd7..20a0f3c3fe04 100644
@@ -27,8 +27,8 @@ obj-$(CONFIG_PAPR_SCM)		+= papr_scm.o
  obj-$(CONFIG_PPC_SPLPAR)	+= vphn.o
  obj-$(CONFIG_PPC_SVM)		+= svm.o
  obj-$(CONFIG_FA_DUMP)		+= rtas-fadump.o
-obj-$(CONFIG_PSERIES_PLPKS) += plpks.o
-
+obj-$(CONFIG_PSERIES_PLPKS)	+= plpks.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= plpks-secvar.o
  obj-$(CONFIG_SUSPEND)		+= suspend.o
  obj-$(CONFIG_PPC_VAS)		+= vas.o vas-sysfs.o

arch/powerpc/platforms/pseries/plpks-secvar.c new file +128 -0

new file mode 100644
index 000000000000..98d0f2b31e0d
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+// Secure variable implementation using the PowerVM LPAR Platform KeyStore (PLPKS)
+//
+// Copyright 2022, 2023 IBM Corporation
+// Authors: Russell Currey
+//          Andrew Donnellan
+//          Nayna Jain
+
+#define pr_fmt(fmt) "secvar: "fmt
+
+#include <linux/printk.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/kobject.h>
+#include <linux/nls.h>
+#include <asm/machdep.h>
+#include <asm/secvar.h>
+#include <asm/plpks.h>
+
+// Config attributes for sysfs
+#define PLPKS_CONFIG_ATTR(name, fmt, func)			\
+	static ssize_t name##_show(struct kobject *kobj,	\
+				   struct kobj_attribute *attr,	\
+				   char *buf)			\
+	{							\
+		return sysfs_emit(buf, fmt, func());		\
+	}							\
+	static struct kobj_attribute attr_##name = __ATTR_RO(name)
+
+PLPKS_CONFIG_ATTR(version, "%u\n", plpks_get_version);
+PLPKS_CONFIG_ATTR(max_object_size, "%u\n", plpks_get_maxobjectsize);
+PLPKS_CONFIG_ATTR(total_size, "%u\n", plpks_get_totalsize);
+PLPKS_CONFIG_ATTR(used_space, "%u\n", plpks_get_usedspace);
+PLPKS_CONFIG_ATTR(supported_policies, "%08x\n", plpks_get_supportedpolicies);
+PLPKS_CONFIG_ATTR(signed_update_algorithms, "%016llx\n", plpks_get_signedupdatealgorithms);
+
+static const struct attribute *config_attrs[] = {
+	&attr_version.attr,
+	&attr_max_object_size.attr,
+	&attr_total_size.attr,
+	&attr_used_space.attr,
+	&attr_supported_policies.attr,
+	&attr_signed_update_algorithms.attr,
+	NULL,
+};
+
+static u32 get_policy(const char *name)
+{
+	if ((strcmp(name, "db") == 0) ||
+	    (strcmp(name, "dbx") == 0) ||
+	    (strcmp(name, "grubdb") == 0) ||
+	    (strcmp(name, "grubdbx") == 0) ||
+	    (strcmp(name, "sbat") == 0))
+		return (PLPKS_WORLDREADABLE | PLPKS_SIGNEDUPDATE);
+	else
+		return PLPKS_SIGNEDUPDATE;
+}
+
+static const char * const plpks_var_names[] = {
+	"PK",
+	"KEK",
+	"db",
+	"dbx",
+	"grubdb",
+	"grubdbx",
+	"sbat",
+	"moduledb",
+	"trustedcadb",
+	NULL,
+};
+
+static int plpks_get_variable(const char *key, u64 key_len, u8 *data,
+			      u64 *data_size)
+{
+	struct plpks_var var = {0};
+	int rc = 0;
+
+	// We subtract 1 from key_len because we don't need to include the
+	// null terminator at the end of the string
+	var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL);
+	if (!var.name)
+		return -ENOMEM;
+	rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name,
+			     key_len - 1);
+	if (rc < 0)
+		goto err;
+	var.namelen = rc * 2;
+
+	var.os = PLPKS_VAR_LINUX;
+	if (data) {
+		var.data = data;
+		var.datalen = *data_size;
+	}
+	rc = plpks_read_os_var(&var);
+
+	if (rc)
+		goto err;
+
+	*data_size = var.datalen;
+
+err:
+	kfree(var.name);
+	if (rc && rc != -ENOENT) {
+		pr_err("Failed to read variable '%s': %d\n", key, rc);
+		// Return -EIO since userspace probably doesn't care about the
+		// specific error
+		rc = -EIO;
+	}
+	return rc;
+}
+
+static int plpks_set_variable(const char *key, u64 key_len, u8 *data,
+			      u64 data_size)
+{
+	struct plpks_var var = {0};
+	int rc = 0;
+	u64 flags;
+
+	// Secure variables need to be prefixed with 8 bytes of flags.
+	// We only want to perform the write if we have at least one byte of data.
+	if (data_size <= sizeof(flags))
+		return -EINVAL;
+
+	// We subtract 1 from key_len because we don't need to include the
+	// null terminator at the end of the string

arch/powerpc/platforms/pseries/plpks-secvar.c line 138

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0

+	var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL);
+	if (!var.name)
+		return -ENOMEM;
+	rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name,
+			     key_len - 1);
+	if (rc < 0)
+		goto err;
+	var.namelen = rc * 2;
+
+	memcpy(&flags, data, sizeof(flags));

conversion from bytestream to integer: I think in this case it would be better to use

flags = cpu_to_be64p((__u64*)data);

so that the flags always in hypervisor/big endian format

Quoted 17 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +17 -0

+
+	var.datalen = data_size - sizeof(flags);
+	var.data = data + sizeof(flags);
+	var.os = PLPKS_VAR_LINUX;
+	var.policy = get_policy(key);
+
+	// Unlike in the read case, the plpks error code can be useful to
+	// userspace on write, so we return it rather than just -EIO
+	rc = plpks_signed_update_var(&var, flags);
+
+err:
+	kfree(var.name);
+	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.

arch/powerpc/platforms/pseries/plpks-secvar.c line 165

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0

+// 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)
+{
+	struct plpks_var var = {0};
+	ssize_t ret;
+	u8 version;
+
+	var.component = NULL;

Since it's initialized with {0} this is not necessary.

Quoted 12 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +12 -0

+	// 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 {

arch/powerpc/platforms/pseries/plpks-secvar.c line 187

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0

+			pr_err("Error %ld reading SB_VERSION from firmware\n", ret);
+			ret = -EIO;
+		}
+		goto err;
+	}
+
+	ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", version);
+
+err:
+	kfree(var.data);

remove the kfree()

Quoted 31 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +31 -0

+	return ret;
+}
+
+static int plpks_max_size(u64 *max_size)
+{
+	// The max object size reported by the hypervisor is accurate for the
+	// object itself, but we use the first 8 bytes of data on write as the
+	// signed update flags, so the max size a user can write is larger.
+	*max_size = (u64)plpks_get_maxobjectsize() + sizeof(u64);
+
+	return 0;
+}
+
+
+static const struct secvar_operations plpks_secvar_ops = {
+	.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 int plpks_secvar_init(void)
+{
+	if (!plpks_is_available())
+		return -ENODEV;
+
+	return set_secvar_ops(&plpks_secvar_ops);
+}
+machine_device_initcall(pseries, plpks_secvar_init);

Stefan Berger

2023-02-10 21:28 UTC #30 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 35 lines · Stefan Berger

Quoted 6 lines
arch/powerpc/platforms/pseries/plpks-secvar.c new file +3 -0
+
+// 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.
arch/powerpc/platforms/pseries/plpks-secvar.c line 165
Quoted 10 lines
arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0
+// 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)
+{
+    struct plpks_var var = {0};
+    ssize_t ret;
+    u8 version;
+
+    var.component = NULL;
Since it's initialized with {0} this is not necessary.
Quoted 16 lines
+    // 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;

Quoted 8 lines

arch/powerpc/platforms/pseries/plpks-secvar.c line 187
Quoted 6 lines
+ }
arch/powerpc/platforms/pseries/plpks-secvar.c new file +5 -0
+
+    ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", version);
+
+err:
+    kfree(var.data);
remove the kfree()

Actually don't remove it but it should probably be

if (var.data != &version)

    kfree(var.data);

Quoted 4 lines

Quoted 4 lines
+ return ret;
(quoted diff) +2 -0
+}
+

Andrew Donnellan

author

2023-02-12 13:33 UTC #31 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 10 lines · Stefan Berger

Quoted 4 lines
Quoted 2 lines
+err:
+ kfree(var.data);
remove the kfree()
Actually don't remove it but it should probably be
if (var.data != &version)
kfree(var.data);

Argh, thanks for catching this.

I don't think the condition is needed - we can assume the var.data is unmodified.

mpe, are you able to fix this up in merge?

Signature · 2 lines

Andrew Donnellan    OzLabs, ADL Canberra
[email protected]   IBM Australia Limited

Michael Ellerman

2023-02-13 05:27 UTC #32 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 12 lines · Andrew Donnellan

Quoted 10 lines · Stefan Berger
Quoted 4 lines
Quoted 2 lines
+err:
+ kfree(var.data);
remove the kfree()
Actually don't remove it but it should probably be
if (var.data != &version)
kfree(var.data);

Quoted 6 lines

Argh, thanks for catching this.
I don't think the condition is needed - we can assume the var.data is unmodified.
mpe, are you able to fix this up in merge?

Yeah, can you reply here with the delta you want applied.

cheers

Andrew Donnellan

author

2023-02-13 07:07 UTC #33 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 19 lines · Michael Ellerman

Quoted 18 lines · Andrew Donnellan
Quoted 10 lines · Stefan Berger
Quoted 4 lines
Quoted 2 lines
+err:
+ kfree(var.data);
remove the kfree()
Actually don't remove it but it should probably be
if (var.data != &version)
kfree(var.data);
Argh, thanks for catching this.
I don't think the condition is needed - we can assume the var.data is
unmodified.

Quoted 5 lines

Quoted 1 line
mpe, are you able to fix this up in merge?
Yeah, can you reply here with the delta you want applied.
cheers

(quoted diff) +0 -0

b/arch/powerpc/platforms/pseries/plpks-secvar.c
index 98d0f2b31e0d..bdfe63bc8705 100644

arch/powerpc/platforms/pseries/plpks-secvar.c +0 -0

@@ -135,7 +135,7 @@ static int plpks_set_variable(const char *key, u64

key_len, u8 *data,
goto err;
var.namelen = rc * 2;

- memcpy(&flags, data, sizeof(flags));
+ flags = cpu_to_be64p((__u64 *)data);

var.datalen = data_size - sizeof(flags);
var.data = data + sizeof(flags);
@@ -184,7 +184,6 @@ static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", version);

err:
- kfree(var.data);
return ret;
}

Signature · 2 lines

Andrew Donnellan    OzLabs, ADL Canberra
[email protected]   IBM Australia Limited

Andrew Donnellan

author

2023-02-13 07:06 UTC #34 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 8 lines · Stefan Berger

Quoted 1 line
+ memcpy(&flags, data, sizeof(flags));
conversion from bytestream to integer: I think in this case it would be better to use
flags = cpu_to_be64p((__u64*)data);
so that the flags always in hypervisor/big endian format

Thanks for catching this - it turns out we weren't properly testing the one flag that exists (append vs replace) in our test script, so I didn't notice this.

Signature · 2 lines

Andrew Donnellan    OzLabs, ADL Canberra
[email protected]   IBM Australia Limited

Michael Ellerman

2023-02-13 11:32 UTC #35 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

In reply to Stefan Berger

Quoted 2 lines · Stefan Berger

Quoted 1 line · Andrew Donnellan
From: Russell Currey <[email protected]>

...

Quoted 27 lines

Quoted 14 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +14 -0

+static int plpks_set_variable(const char *key, u64 key_len, u8 *data,
+			      u64 data_size)
+{
+	struct plpks_var var = {0};
+	int rc = 0;
+	u64 flags;
+
+	// Secure variables need to be prefixed with 8 bytes of flags.
+	// We only want to perform the write if we have at least one byte of data.
+	if (data_size <= sizeof(flags))
+		return -EINVAL;
+
+	// We subtract 1 from key_len because we don't need to include the
+	// null terminator at the end of the string

arch/powerpc/platforms/pseries/plpks-secvar.c line 138

Quoted 10 lines

arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0

+	var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL);
+	if (!var.name)
+		return -ENOMEM;
+	rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name,
+			     key_len - 1);
+	if (rc < 0)
+		goto err;
+	var.namelen = rc * 2;
+
+	memcpy(&flags, data, sizeof(flags));

conversion from bytestream to integer: I think in this case it would be better to use

Quoted 3 lines

flags = cpu_to_be64p((__u64*)data);
so that the flags always in hypervisor/big endian format

I don't think it's correct to byte swap the flags here. They must be in big endian format, but that's up to the caller.

The powernv secvar backend doesn't byte swap the flags, if the pseries one did then the final content of the variable, written either by phyp or OPAL, would differ depending on which backend is active.

Or am I missing something?

cheers

Stefan Berger

2023-02-13 13:52 UTC #36 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 35 lines · Michael Ellerman

Quoted 2 lines · Stefan Berger
Quoted 1 line · Andrew Donnellan
From: Russell Currey <[email protected]>
...
Quoted 27 lines
Quoted 14 lines
arch/powerpc/platforms/pseries/plpks-secvar.c new file +14 -0
+static int plpks_set_variable(const char *key, u64 key_len, u8 *data,
+			      u64 data_size)
+{
+	struct plpks_var var = {0};
+	int rc = 0;
+	u64 flags;
+
+	// Secure variables need to be prefixed with 8 bytes of flags.
+	// We only want to perform the write if we have at least one byte of data.
+	if (data_size <= sizeof(flags))
+		return -EINVAL;
+
+	// We subtract 1 from key_len because we don't need to include the
+	// null terminator at the end of the string
arch/powerpc/platforms/pseries/plpks-secvar.c line 138
Quoted 10 lines
arch/powerpc/platforms/pseries/plpks-secvar.c new file +10 -0
+	var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL);
+	if (!var.name)
+		return -ENOMEM;
+	rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name,
+			     key_len - 1);
+	if (rc < 0)
+		goto err;
+	var.namelen = rc * 2;
+
+	memcpy(&flags, data, sizeof(flags));
conversion from bytestream to integer: I think in this case it would be better to use
Quoted 3 lines
flags = cpu_to_be64p((__u64*)data);
so that the flags always in hypervisor/big endian format

Quoted 8 lines

I don't think it's correct to byte swap the flags here. They must be in big endian format, but that's up to the caller.
The powernv secvar backend doesn't byte swap the flags, if the pseries one did then the final content of the variable, written either by phyp or OPAL, would differ depending on which backend is active.
Or am I missing something?

It seems wrong to not use the cpu_to_be64p() API to convert a byte stream to flags... That's why I suggested this.

Stefan

Quoted 2 lines

cheers

Andrew Donnellan

author

2023-02-14 06:08 UTC #37 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

In reply to Michael Ellerman

Quoted 5 lines · Michael Ellerman

Quoted 5 lines
Quoted 1 line
+ memcpy(&flags, data, sizeof(flags));
conversion from bytestream to integer: I think in this case it would be better to use

Quoted 7 lines

Quoted 3 lines
flags = cpu_to_be64p((__u64*)data);
so that the flags always in hypervisor/big endian format
I don't think it's correct to byte swap the flags here. They must be in
big endian format, but that's up to the caller.

That was what I initially thought, until I went and tested it properly and found it was indeed broken (at least in our qemu environment, this is slightly tricky for me to test right now on real hardware with real PowerVM) depending on kernel endianness.

- Userspace writes the flags into the buffer in BE order

- The first 8 bytes of the buffer are memcpy()ed, in BE order, into flags (a u64)

- plpar_hcall9() is called with flags as an argument, loaded into r9

- r9 is moved to r8 before jumping into the hypervisor

On a BE system, this works fine. On an LE system, this results in the bytes in the flags variable being loaded into the register in LE order, so the conversion is necessary.

Quoted 7 lines

The powernv secvar backend doesn't byte swap the flags, if the pseries
one did then the final content of the variable, written either by phyp
or OPAL, would differ depending on which backend is active.
Or am I missing something?

The powernv secvar backend doesn't have a notion of flags at all. (The flags interface for pseries is a little ugly, but it gets the job done
- userspace can read the format attribute to discover what it needs to do.)

Signature · 2 lines

Andrew Donnellan    OzLabs, ADL Canberra
[email protected]   IBM Australia Limited

Michael Ellerman

2023-02-15 11:47 UTC #38 lore raw

Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

Quoted 23 lines · Andrew Donnellan

Quoted 11 lines · Michael Ellerman
Quoted 8 lines
Quoted 1 line
+ memcpy(&flags, data, sizeof(flags));
conversion from bytestream to integer: I think in this case it would be better to use
flags = cpu_to_be64p((__u64*)data);
so that the flags always in hypervisor/big endian format
I don't think it's correct to byte swap the flags here. They must be in big endian format, but that's up to the caller.
That was what I initially thought, until I went and tested it properly and found it was indeed broken (at least in our qemu environment, this is slightly tricky for me to test right now on real hardware with real PowerVM) depending on kernel endianness.
- Userspace writes the flags into the buffer in BE order
- The first 8 bytes of the buffer are memcpy()ed, in BE order, into flags (a u64)

Quoted 7 lines

- plpar_hcall9() is called with flags as an argument, loaded into r9
- r9 is moved to r8 before jumping into the hypervisor
On a BE system, this works fine. On an LE system, this results in the bytes in the flags variable being loaded into the register in LE order, so the conversion is necessary.

Ah yep of course. So although the flags are written by userspace as part of the data as a stream of bytes, they're passed to the HV via a register.

I've had this patch in next for a few days and don't want to rebase it. So can you send a follow-up patch to fix the flags endianess, with a nice changelog and comment :)

cheers

Andrew Donnellan

author

2023-02-10 08:06 UTC #39 lore raw

PATCH v6 25/26 integrity/powerpc: Improve error handling & reporting when loading certs

From: Russell Currey <[email protected]>

A few improvements to load_powerpc.c:

- include integrity.h for the pr_fmt()
- move all error reporting out of get_cert_list()
- use ERR_PTR() to better preserve error detail
- don't use pr_err() for missing keys

Reviewed-by Mimi Zohar <[email protected]>

---

v3: New patch

---
 .../integrity/platform_certs/load_powerpc.c   | 26 ++++++++++++++-----
 1 file changed, 20 insertions(+), 6 deletions(-)

security/integrity/platform_certs/load_powerpc.c +20 -6

index 1e4f80a4e71c..dee51606d5f4 100644
@@ -14,9 +14,15 @@
 #include <asm/secure_boot.h>
 #include <asm/secvar.h>
 #include "keyring_handler.h"
+#include "../integrity.h"
 
 /*
  * Get a certificate list blob from the named secure variable.
+ *
+ * Returns:
+ *  - a pointer to a kmalloc'd buffer containing the cert list on success
+ *  - NULL if the key does not exist
+ *  - an ERR_PTR on error
  */
 static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
 {

@@ -25,19 +31,19 @@ static __init void *get_cert_list(u8 *key, unsigned long keylen, u64 *size)
 
 	rc = secvar_ops->get(key, keylen, NULL, size);
 	if (rc) {
-		pr_err("Couldn't get size: %d\n", rc);
-		return NULL;
+		if (rc == -ENOENT)
+			return NULL;
+		return ERR_PTR(rc);
 	}
 
 	db = kmalloc(*size, GFP_KERNEL);
 	if (!db)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	rc = secvar_ops->get(key, keylen, db, size);
 	if (rc) {
 		kfree(db);
-		pr_err("Error reading %s var: %d\n", key, rc);
-		return NULL;
+		return ERR_PTR(rc);
 	}
 
 	return db;

@@ -69,7 +75,11 @@ static int __init load_powerpc_certs(void)
 	 */
 	db = get_cert_list("db", 3, &dbsize);
 	if (!db) {
-		pr_err("Couldn't get db list from firmware\n");
+		pr_info("Couldn't get db list from firmware\n");
+	} else if (IS_ERR(db)) {
+		rc = PTR_ERR(db);
+		pr_err("Error reading db from firmware: %d\n", rc);
+		return rc;
 	} else {
 		rc = parse_efi_signature_list("powerpc:db", db, dbsize,
 					      get_handler_for_db);

@@ -81,6 +91,10 @@ static int __init load_powerpc_certs(void)
 	dbx = get_cert_list("dbx", 4,  &dbxsize);
 	if (!dbx) {
 		pr_info("Couldn't get dbx list from firmware\n");
+	} else if (IS_ERR(dbx)) {
+		rc = PTR_ERR(dbx);
+		pr_err("Error reading dbx from firmware: %d\n", rc);
+		return rc;
 	} else {
 		rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
 					      get_handler_for_dbx);

Signature · 1 line

2.39.1

Andrew Donnellan

author

2023-02-10 08:06 UTC #40 lore raw

PATCH v6 26/26 integrity/powerpc: Support loading keys from PLPKS

From: Russell Currey <[email protected]>

Add support for loading keys from the PLPKS on pseries machines, with the "ibm,plpks-sb-v1" format.

The object format is expected to be the same, so there shouldn't be any functional differences between objects retrieved on powernv or pseries.

Unlike on powernv, on pseries the format string isn't contained in the device tree. Use secvar_ops->format() to fetch the format string in a generic manner, rather than searching the device tree ourselves.

(The current code searches the device tree for a node compatible with "ibm,edk2-compat-v1". This patch switches to calling secvar_ops->format(), which in the case of OPAL/powernv means opal_secvar_format(), which searches the device tree for a node compatible with "ibm,secvar-backend" and checks its "format" property. These are equivalent, as skiboot creates a node with both "ibm,edk2-compat-v1" and "ibm,secvar-backend" as compatible strings.)

---

v3: New patch

v4: Pass format buffer size (stefanb, npiggin)

v5: Use sizeof(buf) rather than stating the size twice (npiggin)

Clarify change to DT compatible strings in commit message (zohar)

Reword commit message a bit (ajd)

---
 .../integrity/platform_certs/load_powerpc.c     | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

security/integrity/platform_certs/load_powerpc.c +10 -7

index dee51606d5f4..b9de70b90826 100644
@@ -10,7 +10,6 @@
 #include <linux/cred.h>
 #include <linux/err.h>
 #include <linux/slab.h>
-#include <linux/of.h>
 #include <asm/secure_boot.h>
 #include <asm/secvar.h>
 #include "keyring_handler.h"

@@ -59,16 +58,22 @@ static int __init load_powerpc_certs(void)
 	void *db = NULL, *dbx = NULL;
 	u64 dbsize = 0, dbxsize = 0;
 	int rc = 0;
-	struct device_node *node;
+	ssize_t len;
+	char buf[32];
 
 	if (!secvar_ops)
 		return -ENODEV;
 
-	/* The following only applies for the edk2-compat backend. */
-	node = of_find_compatible_node(NULL, NULL, "ibm,edk2-compat-v1");
-	if (!node)
+	len = secvar_ops->format(buf, sizeof(buf));
+	if (len <= 0)
 		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)) {
+		pr_err("Unsupported secvar implementation \"%s\", not loading certs\n", buf);
+		return -ENODEV;
+	}
+
 	/*
 	 * Get db, and dbx. They might not exist, so it isn't an error if we
 	 * can't get them.

@@ -103,8 +108,6 @@ static int __init load_powerpc_certs(void)
 		kfree(dbx);
 	}
 
-	of_node_put(node);
-
 	return rc;
 }
 late_initcall(load_powerpc_certs);

Signature · 1 line

2.39.1

Michael Ellerman

Applied

2023-02-15 12:43 UTC #41 lore raw