[v4,22/24] powerpc/pseries: Implement secvars for dynamic secure boot

  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
  

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.