Russell Currey [off-list ref] writes:

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 [off-list ref]
+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.

+		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 [off-list ref]
+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 [off-list ref]
+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 [off-list ref]
+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 [off-list ref]
+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_algorithms
+Date:		December 2022
+Contact:	Nayna Jain [off-list ref]
+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 we need that config option at all, or if we do it should
not be user selectable and just enabled automatically by PSERIES_PLPKS.

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.

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.

+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?

+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().

+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?

 - 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.

+	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.

+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?

+	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.

+	}
+
+	// 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?

+	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().

cheers