Re: [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic secure boot
From: Russell Currey
Date: Sun Jan 08 2023 - 22:34:40 EST
On Fri, 2023-01-06 at 21:49 +1100, Michael Ellerman wrote:
> Russell Currey <ruscur@xxxxxxxxxx> writes:
> > 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.
> >
> > Co-developed-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > Co-developed-by: Andrew Donnellan <ajd@xxxxxxxxxxxxx>
> > Signed-off-by: Andrew Donnellan <ajd@xxxxxxxxxxxxx>
> > Signed-off-by: Russell Currey <ruscur@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
> > +Contact: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> > +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.
>
> I think it's OK to mention that currently this only exists for PLPKS
> ...
>
> > +What: /sys/firmware/secvar/config/version
> > +Date: December 2022
> > +Contact: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > +Description: RO file, only present if the secvar implementation
> > is PLPKS.
>
> ... but I don't think we want to specify that files are only present
> for PLPKS.
>
> Because if another backend wanted to create them in future, that
> would
> technically be an ABI change.
Some are going to be PLPKS-specific, but for generic stuff like this I
can change the description.
>
> > + 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@xxxxxxxxxxxxx>
> > +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@xxxxxxxxxxxxx>
> > +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.
>
> Similarly here I think the description should be written in a way
> that
> is agnostic about the backend. So eg. "Contains the total size of the
> key store in bytes".
>
>
> > +What: /sys/firmware/secvar/config/used_space
> > +Date: December 2022
> > +Contact: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > +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.
> > +
> > +What: /sys/firmware/secvar/config/supported_policies
> > +Date: December 2022
> > +Contact: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > +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@xxxxxxxxxxxxx>
> > +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"?
I don't think there is currently a published PAPR version with this
stuff in it, but once there is we should update the docs to reference
it.
>
> > 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.
This code needs secvar (which is why PPC_SECURE_BOOT is there). We
could add a PPC_SECURE_BOOT dependency to PSERIES_PLPKS, but that's not
necessary to just use the keystore, i.e. what [0] is doing. While
there's no other PLPKS consumers upstream right now, there will be [0],
so that's why I added a new config option for the secure boot case.
I can make it selected automatically if you have both PSERIES_PLPKS and
PPC_SECURE_BOOT. Not exactly sure what convention is when it comes to
nested dependencies.
[0]:
https://lore.kernel.org/linuxppc-dev/20221130202358.18034-3-gjoyce@xxxxxxxxxxxxxxxxxx/
>
> > 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 think it makes sense for the reasons above - PPC_SECURE_BOOT (which
is needed for secvar) has a ton of dependencies and any other consumers
of the keystore outside of secure boot would have to build in a lot
more stuff than they need.
Some of plpks.h is going to need to move into include/ to solve some
kexec issues I found too, so I don't think we can hope to entirely get
rid of it.
>
> > 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);
> > +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.
We can do that. We should still blat the integer value in case the
hypervisor adds something the kernel doesn't know about yet, though.
>
> > +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?
We didn't think so. There's routines for dealing with ucs2 strings,
but we don't ever actually do that - all we do is pad C strings and
double the length. There's ucs2_to_utf8() there, but not the opposite.
We could drop this function and instead hardcode "P\0K\0", "K\0E\0K\0"
etc but that seemed like a lot of duplication.
>
> > +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
>
> I don't think we need that. Just declare the array as unsized and
> then
> use ARRAY_SIZE(var_names) in plpks_get_next_variable().
True, that's better.
>
> > +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);
> > + }
>
> There's a lot of allocation and copying going on. The secvar-sysfs.c
> data_read() has kzalloc'ed data, but only after already doing the
> hcall
> to get the size. Then plpks_read_os_var() does an allocation for the
> hcall and then another allocation of the exact data size. Then
> data_read()
> does another copy into the sysfs supplied buffer.
>
> So to read a single variable we do the hcall twice, and allocate/copy
> the content of the variable 4 times?
We don't need to do the hcall twice for PLPKS. I can add a flag to
secvar_ops to skip object size discovery and just allocate max_size()
in data_read() instead. I can't see a reason why OPAL can't just do
that too, but I don't know the details and I don't want to break it.
We would also have to change get_cert_list() in load_powerpc.c, which
does the same thing.
>
> - Hypervisor into "output" in plpks_read_var().
> - "output" into "var->data" in plpks_read_var().
> - "var.data" into "data" in plpks_get_variable().
> - "data" into "buf" in data_read().
>
> As long as maxobjsize is < PAGE_SIZE I think we could do the hcall
> directly into "buf". Maybe we want to avoid writing into "buf"
> directly
> in case the hcall fails or something, but the other 3 copies seem
> unnecessary.
The plpks.c code is pretty heavily abstracted, though maybe we could do
something like not allocate a new buffer if we call plpks_read_var()
and var.data != NULL. So if plpks_get_variable() calls
plpks_read_var() where var.data = data + 4 (gotta make room for the
policy), I think those changes combine to go from 2 hcalls to 1, and 4
copies to 2.
That said, while we should make it faster, this isn't a particularly
hot code path.
>
> > + 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.
> > + */
>
> That's kinda gross. Just change the ops API to do what we need?
> Either
> add a separate get-by-index routine or change the existing one and
> update the only other implementation.
I tried the latter and it was substantially more complex, I'll add a
new op and secvar can use whichever it's provided with.
>
> > +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)
> > + 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";
>
> Is that specified somewhere?
Not publicly, at least not yet. PAPR will document everything about
the hcalls but I'm not sure if it will document specific pre-created
objects used for dynamic secure boot.
>
> > + 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;
>
> I'm not sure you should pass that raw error back to sysfs. Some of
> the
> values could be confusing, eg. if you return -EINVAL it looks like a
> parameter to the read() syscall was invalid. Might be better to just
> return -EIO.
OK, is it sane to print a different error code than the one we return?
I assume it's fine in this context, just wouldn't want to lose
information.
>
> > + }
> > +
> > + // Hypervisor defines SB_VERSION as a "1 byte unsigned
> > integer value"
> > + ret = sysfs_emit(buf, "ibm,plpks-sb-%hhu\n", var.data[0]);
>
> The rest of the name string is just made up by us?
The format string is entirely made up by me.
OPAL secvar has a "real" format string (i.e. it's provided by the
device tree). We have a format string in sysfs which is ABI, and
security/integrity/platform_certs/load_powerpc.c looks for it (by
looking in the device tree rather than using secvar_ops->format(), I
need to fix that too). I figured pseries should use it too, and that
it should be different from OPAL. There is nothing specified on the
hypervisor end that we could use, all we have is the magic SB_VERSION
value.
I figured it was a reasonable way to do things. Open to other ideas
(including better names).
>
> > + 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);
>
> That must be a machine_device_initcall(pseries, ...), otherwise we
> will
> blow up doing a hcall on powernv in plpks_is_available().
OK, can do. I don't understand your case of how powernv could hit
this, but I think I to have to move plpks_is_available() into include/,
so it's going to be even more possible anyway.
>
> cheers