Message-ID: <dcd26f6df2fc1c3ef01b39cc418ffd090b830aae.camel@russell.cc>
Date:   Fri, 06 Jan 2023 17:49:05 +1100
From:   Russell Currey <ruscur@...sell.cc>
To:     Andrew Donnellan <ajd@...ux.ibm.com>, linuxppc-dev@...ts.ozlabs.org
Cc:     gregkh@...uxfoundation.org, gcwilson@...ux.ibm.com,
        linux-kernel@...r.kernel.org, nayna@...ux.ibm.com,
        zohar@...ux.ibm.com, mpe@...erman.id.au
Subject: Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic
 secure boot

On Thu, 2023-01-05 at 19:15 +1100, Andrew Donnellan wrote:
> On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote:
> > 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.
> 
> The PAGE_SIZE limit, in practice, will be a major limitation with 4K
> pages (we expect several of the variables to regularly be larger than
> 4K) but won't be much of an issue for 64K (that's all the storage
> space
> the hypervisor will give you anyway).
> 
> In a previous internal version, we printed a message when PAGE_SIZE <
> plpks_get_maxobjectsize(), might be worth still doing that?

Yeah, we should do that in the secvar code.  The same limitation
applies on the powernv side as well.

> 
> > 
> > Co-developed-by: Nayna Jain <nayna@...ux.ibm.com>
> > Signed-off-by: Nayna Jain <nayna@...ux.ibm.com>
> > Co-developed-by: Andrew Donnellan <ajd@...ux.ibm.com>
> > Signed-off-by: Andrew Donnellan <ajd@...ux.ibm.com>
> > Signed-off-by: Russell Currey <ruscur@...sell.cc>
> 
> Some minor comments for v3 on a patch which already carries my
> signoff...
> 
> > ---
> > 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.
> > 
> >  Documentation/ABI/testing/sysfs-secvar        |  67 ++++-
> >  arch/powerpc/platforms/pseries/Kconfig        |  13 +
> >  arch/powerpc/platforms/pseries/Makefile       |   4 +-
> >  arch/powerpc/platforms/pseries/plpks-secvar.c | 245
> > ++++++++++++++++++
> >  4 files changed, 326 insertions(+), 3 deletions(-)
> >  create mode 100644 arch/powerpc/platforms/pseries/plpks-secvar.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-secvar
> > b/Documentation/ABI/testing/sysfs-secvar
> > index feebb8c57294..466a8cb92b92 100644
> > --- a/Documentation/ABI/testing/sysfs-secvar
> > +++ b/Documentation/ABI/testing/sysfs-secvar
> > @@ -34,7 +34,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<nayna@...ux.ibm.com>
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> >  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 +44,68 @@ Contact:     Nayna Jain <nayna@...ux.ibm.com>
> >  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:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> > +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:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> > +Description:   RO file, only present if the secvar implementation
> > is
> > PLPKS.
> > +
> > +               Contains the config version as reported by the
> > hypervisor in
> > +               ASCII decimal format.
> > +
> > +What:          /sys/firmware/secvar/config/max_object_size
> > +Date:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> > +Description:   RO file, only present if the secvar implementation
> > is
> > PLPKS.
> > +
> > +               Contains the 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.
> > +
> > +What:          /sys/firmware/secvar/config/total_size
> > +Date:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> > +Description:   RO file, only present if the secvar implementation
> > is
> > PLPKS.
> > +
> > +               Contains the total size of the PLPKS in bytes,
> > represented in
> > +               ASCII decimal format.
> > +
> > +What:          /sys/firmware/secvar/config/used_space
> > +Date:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.ibm.com>
> > +Description:   RO file, only present if the secvar implementation
> > is
> > PLPKS.
> > +
> > +               Contains the current space consumed of the PLPKS in
> > bytes,
> > +               represented in ASCII decimal format.
> 
> Note that presently, this isn't actually updated when the user writes
> objects. I suppose we could fix this.

We should definitely fix that.

> 
> > +
> > +What:          /sys/firmware/secvar/config/supported_policies
> > +Date:          December 2022
> > +Contact:       Nayna Jain <nayna@...ux.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@...ux.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.
> > 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
> 
> PSERIES_PLPKS has no use on its own without the secvar frontend. We
> should either just have one option, or for future-proofing purposes
> turn this depends into a select and get PSERIES_PLPKS out of
> menuconfig.

I believe I did this to enforce the dependency on PPC_SECURE_BOOT.  As
of right now they're tied together, but if I add that dependency to
PSERIES_PLPKS, then it makes things awkward if there's a more generic
PLPKS interface in future that isn't specific to secure boot.  I
suppose things can be compartmentalised in future if that happens.

> 
> > +       depends on PPC_SECURE_BOOT
> 
> FWIW, starting from a pseries_le_defconfig, turning on all the
> options
> that are required to get to PPC_SECURE_BOOT was annoying. I'd like to
> have PSERIES_PLPKS_SECVAR enabled in the defconfig but it involves
> switching on quite a lot.

Probably more of an mpe question, but we have to turn on a bunch of IMA
stuff that adds complexity and increases the build time, so I'm not
sure if we'd want it in pseries_defconfig.  We could definitely either
add things to security.config or make a new config fragment for it
though.

> 
> > +       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
> 
> We should also expand PLPKS to PowerVM LPAR Platform KeyStore.

True.

> 
> > +         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.
> > +
> >  config PAPR_SCM
> >         depends on PPC_PSERIES && MEMORY_HOTPLUG && LIBNVDIMM
> >         tristate "Support for the PAPR Storage Class Memory
> > interface"
> > 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
> >  obj-$(CONFIG_SUSPEND)          += suspend.o
> >  obj-$(CONFIG_PPC_VAS)          += vas.o vas-sysfs.o
> >  
> > 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
> 
> And by the time this gets merged, 2023

Optimistic!  I'll update that in v3.

> 
> > + * 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);
> > +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 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;
> > +}
> > +
> > +static u32 get_policy(const char *name)
> > +{
> > +       if ((strcmp(name, "db") == 0) ||
> > +           (strcmp(name, "dbx") == 0) ||
> > +           (strcmp(name, "grubdb") == 0) ||
> > +           (strcmp(name, "sbat") == 0))
> > +               return (WORLDREADABLE | SIGNEDUPDATE);
> > +       else
> > +               return SIGNEDUPDATE;
> > +}
> > +
> > +#define PLPKS_SECVAR_COUNT 8
> > +static char *var_names[PLPKS_SECVAR_COUNT] = {
> > +       "PK",
> > +       "KEK",
> > +       "db",
> > +       "dbx",
> > +       "grubdb",
> > +       "sbat",
> > +       "moduledb",
> > +       "trustedcadb",
> > +};
> > +
> > +static int plpks_get_variable(const char *key, uint64_t key_len,
> > +                             u8 *data, uint64_t *data_size)
> > +{
> > +       struct plpks_var var = {0};
> > +       u16 ucs2_namelen;
> > +       u8 *ucs2_name;
> > +       int rc = 0;
> > +
> > +       ucs2_namelen = get_ucs2name(key, &ucs2_name);
> > +       if (!ucs2_namelen)
> > +               return -ENOMEM;
> > +
> > +       var.name = ucs2_name;
> > +       var.namelen = ucs2_namelen;
> > +       var.os = PLPKS_VAR_LINUX;
> > +       rc = plpks_read_os_var(&var);
> > +
> > +       if (rc)
> > +               goto err;
> > +
> > +       *data_size = var.datalen + sizeof(var.policy);
> > +
> > +       // We can be called with data = NULL to just get the object
> > size.
> > +       if (data) {
> > +               memcpy(data, &var.policy, sizeof(var.policy));
> > +               memcpy(data + sizeof(var.policy), var.data,
> > var.datalen);
> > +       }
> > +
> > +       kfree(var.data);
> > +err:
> > +       kfree(ucs2_name);
> > +       return rc;
> > +}
> > +
> > +static int plpks_set_variable(const char *key, uint64_t key_len,
> > +                             u8 *data, uint64_t data_size)
> > +{
> > +       struct plpks_var var = {0};
> > +       u16 ucs2_namelen;
> > +       u8 *ucs2_name;
> > +       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;
> > +
> > +       ucs2_namelen = get_ucs2name(key, &ucs2_name);
> > +       if (!ucs2_namelen)
> > +               return -ENOMEM;
> > +
> > +       memcpy(&flags, data, sizeof(flags));
> > +
> > +       var.datalen = data_size - sizeof(flags);
> > +       var.data = kzalloc(var.datalen, GFP_KERNEL);
> > +       if (!var.data) {
> > +               rc = -ENOMEM;
> > +               goto err;
> > +       }
> > +
> > +       memcpy(var.data, data + sizeof(flags), var.datalen);
> > +
> > +       var.name = ucs2_name;
> > +       var.namelen = ucs2_namelen;
> > +       var.os = PLPKS_VAR_LINUX;
> > +       var.policy = get_policy(key);
> > +
> > +       rc = plpks_signed_update_var(var, flags);
> > +
> > +       kfree(var.data);
> > +err:
> > +       kfree(ucs2_name);
> > +       return rc;
> > +}
> > +
> > +/*
> > + * get_next() in the secvar API is designed for the OPAL API.
> > + * If *key is 0, it returns the first variable in the keystore.
> > + * Otherwise, you pass the name of a key and it returns next in
> > line.
> > + *
> > + * We're going to cheat here - since we have fixed keys and don't
> > care about
> > + * key_len, we can just use it as an index.
> 
> This is kinda gross.

It's gross, but it's really simple and means we don't either have to
have two ops for the same thing (getting all the variables) or rework
powernv to better fit pseries.  Open to suggestions on other ways to do
it, this is just the best I came up with.

> 
> > + */
> 
> Inconsistent comment style

True, I'm using // for multi-line comments in other places.  I think my
brain decided that this one was too long for that, but I'll make the
other multi-line comments similarly old-fashioned for consistency.

> 
> > +static int plpks_get_next_variable(const char *key, uint64_t
> > *key_len, uint64_t keybufsize)
> > +{
> > +       if (!key || !key_len)
> > +               return -EINVAL;
> > +
> > +       if (*key_len >= PLPKS_SECVAR_COUNT)
> > +               return -ENOENT;
> > +
> > +       if (strscpy((char *)key, var_names[(*key_len)++],
> > keybufsize)
> > < 0)
> 
> Not sure how I feel about the increment being buried in here

Yeah this is definitely more clever than clear, I'll separate the
increment.

Cheers for the review (and for your contributions obviously)

> 
> > +               return -E2BIG;
> > +
> > +       return 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.
> > +static ssize_t plpks_secvar_format(char *buf)
> > +{
> > +       struct plpks_var var = {0};
> > +       ssize_t ret;
> > +
> > +       var.component = NULL;
> > +       // Only the signed variables have ucs2-encoded names, this
> > one doesn't
> > +       var.name = "SB_VERSION";
> > +       var.namelen = 10;
> > +       var.datalen = 0;
> > +       var.data = NULL;
> > +
> > +       // 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)
> > +                       return sysfs_emit(buf, "ibm,plpks-sb-
> > unknown\n");
> > +
> > +               pr_err("Error %ld reading SB_VERSION from
> > firmware\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       // Hypervisor defines SB_VERSION as a "1 byte unsigned
> > integer value"
> > +       ret = sysfs_emit(buf, "ibm,plpks-sb-%hhu\n", var.data[0]);
> > +
> > +       kfree(var.data);
> > +       return ret;
> > +}
> > +
> > +static int plpks_max_size(uint64_t *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 = (uint64_t)plpks_get_maxobjectsize() + 8;
> > +
> > +       return 0;
> > +}
> > +
> > +
> > +static const struct secvar_operations plpks_secvar_ops = {
> > +       .get = plpks_get_variable,
> > +       .get_next = plpks_get_next_variable,
> > +       .set = plpks_set_variable,
> > +       .format = plpks_secvar_format,
> > +       .max_size = plpks_max_size,
> > +};
> > +
> > +static int plpks_secvar_init(void)
> > +{
> > +       if (!plpks_is_available())
> > +               return -ENODEV;
> > +
> > +       set_secvar_ops(&plpks_secvar_ops);
> > +       set_secvar_config_attrs(config_attrs);
> > +       return 0;
> > +}
> > +device_initcall(plpks_secvar_init);
> 

Powered by blists - more mailing lists