| Message ID | 20230120074306.1326298-23-ajd@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pSeries dynamic secure boot secvar interface + platform keyring loading | expand |
On Fri Jan 20, 2023 at 5:43 PM AEST, Andrew Donnellan wrote: > From: Russell Currey <ruscur@russell.cc> > > 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. > (Even when using bin_attributes, which can be larger than a single page, > sysfs only gives us one page's worth of write buffer at a time, and the > hypervisor does not expose an interface for partial writes.) > > Co-developed-by: Nayna Jain <nayna@linux.ibm.com> > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > Signed-off-by: Russell Currey <ruscur@russell.cc> > > --- > > 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. > > v3: plpks_set_variable(): pass var to plpks_signed_update_var() as a > pointer (mpe) > > Update copyright date (ajd) > > Consistent comment style (ajd) > > Change device_initcall() to machine_arch_initcall(pseries...) so we > don't try to load on powernv and kill the machine (mpe) > > Add config attributes into plpks_secvar_ops (mpe) > > Get rid of PLPKS_SECVAR_COUNT macro (mpe) > > Reworded descriptions in ABI documentation (mpe) > > Switch to using secvar_ops->var_names rather than > secvar_ops->get_next() (ajd/mpe) > > Optimise allocation/copying of buffers (mpe) > > Elaborate the comment documenting the "format" string (mpe) > > Return -EIO on errors in the read case (mpe) > > Add "grubdbx" variable (Sudhakar Kuppusamy) > > Use utf8s_to_utf16s() rather than our own "UCS-2" conversion code (mpe) > > Change uint64_t to u64 (mpe) > > Fix SB_VERSION data length (ruscur) > > Stop prepending policy data on read (ruscur) > > Enforce max format length on format string (not strictly needed, but > makes the length limit clear) (ajd) > > Update include of plpks.h to reflect new path (ruscur) > > Consistent constant naming scheme (ruscur) > > v4: Return set_secvar_ops() return code > > Pass buffer size to plpks_secvar_format() (stefanb, npiggin) > > Add missing null check (stefanb) > > Add comment to commit message explaining PAGE_SIZE write limit (joel) > --- > Documentation/ABI/testing/sysfs-secvar | 75 +++++- > arch/powerpc/platforms/pseries/Makefile | 4 +- > arch/powerpc/platforms/pseries/plpks-secvar.c | 215 ++++++++++++++++++ > 3 files changed, 291 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..a19f4d5fcec6 100644 > --- a/Documentation/ABI/testing/sysfs-secvar > +++ b/Documentation/ABI/testing/sysfs-secvar > @@ -18,6 +18,14 @@ Description: A string indicating which backend is in use by the firmware. > This determines the format of the variable and the accepted > format of variable updates. > > + On powernv/OPAL, this value is provided by the OPAL firmware > + and is expected to be "ibm,edk2-compat-v1". > + > + On pseries/PLPKS, this is generated by the kernel based on the > + version number in the SB_VERSION variable in the keystore, and > + has the form "ibm,plpks-sb-v<version>", or > + "ibm,plpks-sb-unknown" if there is no SB_VERSION variable. > + > What: /sys/firmware/secvar/vars/<variable name> > Date: August 2019 > Contact: Nayna Jain <nayna@linux.ibm.com> > @@ -34,7 +42,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@linux.ibm.com> > +Contact: Nayna Jain <nayna@linux.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 +52,68 @@ Contact: Nayna Jain <nayna@linux.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@linux.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@linux.ibm.com> > +Description: Config version as reported by the hypervisor in ASCII decimal > + format. > + > + Currently only provided by PLPKS on the pseries platform. > + > +What: /sys/firmware/secvar/config/max_object_size > +Date: December 2022 > +Contact: Nayna Jain <nayna@linux.ibm.com> > +Description: 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. > + > + Currently only provided by PLPKS on the pseries platform. > + > +What: /sys/firmware/secvar/config/total_size > +Date: December 2022 > +Contact: Nayna Jain <nayna@linux.ibm.com> > +Description: Total size of the PLPKS in bytes, represented in ASCII decimal > + format. > + > + Currently only provided by PLPKS on the pseries platform. > + > +What: /sys/firmware/secvar/config/used_space > +Date: December 2022 > +Contact: Nayna Jain <nayna@linux.ibm.com> > +Description: Current space consumed by the key store, in bytes, represented > + in ASCII decimal format. > + > + Currently only provided by PLPKS on the pseries platform. > + > +What: /sys/firmware/secvar/config/supported_policies > +Date: December 2022 > +Contact: Nayna Jain <nayna@linux.ibm.com> > +Description: 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. > + > + Currently only provided by PLPKS on the pseries platform. > + > +What: /sys/firmware/secvar/config/signed_update_algorithms > +Date: December 2022 > +Contact: Nayna Jain <nayna@linux.ibm.com> > +Description: Bitmask of flags indicating which algorithms the hypervisor > + supports for signed update of objects, represented as a 16 byte > + hexadecimal ASCII string. Consult the hypervisor documentation > + for what these flags mean. > + > + Currently only provided by PLPKS on the pseries platform. > diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile > index 92310202bdd7..d52b7ec1a678 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_PPC_SECVAR_SYSFS) += 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..a80d9f9469f9 > --- /dev/null > +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c > @@ -0,0 +1,215 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +// Secure variable implementation using the PowerVM LPAR Platform KeyStore (PLPKS) > +// > +// Copyright 2022, 2023 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 <linux/nls.h> > +#include <asm/machdep.h> > +#include <asm/secvar.h> > +#include <asm/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 u32 get_policy(const char *name) > +{ > + if ((strcmp(name, "db") == 0) || > + (strcmp(name, "dbx") == 0) || > + (strcmp(name, "grubdb") == 0) || > + (strcmp(name, "grubdbx") == 0) || > + (strcmp(name, "sbat") == 0)) > + return (PLPKS_WORLDREADABLE | PLPKS_SIGNEDUPDATE); > + else > + return PLPKS_SIGNEDUPDATE; > +} > + > +static const char * const plpks_var_names[] = { > + "PK", > + "KEK", > + "db", > + "dbx", > + "grubdb", > + "grubdbx", > + "sbat", > + "moduledb", > + "trustedcadb", > + NULL, > +}; Var and key are used somewhat interchangeably? These are keys, I think? And plpks could have other vars but we're only interested in (at least a subset of) keys here if I understood right. I guess the terminology is like that throughout secvar so maybe nothing to be done. > + > +static int plpks_get_variable(const char *key, u64 key_len, u8 *data, > + u64 *data_size) > +{ > + struct plpks_var var = {0}; > + int rc = 0; > + > + var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL); > + if (!var.name) > + return -ENOMEM; > + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name, > + key_len - 1); > + if (rc < 0) > + goto err; Okay I can't work out why it's key_len - 1 rather than key_len. > + var.namelen = rc * 2; > + > + var.os = PLPKS_VAR_LINUX; > + if (data) { > + var.data = data; > + var.datalen = *data_size; > + } > + rc = plpks_read_os_var(&var); > + > + if (rc) > + goto err; > + > + *data_size = var.datalen; > + > +err: > + kfree(var.name); > + if (rc && rc != -ENOENT) { > + pr_err("Failed to read variable '%s': %d\n", key, rc); > + // Return -EIO since userspace probably doesn't care about the > + // specific error > + rc = -EIO; > + } > + return rc; > +} > + > +static int plpks_set_variable(const char *key, u64 key_len, u8 *data, > + u64 data_size) > +{ > + struct plpks_var var = {0}; > + 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)) So it's unstructured 8 byte of flags, not a u64 integer? Why not u8 flags[8] then? > + return -EINVAL; > + > + var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL); > + if (!var.name) > + return -ENOMEM; > + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name, > + key_len - 1); > + if (rc < 0) > + goto err; > + var.namelen = rc * 2; > + > + memcpy(&flags, data, sizeof(flags)); > + > + var.datalen = data_size - sizeof(flags); > + var.data = data + sizeof(flags); > + var.os = PLPKS_VAR_LINUX; > + var.policy = get_policy(key); > + > + // Unlike in the read case, the plpks error code can be useful to > + // userspace on write, so we return it rather than just -EIO > + rc = plpks_signed_update_var(&var, flags); > + > +err: > + kfree(var.name); > + return rc; > +} > + > +// 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, size_t bufsize) > +{ > + struct plpks_var var = {0}; > + ssize_t ret; > + > + var.component = NULL; > + // Only the signed variables have null bytes in their names, this one doesn't > + var.name = "SB_VERSION"; > + var.namelen = 10; Could you make that strlen(var.name) for the benefit of those of us with missing fingers? > + var.datalen = 1; > + var.data = kzalloc(1, GFP_KERNEL); This could just point to a u8 on stack I think? > + if (!var.data) > + return -ENOMEM; > + > + // 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) { > + ret = snprintf(buf, bufsize, "ibm,plpks-sb-unknown"); > + } else { > + pr_err("Error %ld reading SB_VERSION from firmware\n", ret); > + ret = -EIO; > + } Is there a meaningful distinction? Does anything good come of advertising an unknown format like this? > + goto err; > + } > + > + // This string is made up by us - the hypervisor doesn't provide us > + // with a format string in the way that OPAL firmware does. Hypervisor > + // defines SB_VERSION as a "1 byte unsigned integer value". I'd put the comment about SB_VERSION at the top where you use/define it or mention it in the comment. > + ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", var.data[0]); > + > +err: > + kfree(var.data); > + return ret; > +} > + > +static int plpks_max_size(u64 *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 = (u64)plpks_get_maxobjectsize() + 8; You have this 8 open coded twice (once as sizeof(u64)). You could make it a #define at the top with a brief overview of the hcall format so you don't need so much commentage for it. Although a note here that the objsize does not include the flags bytes is good to keep. Thanks, Nick > + > + return 0; > +} > + > + > +static const struct secvar_operations plpks_secvar_ops = { > + .get = plpks_get_variable, > + .set = plpks_set_variable, > + .format = plpks_secvar_format, > + .max_size = plpks_max_size, > + .config_attrs = config_attrs, > + .var_names = plpks_var_names, > +}; > + > +static int plpks_secvar_init(void) > +{ > + if (!plpks_is_available()) > + return -ENODEV; > + > + return set_secvar_ops(&plpks_secvar_ops); > +} > +machine_device_initcall(pseries, plpks_secvar_init); > -- > 2.39.0
On Tue, 2023-01-24 at 15:17 +1000, Nicholas Piggin wrote: > > +static const char * const plpks_var_names[] = { > > + "PK", > > + "KEK", > > + "db", > > + "dbx", > > + "grubdb", > > + "grubdbx", > > + "sbat", > > + "moduledb", > > + "trustedcadb", > > + NULL, > > +}; > > Var and key are used somewhat interchangeably? These are keys, I > think? > And plpks could have other vars but we're only interested in (at > least a > subset of) keys here if I understood right. > > I guess the terminology is like that throughout secvar so maybe > nothing > to be done. The "key" terminology seems to come from OPAL, while on the PLPKS side it's a bit of a mess but "var" follows the usage in existing code (the spec refers more to "objects"). > > > + > > +static int plpks_get_variable(const char *key, u64 key_len, u8 > > *data, > > + u64 *data_size) > > +{ > > + struct plpks_var var = {0}; > > + int rc = 0; > > + > > + var.name = kcalloc(key_len - 1, sizeof(wchar_t), > > GFP_KERNEL); > > + if (!var.name) > > + return -ENOMEM; > > + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, > > (wchar_t *)var.name, > > + key_len - 1); > > + if (rc < 0) > > + goto err; > > Okay I can't work out why it's key_len - 1 rather than key_len. The existing code in secvar-sysfs.c calls secvar_ops->get() with key_len = strlen(name) + 1, to include the null byte, which is what OPAL expects. For PLPKS, the variable name explicitly does not include a trailing null byte. I'll add a comment indicating as such, perhaps at some later point it can be reworked. > > > + var.namelen = rc * 2; > > + > > + var.os = PLPKS_VAR_LINUX; > > + if (data) { > > + var.data = data; > > + var.datalen = *data_size; > > + } > > + rc = plpks_read_os_var(&var); > > + > > + if (rc) > > + goto err; > > + > > + *data_size = var.datalen; > > + > > +err: > > + kfree(var.name); > > + if (rc && rc != -ENOENT) { > > + pr_err("Failed to read variable '%s': %d\n", key, > > rc); > > + // Return -EIO since userspace probably doesn't > > care about the > > + // specific error > > + rc = -EIO; > > + } > > + return rc; > > +} > > + > > +static int plpks_set_variable(const char *key, u64 key_len, u8 > > *data, > > + u64 data_size) > > +{ > > + struct plpks_var var = {0}; > > + 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)) > > So it's unstructured 8 byte of flags, not a u64 integer? Why not u8 > flags[8] then? No, it's a u64 and it's passed in the hcall as a single u64. > > > + return -EINVAL; > > + > > + var.name = kcalloc(key_len - 1, sizeof(wchar_t), > > GFP_KERNEL); > > + if (!var.name) > > + return -ENOMEM; > > + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, > > (wchar_t *)var.name, > > + key_len - 1); > > + if (rc < 0) > > + goto err; > > + var.namelen = rc * 2; > > + > > + memcpy(&flags, data, sizeof(flags)); > > + > > + var.datalen = data_size - sizeof(flags); > > + var.data = data + sizeof(flags); > > + var.os = PLPKS_VAR_LINUX; > > + var.policy = get_policy(key); > > + > > + // Unlike in the read case, the plpks error code can be > > useful to > > + // userspace on write, so we return it rather than just - > > EIO > > + rc = plpks_signed_update_var(&var, flags); > > + > > +err: > > + kfree(var.name); > > + return rc; > > +} > > + > > +// 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, size_t bufsize) > > +{ > > + struct plpks_var var = {0}; > > + ssize_t ret; > > + > > + var.component = NULL; > > + // Only the signed variables have null bytes in their > > names, this one doesn't > > + var.name = "SB_VERSION"; > > + var.namelen = 10; > > Could you make that strlen(var.name) for the benefit of those of us > with > missing fingers? Will do. > > > + var.datalen = 1; > > + var.data = kzalloc(1, GFP_KERNEL); > > This could just point to a u8 on stack I think? Until we get VMAP_STACK and we'll have to switch back. > > > > + if (!var.data) > > + return -ENOMEM; > > + > > + // 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) { > > + ret = snprintf(buf, bufsize, "ibm,plpks-sb- > > unknown"); > > + } else { > > + pr_err("Error %ld reading SB_VERSION from > > firmware\n", ret); > > + ret = -EIO; > > + } > > Is there a meaningful distinction? Does anything good come of > advertising an unknown format like this? Our thinking was simply to distinguish between cases where the API is otherwise working happily but for some reason simply not advertising a version number (you might still want to try to interact with the key store regardless) vs the case where the hypervisor is returning a real error. I plan to keep this as is for the next revision, but I'm happy to change it if there's a strong objection, it could go either way. > > > + goto err; > > + } > > + > > + // This string is made up by us - the hypervisor doesn't > > provide us > > + // with a format string in the way that OPAL firmware does. > > Hypervisor > > + // defines SB_VERSION as a "1 byte unsigned integer value". > > I'd put the comment about SB_VERSION at the top where you use/define > it > or mention it in the comment. Will fix. > > > + ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", > > var.data[0]); > > + > > +err: > > + kfree(var.data); > > + return ret; > > +} > > + > > +static int plpks_max_size(u64 *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 = (u64)plpks_get_maxobjectsize() + 8; > > You have this 8 open coded twice (once as sizeof(u64)). You could > make > it a #define at the top with a brief overview of the hcall format so > you > don't need so much commentage for it. Although a note here that the > objsize does not include the flags bytes is good to keep. Will do.
On Tue, 2023-01-31 at 13:54 +1100, Andrew Donnellan wrote: > > > +{ > > > + // 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 = (u64)plpks_get_maxobjectsize() + 8; > > > > You have this 8 open coded twice (once as sizeof(u64)). You could > > make > > it a #define at the top with a brief overview of the hcall format > > so > > you > > don't need so much commentage for it. Although a note here that the > > objsize does not include the flags bytes is good to keep. > > Will do. Thinking about this further, I'm going to change the 8 to sizeof(u64) to make it clearer that it's linked with the type of the flags variable, but I am going to keep the other cases as is (they're sizeof(flags), not sizeof(u64), so it's already obvious how they work, and I don't want to define a macro for one user).
On Tue Jan 31, 2023 at 12:54 PM AEST, Andrew Donnellan wrote: > On Tue, 2023-01-24 at 15:17 +1000, Nicholas Piggin wrote: > > > +static const char * const plpks_var_names[] = { > > > + "PK", > > > + "KEK", > > > + "db", > > > + "dbx", > > > + "grubdb", > > > + "grubdbx", > > > + "sbat", > > > + "moduledb", > > > + "trustedcadb", > > > + NULL, > > > +}; > > > > Var and key are used somewhat interchangeably? These are keys, I > > think? > > And plpks could have other vars but we're only interested in (at > > least a > > subset of) keys here if I understood right. > > > > I guess the terminology is like that throughout secvar so maybe > > nothing > > to be done. > > The "key" terminology seems to come from OPAL, while on the PLPKS side > it's a bit of a mess but "var" follows the usage in existing code (the > spec refers more to "objects"). > > > > > > + > > > +static int plpks_get_variable(const char *key, u64 key_len, u8 > > > *data, > > > + u64 *data_size) > > > +{ > > > + struct plpks_var var = {0}; > > > + int rc = 0; > > > + > > > + var.name = kcalloc(key_len - 1, sizeof(wchar_t), > > > GFP_KERNEL); > > > + if (!var.name) > > > + return -ENOMEM; > > > + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, > > > (wchar_t *)var.name, > > > + key_len - 1); > > > + if (rc < 0) > > > + goto err; > > > > Okay I can't work out why it's key_len - 1 rather than key_len. > > The existing code in secvar-sysfs.c calls secvar_ops->get() with > key_len = strlen(name) + 1, to include the null byte, which is what > OPAL expects. For PLPKS, the variable name explicitly does not include > a trailing null byte. > > I'll add a comment indicating as such, perhaps at some later point it > can be reworked. > > > > > > + var.namelen = rc * 2; > > > + > > > + var.os = PLPKS_VAR_LINUX; > > > + if (data) { > > > + var.data = data; > > > + var.datalen = *data_size; > > > + } > > > + rc = plpks_read_os_var(&var); > > > + > > > + if (rc) > > > + goto err; > > > + > > > + *data_size = var.datalen; > > > + > > > +err: > > > + kfree(var.name); > > > + if (rc && rc != -ENOENT) { > > > + pr_err("Failed to read variable '%s': %d\n", key, > > > rc); > > > + // Return -EIO since userspace probably doesn't > > > care about the > > > + // specific error > > > + rc = -EIO; > > > + } > > > + return rc; > > > +} > > > + > > > +static int plpks_set_variable(const char *key, u64 key_len, u8 > > > *data, > > > + u64 data_size) > > > +{ > > > + struct plpks_var var = {0}; > > > + 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)) > > > > So it's unstructured 8 byte of flags, not a u64 integer? Why not u8 > > flags[8] then? > > No, it's a u64 and it's passed in the hcall as a single u64. In host endian? This is done so userspace can acces it with the existing secvar API, right? I suppose that's okay... > > > +static ssize_t plpks_secvar_format(char *buf, size_t bufsize) > > > +{ > > > + struct plpks_var var = {0}; > > > + ssize_t ret; > > > + > > > + var.component = NULL; > > > + // Only the signed variables have null bytes in their > > > names, this one doesn't > > > + var.name = "SB_VERSION"; > > > + var.namelen = 10; > > > > Could you make that strlen(var.name) for the benefit of those of us > > with > > missing fingers? > > Will do. > > > > > > + var.datalen = 1; > > > + var.data = kzalloc(1, GFP_KERNEL); > > > > This could just point to a u8 on stack I think? > > Until we get VMAP_STACK and we'll have to switch back. AFAIKS plpks_read_var does not require linear map, so should not be required. IMO that's the right way to go for an external API, so actually plpks_write_var is the outlier there and should be changed to follow read and remove in not requiring special pointers. Thanks, Nick
On Tue, 2023-01-31 at 18:55 +1000, Nicholas Piggin wrote: > > > > + var.datalen = 1; > > > > + var.data = kzalloc(1, GFP_KERNEL); > > > > > > This could just point to a u8 on stack I think? > > > > Until we get VMAP_STACK and we'll have to switch back. > > AFAIKS plpks_read_var does not require linear map, so should not be > required. IMO that's the right way to go for an external API, so > actually plpks_write_var is the outlier there and should be changed > to follow read and remove in not requiring special pointers. Indeed, I was confused - in the read case, the buffer doesn't get directly passed to the hcall. I'll wait a little bit longer for more feedback on v5 of this series and maybe fix this in v6 if mpe thinks I should respin it again.
diff --git a/Documentation/ABI/testing/sysfs-secvar b/Documentation/ABI/testing/sysfs-secvar index feebb8c57294..a19f4d5fcec6 100644 --- a/Documentation/ABI/testing/sysfs-secvar +++ b/Documentation/ABI/testing/sysfs-secvar @@ -18,6 +18,14 @@ Description: A string indicating which backend is in use by the firmware. This determines the format of the variable and the accepted format of variable updates. + On powernv/OPAL, this value is provided by the OPAL firmware + and is expected to be "ibm,edk2-compat-v1". + + On pseries/PLPKS, this is generated by the kernel based on the + version number in the SB_VERSION variable in the keystore, and + has the form "ibm,plpks-sb-v<version>", or + "ibm,plpks-sb-unknown" if there is no SB_VERSION variable. + What: /sys/firmware/secvar/vars/<variable name> Date: August 2019 Contact: Nayna Jain <nayna@linux.ibm.com> @@ -34,7 +42,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@linux.ibm.com> +Contact: Nayna Jain <nayna@linux.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 +52,68 @@ Contact: Nayna Jain <nayna@linux.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@linux.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@linux.ibm.com> +Description: Config version as reported by the hypervisor in ASCII decimal + format. + + Currently only provided by PLPKS on the pseries platform. + +What: /sys/firmware/secvar/config/max_object_size +Date: December 2022 +Contact: Nayna Jain <nayna@linux.ibm.com> +Description: 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. + + Currently only provided by PLPKS on the pseries platform. + +What: /sys/firmware/secvar/config/total_size +Date: December 2022 +Contact: Nayna Jain <nayna@linux.ibm.com> +Description: Total size of the PLPKS in bytes, represented in ASCII decimal + format. + + Currently only provided by PLPKS on the pseries platform. + +What: /sys/firmware/secvar/config/used_space +Date: December 2022 +Contact: Nayna Jain <nayna@linux.ibm.com> +Description: Current space consumed by the key store, in bytes, represented + in ASCII decimal format. + + Currently only provided by PLPKS on the pseries platform. + +What: /sys/firmware/secvar/config/supported_policies +Date: December 2022 +Contact: Nayna Jain <nayna@linux.ibm.com> +Description: 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. + + Currently only provided by PLPKS on the pseries platform. + +What: /sys/firmware/secvar/config/signed_update_algorithms +Date: December 2022 +Contact: Nayna Jain <nayna@linux.ibm.com> +Description: Bitmask of flags indicating which algorithms the hypervisor + supports for signed update of objects, represented as a 16 byte + hexadecimal ASCII string. Consult the hypervisor documentation + for what these flags mean. + + Currently only provided by PLPKS on the pseries platform. diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile index 92310202bdd7..d52b7ec1a678 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_PPC_SECVAR_SYSFS) += 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..a80d9f9469f9 --- /dev/null +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: GPL-2.0-only + +// Secure variable implementation using the PowerVM LPAR Platform KeyStore (PLPKS) +// +// Copyright 2022, 2023 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 <linux/nls.h> +#include <asm/machdep.h> +#include <asm/secvar.h> +#include <asm/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 u32 get_policy(const char *name) +{ + if ((strcmp(name, "db") == 0) || + (strcmp(name, "dbx") == 0) || + (strcmp(name, "grubdb") == 0) || + (strcmp(name, "grubdbx") == 0) || + (strcmp(name, "sbat") == 0)) + return (PLPKS_WORLDREADABLE | PLPKS_SIGNEDUPDATE); + else + return PLPKS_SIGNEDUPDATE; +} + +static const char * const plpks_var_names[] = { + "PK", + "KEK", + "db", + "dbx", + "grubdb", + "grubdbx", + "sbat", + "moduledb", + "trustedcadb", + NULL, +}; + +static int plpks_get_variable(const char *key, u64 key_len, u8 *data, + u64 *data_size) +{ + struct plpks_var var = {0}; + int rc = 0; + + var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL); + if (!var.name) + return -ENOMEM; + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name, + key_len - 1); + if (rc < 0) + goto err; + var.namelen = rc * 2; + + var.os = PLPKS_VAR_LINUX; + if (data) { + var.data = data; + var.datalen = *data_size; + } + rc = plpks_read_os_var(&var); + + if (rc) + goto err; + + *data_size = var.datalen; + +err: + kfree(var.name); + if (rc && rc != -ENOENT) { + pr_err("Failed to read variable '%s': %d\n", key, rc); + // Return -EIO since userspace probably doesn't care about the + // specific error + rc = -EIO; + } + return rc; +} + +static int plpks_set_variable(const char *key, u64 key_len, u8 *data, + u64 data_size) +{ + struct plpks_var var = {0}; + 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; + + var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL); + if (!var.name) + return -ENOMEM; + rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name, + key_len - 1); + if (rc < 0) + goto err; + var.namelen = rc * 2; + + memcpy(&flags, data, sizeof(flags)); + + var.datalen = data_size - sizeof(flags); + var.data = data + sizeof(flags); + var.os = PLPKS_VAR_LINUX; + var.policy = get_policy(key); + + // Unlike in the read case, the plpks error code can be useful to + // userspace on write, so we return it rather than just -EIO + rc = plpks_signed_update_var(&var, flags); + +err: + kfree(var.name); + return rc; +} + +// 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, size_t bufsize) +{ + struct plpks_var var = {0}; + ssize_t ret; + + var.component = NULL; + // Only the signed variables have null bytes in their names, this one doesn't + var.name = "SB_VERSION"; + var.namelen = 10; + var.datalen = 1; + var.data = kzalloc(1, GFP_KERNEL); + if (!var.data) + return -ENOMEM; + + // 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) { + ret = snprintf(buf, bufsize, "ibm,plpks-sb-unknown"); + } else { + pr_err("Error %ld reading SB_VERSION from firmware\n", ret); + ret = -EIO; + } + goto err; + } + + // This string is made up by us - the hypervisor doesn't provide us + // with a format string in the way that OPAL firmware does. Hypervisor + // defines SB_VERSION as a "1 byte unsigned integer value". + ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", var.data[0]); + +err: + kfree(var.data); + return ret; +} + +static int plpks_max_size(u64 *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 = (u64)plpks_get_maxobjectsize() + 8; + + return 0; +} + + +static const struct secvar_operations plpks_secvar_ops = { + .get = plpks_get_variable, + .set = plpks_set_variable, + .format = plpks_secvar_format, + .max_size = plpks_max_size, + .config_attrs = config_attrs, + .var_names = plpks_var_names, +}; + +static int plpks_secvar_init(void) +{ + if (!plpks_is_available()) + return -ENODEV; + + return set_secvar_ops(&plpks_secvar_ops); +} +machine_device_initcall(pseries, plpks_secvar_init);