On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote:

quoted

+What:          /sys/firmware/secvar/config/supported_policies
+Date:          December 2022
+Contact:       Nayna Jain [off-list ref]
+Description:   RO file, only present if the secvar implementation
is PLPKS.
+
+               Contains a 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.
+
+What:          /sys/firmware/secvar/config/signed_update_algorithm
s
+Date:          December 2022
+Contact:       Nayna Jain [off-list ref]
+Description:   RO file, only present if the secvar implementation
is PLPKS.
+
+               Contains a bitmask of flags indicating which
algorithms the
+               hypervisor supports objects to be signed with when
modifying
+               secvars, represented as a 16 byte hexadecimal ASCII
string.
+               Consult the hypervisor documentation for what these
flags mean.

 
Can this at least say "as defined in PAPR version X section Y"?

We don't currently have a PAPR reference for this - that will come
eventually.

quoted

diff --git a/arch/powerpc/platforms/pseries/Kconfig

b/arch/powerpc/platforms/pseries/Kconfig
index a3b4d99567cb..94e08c405d50 100644

--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig

@@ -162,6 +162,19 @@ config PSERIES_PLPKS

 
          If unsure, select N.
 
+config PSERIES_PLPKS_SECVAR
+       depends on PSERIES_PLPKS
+       depends on PPC_SECURE_BOOT
+       bool "Support for the PLPKS secvar interface"
+       help
+         PowerVM can support dynamic secure boot with user-defined
keys
+         through the PLPKS. Keystore objects used in dynamic
secure boot
+         can be exposed to the kernel and userspace through the
powerpc
+         secvar infrastructure. Select this to enable the PLPKS
backend
+         for secvars for use in pseries dynamic secure boot.
+
+         If unsure, select N.

I don't think we need that config option at all, or if we do it
should
not be user selectable and just enabled automatically by
PSERIES_PLPKS.

quoted

diff --git a/arch/powerpc/platforms/pseries/Makefile

b/arch/powerpc/platforms/pseries/Makefile
index 92310202bdd7..807756991f9d 100644

--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile

@@ -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_PSERIES_PLPKS_SECVAR)     += plpks-secvar.o

I'm not convinced the secvar parts need to be in a separate C file.

If it was all in plpks.c we could avoid all/most of plpks.h and a
bunch
of accessors and so on.

But I don't feel that strongly about it if you think it's better
separate.

I feel pretty strongly that we should keep it separate. We're going to
need a header file anyway because in future patches to come shortly we
need to wire plpks up with the integrity subsystem to load keys into
kernel keyrings.

quoted

diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c

b/arch/powerpc/platforms/pseries/plpks-secvar.c
new file mode 100644
index 000000000000..8298f039bef4

--- /dev/null
+++ b/arch/powerpc/platforms/pseries/plpks-secvar.c

@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Secure variable implementation using the PowerVM LPAR Platform

KeyStore (PLPKS)
+ *
+ * Copyright 2022, 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 <asm/secvar.h>
+#include "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);les
+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);

For those last two I wonder if we should be decoding the integer
values
into a comma separated list of named flags?

Just blatting out the integer values is a bit gross. It's not helpful
for shell scripts, and a consumer written in C has to strtoull() the
value back into an integer before it can decode it.

How would you propose dealing with currently-reserved bits that might
be used by a future version of the hypervisor?

quoted

+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 u16 get_ucs2name(const char *name, uint8_t **ucs2_name)
+{
+       int namelen = strlen(name) * 2;
+       *ucs2_name = kzalloc(namelen, GFP_KERNEL);
+
+       if (!*ucs2_name)
+               return 0;
+
+       for (int i = 0; name[i]; i++) {
+               (*ucs2_name)[i * 2] = name[i];
+               (*ucs2_name)[i * 2 + 1] = '\0';
+       }
+
+       return namelen;
+}

There are some ucs2 routines in lib/ucs2_string.c, can we use any of
them?

ucs2_string.c doesn't do this.


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