Re: [PATCH 2/2] powerpc: expose secure variables via sysfs

On Thu, Jun 13, 2019 at 04:50:27PM -0400, Nayna Jain wrote:

As part of PowerNV secure boot support, OS verification keys are stored
and controlled by OPAL as secure variables. These need to be exposed to
the userspace so that sysadmins can perform key management tasks.

This patch adds the support to expose secure variables via a sysfs
interface It reuses the the existing efi defined hooks and backend in
order to maintain the compatibility with the userspace tools.

Though it reuses a great deal of efi, POWER platforms do not use EFI.
A new config, POWER_SECVAR_SYSFS, is defined to enable this new sysfs
interface.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig                 |   2 +
 drivers/firmware/Makefile            |   1 +
 drivers/firmware/efi/efivars.c       |   2 +-
 drivers/firmware/powerpc/Kconfig     |  12 +
 drivers/firmware/powerpc/Makefile    |   3 +
 drivers/firmware/powerpc/efi_error.c |  46 ++++
 drivers/firmware/powerpc/secvar.c    | 326 +++++++++++++++++++++++++++
 7 files changed, 391 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/powerpc/Kconfig
 create mode 100644 drivers/firmware/powerpc/Makefile
 create mode 100644 drivers/firmware/powerpc/efi_error.c
 create mode 100644 drivers/firmware/powerpc/secvar.c

If you add/remove/modify sysfs files, you also need to update the
relevant Documentation/ABI/ entry as well.  Please add something there
to describe your new files when you resend the next version of this
patch series.

default is always n, no need to list it.

Why not just export the symbol from the original file instead of
duplicating it here?

+static int convert_buffer_to_efi_guid(u8 *buffer, efi_guid_t *guid)
+{
+	u32 *a1;
+	u16 *a2;
+	u16 *a3;
+
+	a1 = kzalloc(4, GFP_KERNEL);

No error checking in this function for memory issues at all?

+	memcpy(a1, buffer, 4);
+	*a1 = be32_to_cpu(*a1);
+
+	a2 = kzalloc(2, GFP_KERNEL);
+	memcpy(a2, buffer+4, 2);
+	*a2 = be16_to_cpu(*a2);
+
+	a3 = kzalloc(2, GFP_KERNEL);
+	memcpy(a3, buffer+6, 2);
+	*a3 = be16_to_cpu(*a3);
+
+	*guid = EFI_GUID(*a1, *a2, *a3, *(buffer + 8),
+			*(buffer + 9),
+			*(buffer + 10),
+			*(buffer + 11),
+			*(buffer + 12),
+			*(buffer + 13),
+			*(buffer + 14),
+			*(buffer + 15));
+
+	kfree(a1);
+	kfree(a2);
+	kfree(a3);
+	return 0;
+}
+static efi_status_t powerpc_get_next_variable(unsigned long *name_size,
+					      efi_char16_t *name,
+					      efi_guid_t *vendor)
+{
+	int rc;
+	u8 *key;
+	int namesize;
+	unsigned long keylen;
+	unsigned long keysize = 1024;
+	unsigned long *mdsize;
+	u8 *mdata = NULL;
+	efi_guid_t guid;
+
+	if (ucs2_strnlen(name, 1024) > 0) {
+		createkey(name, &key, &keylen);
+	} else {
+		keylen = 0;
+		key = kzalloc(1024, GFP_KERNEL);
+	}
+
+	pr_info("%s: powerpc get next variable, key is %s\n", __func__, key);

Don't put debugging info like this in the kernel log of everyone :(

+
+	rc = opal_get_next_variable(key, &keylen, keysize);
+	if (rc) {
+		kfree(key);
+		return opal_to_efi_status(rc);
+	}
+
+	mdsize = kzalloc(sizeof(unsigned long), GFP_KERNEL);

No error checking?

+	rc = opal_get_variable_size(key, keylen, mdsize, NULL);
+	if (rc)
+		goto out;
+
+	if (*mdsize <= 0)
+		goto out;
+
+	mdata = kzalloc(*mdsize, GFP_KERNEL);
+
+	rc = opal_get_variable(key, keylen, mdata, mdsize, NULL, NULL);
+	if (rc)
+		goto out;
+
+	if (*mdsize > 0) {
+		namesize = *mdsize - sizeof(efi_guid_t) - sizeof(u32);
+		if (namesize > 0) {
+			memset(&guid, 0, sizeof(efi_guid_t));
+			convert_buffer_to_efi_guid(mdata + namesize, &guid);
+			memcpy(vendor, &guid, sizeof(efi_guid_t));
+			memset(name, 0, namesize + 2);
+			memcpy(name, mdata, namesize);
+			*name_size = namesize + 2;
+			name[namesize++] = 0;
+			name[namesize] = 0;
+		}
+	}
+
+out:
+	kfree(mdsize);
+	kfree(mdata);
+
+	return opal_to_efi_status(rc);
+}
+
+static efi_status_t powerpc_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+					 u32 attr, unsigned long data_size,
+					 void *data)
+{
+	int rc;
+	u8 *key;
+	unsigned long keylen;
+	u8 *metadata;
+	unsigned long mdsize;
+
+	if (!name)
+		return EFI_INVALID_PARAMETER;
+
+	if (!vendor)
+		return EFI_INVALID_PARAMETER;
+
+	createkey(name, &key, &keylen);
+	pr_info("%s: nayna key is %s\n", __func__, key);

Again, please remove all of your debugging code when resending.

+
+	createmetadata(name, vendor, &attr, &metadata, &mdsize);
+
+	rc = opal_set_variable(key, keylen, metadata, mdsize, data, data_size);
+
+	return opal_to_efi_status(rc);
+}
+
+
+static const struct efivar_operations efivar_ops = {
+	.get_variable = powerpc_get_variable,
+	.set_variable = powerpc_set_variable,
+	.get_next_variable = powerpc_get_next_variable,
+};
+
+
+static __init int power_secvar_init(void)
+{
+	int rc = 0;
+	unsigned long ver = 0;
+
+	rc = opal_variable_version(&ver);
+	if (ver != BACKEND_TC_COMPAT_V1) {
+		pr_info("Compatible backend unsupported\n");
+		return -1;

Do not make up error numbers, use the defined values please.

thanks,

greg k-h