Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic secure boot

Andrew Donnellan
2023-01-09T03:34:24Z

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 <nayna@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 <nayna@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
ajd@linux.ibm.com IBM Australia Limited