On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote:
> > +What: /sys/firmware/secvar/config/supported_policies
> > +Date: December 2022
> > +Contact: Nayna Jain <na...@linux.ibm.com>
> > +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 <na...@linux.ibm.com>
> > +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.
>
> > 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.
>
> > 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.
>
> > 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?
>
> > +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
a...@linux.ibm.com IBM Australia Limited
