Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] powerpc/powernv: expose secure variables to userspace 
@ 2019-06-13 20:50 Nayna Jain
  2019-06-13 20:50 ` [PATCH 1/2] powerpc/powernv: add OPAL APIs for secure variables Nayna Jain
  2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
  0 siblings, 2 replies; 9+ messages in thread
From: Nayna Jain @ 2019-06-13 20:50 UTC (permalink / raw)
  To: linuxppc-dev, linux-efi
  Cc: linux-kernel, linux-integrity, Michael Ellerman, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	Nayna Jain, George Wilson, Elaine Palmer, Eric Ricther

This patch set is part of a series that implements secure boot on PowerNV
systems[1]. The original series had been split into two patchsets:
1. powerpc: enable ima arch specific policies[2]
2. powerpc/powernv: expose secure variables to userspace, which is this
patchset.

Since there are major changes in this patchset compared to the previous
one[1], I am posting it as new series rather than v2.

As part of PowerNV secure boot support, NV OS verification keys are stored
and controlled by OPAL as secure variables. However, to allow users to
manage these keys, the secure variables need to be exposed to userspace.

OPAL provides the runtime services for the kernel to be able to access the
secure variables[3]. This patchset defines the kernel interface for the
OPAL APIs. These APIs are used by the hooks, which expose these variables
to userspace for reading/writing.

In order to reuse the existing tools, we currently use the efi hooks to
expose the secure variables via sysfs. Keeping the usability and
maintainability in mind, we are starting with this scheme as simple sysfs
implementation. We expect to refine it over time as we incorporate the
feedback.

The patchset makes substantial reuse of drivers/firmware/efi/efivars.c and
drivers/firmware/efi/vars.c, however because POWER platforms do not use
EFI, a new config, POWER_SECVAR_SYSFS, is defined to enable this sysfs
interface in POWER.

This patchset has a pre-requisiste of other OPAL APIs which are posted as
part of ima arch specific patches[2].

[1]https://patchwork.kernel.org/cover/10882149/  
[2]https://lkml.org/lkml/2019/6/11/868
[3]https://patchwork.ozlabs.org/project/skiboot/list/?series=112868 

Claudio Carvalho (1):
  powerpc/powernv: add OPAL APIs for secure variables

Nayna Jain (1):
  powerpc: expose secure variables via sysfs

 arch/powerpc/Kconfig                         |   2 +
 arch/powerpc/include/asm/opal-api.h          |   3 +
 arch/powerpc/include/asm/opal-secvar.h       |   9 +
 arch/powerpc/include/asm/opal.h              |   8 +
 arch/powerpc/platforms/powernv/opal-call.c   |   3 +
 arch/powerpc/platforms/powernv/opal-secvar.c |  60 +++-
 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 +++++++++++++++++++
 12 files changed, 473 insertions(+), 2 deletions(-)
 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

-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] powerpc/powernv: add OPAL APIs for secure variables
  2019-06-13 20:50 [PATCH 0/2] powerpc/powernv: expose secure variables to userspace Nayna Jain
@ 2019-06-13 20:50 ` Nayna Jain
  2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
  1 sibling, 0 replies; 9+ messages in thread
From: Nayna Jain @ 2019-06-13 20:50 UTC (permalink / raw)
  To: linuxppc-dev, linux-efi
  Cc: linux-kernel, linux-integrity, Michael Ellerman, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	Nayna Jain, George Wilson, Elaine Palmer, Eric Ricther

From: Claudio Carvalho <cclaudio@linux.ibm.com>

The X.509 certificates trusted by the platform and other information
required to secure boot the OS kernel are wrapped in secure variables,
which are controlled by OPAL. These variables are manipulated by
userspace tools using filesystem interface. This patch adds support
for the OPAL APIs required to expose variables to userspace.

OPAL_SECVAR_GET_NEXT:
For a given secure variable, it returns the name and vendor GUID
of the next variable.

OPAL_SECVAR_ENQUEUE_UPDATE:
Enqueue the supplied secure variable update so that it can be processed
by OPAL in the next boot. Variable updates cannot be be processed right
away because the variable storage is write locked at runtime.

OPAL_SECVAR_GET_SIZE:
Returns size information about the variable.

Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/include/asm/opal-api.h          |  3 +
 arch/powerpc/include/asm/opal-secvar.h       |  9 +++
 arch/powerpc/include/asm/opal.h              |  8 +++
 arch/powerpc/platforms/powernv/opal-call.c   |  3 +
 arch/powerpc/platforms/powernv/opal-secvar.c | 60 +++++++++++++++++++-
 5 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index a505e669b4b6..fa3083966efc 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -213,6 +213,9 @@
 #define	OPAL_NX_COPROC_INIT			167
 #define OPAL_XIVE_GET_VP_STATE			170
 #define OPAL_SECVAR_GET                         173
+#define OPAL_SECVAR_GET_SIZE                    174
+#define OPAL_SECVAR_GET_NEXT                    175
+#define OPAL_SECVAR_ENQUEUE_UPDATE              176
 #define OPAL_SECVAR_BACKEND                     177
 #define OPAL_LAST				177
 
diff --git a/arch/powerpc/include/asm/opal-secvar.h b/arch/powerpc/include/asm/opal-secvar.h
index b677171a0368..26ebbc63dd70 100644
--- a/arch/powerpc/include/asm/opal-secvar.h
+++ b/arch/powerpc/include/asm/opal-secvar.h
@@ -20,4 +20,13 @@ extern int opal_get_variable(u8 *key, unsigned long ksize,
 
 extern int opal_variable_version(unsigned long *backend);
 
+extern int opal_get_variable_size(u8 *key, unsigned long ksize,
+				  unsigned long *mdsize, unsigned long *dsize);
+
+extern int opal_get_next_variable(u8 *key, unsigned long *keylen,
+				  unsigned long keysize);
+
+extern int opal_set_variable(u8 *key, unsigned long ksize, u8 *metadata,
+			     unsigned long mdsize, u8 *data,
+			     unsigned long dsize);
 #endif
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 57d2c2356eda..a6fcb59c91cc 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -399,6 +399,14 @@ extern int opal_secvar_get(uint64_t k_key, uint64_t k_key_len,
 			   uint64_t k_data, uint64_t k_data_size);
 
 extern int opal_secvar_backend(uint64_t k_backend);
+extern int opal_secvar_get_size(uint64_t k_key, uint64_t k_key_len,
+				uint64_t k_metadata_size, uint64_t k_data_size);
+extern int opal_secvar_get_next(uint64_t k_key, uint64_t k_key_len,
+				uint64_t k_key_size);
+extern int opal_secvar_enqueue_update(uint64_t k_key, uint64_t k_key_len,
+				      uint64_t k_metadata,
+				      uint64_t k_metadata_size,
+				      uint64_t k_data, uint64_t k_data_size);
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
index 0445980f294f..dda3a4c5bb79 100644
--- a/arch/powerpc/platforms/powernv/opal-call.c
+++ b/arch/powerpc/platforms/powernv/opal-call.c
@@ -290,3 +290,6 @@ OPAL_CALL(opal_sensor_group_enable,		OPAL_SENSOR_GROUP_ENABLE);
 OPAL_CALL(opal_nx_coproc_init,			OPAL_NX_COPROC_INIT);
 OPAL_CALL(opal_secvar_get,                      OPAL_SECVAR_GET);
 OPAL_CALL(opal_secvar_backend,                  OPAL_SECVAR_BACKEND);
+OPAL_CALL(opal_secvar_get_size,                 OPAL_SECVAR_GET_SIZE);
+OPAL_CALL(opal_secvar_get_next,                 OPAL_SECVAR_GET_NEXT);
+OPAL_CALL(opal_secvar_enqueue_update,           OPAL_SECVAR_ENQUEUE_UPDATE);
diff --git a/arch/powerpc/platforms/powernv/opal-secvar.c b/arch/powerpc/platforms/powernv/opal-secvar.c
index dba441dd5af1..afa67b87ad7a 100644
--- a/arch/powerpc/platforms/powernv/opal-secvar.c
+++ b/arch/powerpc/platforms/powernv/opal-secvar.c
@@ -30,7 +30,10 @@ static bool is_opal_secvar_supported(void)
 		return opal_secvar_supported;
 
 	if (!opal_check_token(OPAL_SECVAR_GET)
-	    || !opal_check_token(OPAL_SECVAR_BACKEND)) {
+	    || !opal_check_token(OPAL_SECVAR_BACKEND)
+	    || !opal_check_token(OPAL_SECVAR_GET_SIZE)
+	    || !opal_check_token(OPAL_SECVAR_GET_NEXT)
+	    || !opal_check_token(OPAL_SECVAR_ENQUEUE_UPDATE)) {
 		pr_err("OPAL doesn't support secure variables\n");
 		opal_secvar_supported = false;
 	} else {
@@ -83,3 +86,58 @@ int opal_variable_version(unsigned long *backend)
 
 	return rc;
 }
+
+int opal_get_variable_size(u8 *key, unsigned long ksize, unsigned long *mdsize,
+			   unsigned long *dsize)
+{
+	int rc;
+
+	if (!is_opal_secvar_supported())
+		return OPAL_UNSUPPORTED;
+
+	if (mdsize)
+		*mdsize = cpu_to_be64(*mdsize);
+	if (dsize)
+		*dsize = cpu_to_be64(*dsize);
+
+	rc = opal_secvar_get_size(__pa(key), ksize, __pa(mdsize), __pa(dsize));
+
+	if (mdsize)
+		*mdsize = be64_to_cpu(*mdsize);
+	if (dsize)
+		*dsize = be64_to_cpu(*dsize);
+	return rc;
+}
+
+int opal_get_next_variable(u8 *key, unsigned long *keylen,
+			   unsigned long keysize)
+{
+	int rc;
+
+	if (!is_opal_secvar_supported())
+		return OPAL_UNSUPPORTED;
+
+	if (!keylen)
+		return OPAL_PARAMETER;
+	*keylen = cpu_to_be64(*keylen);
+
+	rc = opal_secvar_get_next(__pa(key), __pa(keylen), keysize);
+
+	*keylen = be64_to_cpu(*keylen);
+
+	return rc;
+}
+
+int opal_set_variable(u8 *key, unsigned long ksize, u8 *metadata,
+		      unsigned long mdsize, u8 *data, unsigned long dsize)
+{
+	int rc;
+
+	if (!is_opal_secvar_supported())
+		return OPAL_UNSUPPORTED;
+
+	rc = opal_secvar_enqueue_update(__pa(key), ksize, __pa(metadata),
+			mdsize, __pa(data), dsize);
+
+	return rc;
+}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-06-13 20:50 [PATCH 0/2] powerpc/powernv: expose secure variables to userspace Nayna Jain
  2019-06-13 20:50 ` [PATCH 1/2] powerpc/powernv: add OPAL APIs for secure variables Nayna Jain
@ 2019-06-13 20:50 ` Nayna Jain
  2019-06-14  6:34   ` Greg Kroah-Hartman
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Nayna Jain @ 2019-06-13 20:50 UTC (permalink / raw)
  To: linuxppc-dev, linux-efi
  Cc: linux-kernel, linux-integrity, Michael Ellerman, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	Nayna Jain, George Wilson, Elaine Palmer, Eric Ricther

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

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 9de77bb14f54..1548dd8cf1a0 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -916,6 +916,8 @@ config PPC_SECURE_BOOT
 	  allows user to enable OS Secure Boot on PowerPC systems that
 	  have firmware secure boot support.
 
+source "drivers/firmware/powerpc/Kconfig"
+
 endmenu
 
 config ISA_DMA_API
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 3fa0b34eb72f..8cfaf7e6769d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_UEFI_CPER)		+= efi/
 obj-y				+= imx/
 obj-y				+= tegra/
 obj-y				+= xilinx/
+obj-$(CONFIG_POWER_SECVAR_SYSFS) += powerpc/
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 7576450c8254..30ef53003c24 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -664,7 +664,7 @@ int efivars_sysfs_init(void)
 	struct kobject *parent_kobj = efivars_kobject();
 	int error = 0;
 
-	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+	if (IS_ENABLED(CONFIG_EFI) && !efi_enabled(EFI_RUNTIME_SERVICES))
 		return -ENODEV;
 
 	/* No efivars has been registered yet */
diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
new file mode 100644
index 000000000000..e0303fc517d5
--- /dev/null
+++ b/drivers/firmware/powerpc/Kconfig
@@ -0,0 +1,12 @@
+config POWER_SECVAR_SYSFS
+	tristate "Enable sysfs interface for POWER secure variables"
+	default n
+	depends on PPC_SECURE_BOOT
+	select UCS2_STRING
+	help
+	  POWER secure variables are managed and controlled by OPAL.
+	  These variables are exposed to userspace via sysfs to allow
+	  user to read/write these variables. Say Y if you have secure
+	  boot enabled and want to expose variables to userspace.
+
+source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/powerpc/Makefile b/drivers/firmware/powerpc/Makefile
new file mode 100644
index 000000000000..d5fa3b007315
--- /dev/null
+++ b/drivers/firmware/powerpc/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_POWER_SECVAR_SYSFS) += ../efi/efivars.o efi_error.o ../efi/vars.o secvar.o
diff --git a/drivers/firmware/powerpc/efi_error.c b/drivers/firmware/powerpc/efi_error.c
new file mode 100644
index 000000000000..b5cabd52e6b4
--- /dev/null
+++ b/drivers/firmware/powerpc/efi_error.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * efi_error.c
+ *      - Error codes as understood by efi based tools
+ *      Taken from drivers/firmware/efi/efi.c
+ */
+#include<linux/efi.h>
+
+int efi_status_to_err(efi_status_t status)
+{
+	int err;
+
+	switch (status) {
+	case EFI_SUCCESS:
+		err = 0;
+		break;
+	case EFI_INVALID_PARAMETER:
+		err = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		err = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		err = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		err = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		err = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		err = -ENOENT;
+		break;
+	case EFI_ABORTED:
+		err = -EINTR;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
diff --git a/drivers/firmware/powerpc/secvar.c b/drivers/firmware/powerpc/secvar.c
new file mode 100644
index 000000000000..f1f134a0bb7c
--- /dev/null
+++ b/drivers/firmware/powerpc/secvar.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * secvar.c
+ * - wrappers to expose secure variables to userspace
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/spinlock.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/ioctl.h>
+#include <linux/uaccess.h>
+#include <linux/kdebug.h>
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/ucs2_string.h>
+#include <asm/opal-secvar.h>
+#include <asm/opal.h>
+
+static struct efivars efivars;
+struct kobject *powerpc_kobj;
+
+efi_status_t opal_to_efi_status_log(int rc, const char *func_name)
+{
+	efi_status_t status;
+
+	switch (rc) {
+	case OPAL_EMPTY:
+		status = EFI_NOT_FOUND;
+		break;
+	case OPAL_HARDWARE:
+		status = EFI_DEVICE_ERROR;
+		break;
+	case OPAL_NO_MEM:
+		pr_err("%s: No space in the volatile storage\n", func_name);
+		status = EFI_OUT_OF_RESOURCES;
+		break;
+	case OPAL_PARAMETER:
+		status = EFI_INVALID_PARAMETER;
+		break;
+	case OPAL_PARTIAL:
+		status = EFI_BUFFER_TOO_SMALL;
+		break;
+	case OPAL_PERMISSION:
+		status = EFI_WRITE_PROTECTED;
+		break;
+	case OPAL_RESOURCE:
+		pr_err("%s: No space in the non-volatile storage\n", func_name);
+		status = EFI_OUT_OF_RESOURCES;
+		break;
+	case OPAL_SUCCESS:
+		status = EFI_SUCCESS;
+		break;
+	default:
+		pr_err("%s: Unknown OPAL error %d\n", func_name, rc);
+		status = EFI_DEVICE_ERROR;
+		break;
+	}
+
+	return status;
+}
+
+#define opal_to_efi_status(rc) opal_to_efi_status_log(rc, __func__)
+
+static void createkey(efi_char16_t *name, u8 **key, unsigned long *keylen)
+{
+	*keylen = ucs2_utf8size(name) + 1;
+
+	*key = kzalloc(*keylen, GFP_KERNEL);
+	if (!*key) {
+		*keylen = 0;
+		*key = NULL;
+		return;
+	}
+
+	ucs2_as_utf8(*key, name, *keylen);
+}
+
+static void createmetadata(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+			   u8 **mdata, unsigned long *mdsize)
+{
+	int size = 0;
+
+	*mdsize = ucs2_strsize(name, 1024) + sizeof(efi_guid_t) + sizeof(u32);
+	*mdata = kzalloc(*mdsize, GFP_KERNEL);
+
+	memcpy(*mdata, name, ucs2_strsize(name, 1024));
+	size = ucs2_strsize(name, 1024);
+
+	memcpy(*mdata + size, vendor, sizeof(efi_guid_t));
+	size += sizeof(efi_guid_t);
+
+	if (attr != NULL)
+		memcpy(*mdata + size, attr, sizeof(u32));
+	else
+		memset(*mdata + size, 0, sizeof(u32));
+}
+
+static int convert_buffer_to_efi_guid(u8 *buffer, efi_guid_t *guid)
+{
+	u32 *a1;
+	u16 *a2;
+	u16 *a3;
+
+	a1 = kzalloc(4, GFP_KERNEL);
+	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_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;
+	unsigned long dsize;
+	unsigned long namesize;
+
+	if (!name)
+		return EFI_INVALID_PARAMETER;
+
+	if (!vendor)
+		return EFI_INVALID_PARAMETER;
+
+	if (*data_size == 0) {
+		/* If *data_size is zero, it implies data size is being asked */
+		createkey(name, &key, &keylen);
+		rc = opal_get_variable_size(key, keylen, &mdsize, &dsize);
+		*data_size = dsize;
+		kfree(key);
+		return opal_to_efi_status(rc);
+	}
+
+	createkey(name, &key, &keylen);
+	createmetadata(name, vendor, attr, &metadata, &mdsize);
+
+	rc = opal_get_variable(key, keylen, metadata, &mdsize, data, data_size);
+
+	if (rc)
+		return opal_to_efi_status(rc);
+
+	if (mdsize > 0) {
+		namesize = mdsize - sizeof(efi_guid_t) - sizeof(u32);
+		if (!attr)
+			return opal_to_efi_status(rc);
+		memset(attr, 0, sizeof(u32));
+		memcpy(attr, metadata + namesize + sizeof(efi_guid_t),
+		       sizeof(u32));
+		*attr = be32_to_cpu(*attr);
+	}
+
+	kfree(key);
+	kfree(metadata);
+
+	return opal_to_efi_status(rc);
+}
+
+
+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);
+
+	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);
+	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);
+
+	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;
+	}
+
+	powerpc_kobj = kobject_create_and_add("secvar", firmware_kobj);
+	if (!powerpc_kobj) {
+		pr_info("secvar: Failed to create firmware kobj\n");
+		goto out_err;
+	}
+
+	rc = efivars_register(&efivars, &efivar_ops, powerpc_kobj);
+	if (rc) {
+		pr_info("powerpc: Failed to register efivars\n");
+		return rc;
+	}
+
+	return 0;
+out_err:
+	kobject_put(powerpc_kobj);
+	pr_info("powerpc: failed to load: %d\n", rc);
+	return rc;
+}
+arch_initcall(power_secvar_init);
+
+static void __exit power_secvar_exit(void)
+{
+	efivars_unregister(&efivars);
+	kobject_put(powerpc_kobj);
+}
+module_exit(power_secvar_exit);
+
+MODULE_AUTHOR("Nayna Jain");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
@ 2019-06-14  6:34   ` Greg Kroah-Hartman
  2019-06-14 13:13     ` Nayna
  2019-07-05  6:05   ` Michael Ellerman
  2019-07-22 10:19   ` Oliver O'Halloran
  2 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-14  6:34 UTC (permalink / raw)
  To: Nayna Jain
  Cc: linuxppc-dev, linux-efi, linux-kernel, linux-integrity,
	Michael Ellerman, Paul Mackerras, Benjamin Herrenschmidt,
	Ard Biesheuvel, Jeremy Kerr, Matthew Garret, Mimi Zohar,
	Claudio Carvalho, George Wilson, Elaine Palmer, Eric Ricther

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.

> diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
> new file mode 100644
> index 000000000000..e0303fc517d5
> --- /dev/null
> +++ b/drivers/firmware/powerpc/Kconfig
> @@ -0,0 +1,12 @@
> +config POWER_SECVAR_SYSFS
> +	tristate "Enable sysfs interface for POWER secure variables"
> +	default n

default is always n, no need to list it.

> --- /dev/null
> +++ b/drivers/firmware/powerpc/efi_error.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * efi_error.c
> + *      - Error codes as understood by efi based tools
> + *      Taken from drivers/firmware/efi/efi.c

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-06-14  6:34   ` Greg Kroah-Hartman
@ 2019-06-14 13:13     ` Nayna
  0 siblings, 0 replies; 9+ messages in thread
From: Nayna @ 2019-06-14 13:13 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nayna Jain, linuxppc-dev, linux-efi, linux-kernel,
	linux-integrity, Michael Ellerman, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Claudio Carvalho, George Wilson,
	Elaine Palmer, Eric Ricther



On 06/14/2019 02:34 AM, Greg Kroah-Hartman wrote:
> 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.
>
>> diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
>> new file mode 100644
>> index 000000000000..e0303fc517d5
>> --- /dev/null
>> +++ b/drivers/firmware/powerpc/Kconfig
>> @@ -0,0 +1,12 @@
>> +config POWER_SECVAR_SYSFS
>> +	tristate "Enable sysfs interface for POWER secure variables"
>> +	default n
> default is always n, no need to list it.
>
>> --- /dev/null
>> +++ b/drivers/firmware/powerpc/efi_error.c
>> @@ -0,0 +1,46 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
>> + *
>> + * efi_error.c
>> + *      - Error codes as understood by efi based tools
>> + *      Taken from drivers/firmware/efi/efi.c
> 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 for the review !!

I will address everything in the next version.

Thanks & Regards,
         - Nayna

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
  2019-06-14  6:34   ` Greg Kroah-Hartman
@ 2019-07-05  6:05   ` Michael Ellerman
  2019-07-23 14:35     ` Nayna
  2019-07-22 10:19   ` Oliver O'Halloran
  2 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2019-07-05  6:05 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-efi
  Cc: linux-kernel, linux-integrity, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	Nayna Jain, George Wilson, Elaine Palmer, Eric Ricther

Hi Nayna,

Nayna Jain <nayna@linux.ibm.com> writes:
> 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.

Which tools? Can you include a log demonstrating how they're used, ie.
so that I can test the sequence of commands.

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

Sorry I haven't been able to keep up with all the discussions, but I
thought the consensus was that pretending to be EFI-like was a bad idea,
because we don't have actual EFI and we're not implementing an entirely
compatible scheme to EFI anyway.

Greg suggested just putting the variables in sysfs, why does that not
work? Matthew mentioned "complex semantics around variable deletion and
immutability" but do we have to emulate those semantics on powerpc?

cheers


> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9de77bb14f54..1548dd8cf1a0 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT
>  	  allows user to enable OS Secure Boot on PowerPC systems that
>  	  have firmware secure boot support.
>  
> +source "drivers/firmware/powerpc/Kconfig"
> +
>  endmenu
>  
>  config ISA_DMA_API
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 3fa0b34eb72f..8cfaf7e6769d 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_UEFI_CPER)		+= efi/
>  obj-y				+= imx/
>  obj-y				+= tegra/
>  obj-y				+= xilinx/
> +obj-$(CONFIG_POWER_SECVAR_SYSFS) += powerpc/
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..30ef53003c24 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -664,7 +664,7 @@ int efivars_sysfs_init(void)
>  	struct kobject *parent_kobj = efivars_kobject();
>  	int error = 0;
>  
> -	if (!efi_enabled(EFI_RUNTIME_SERVICES))
> +	if (IS_ENABLED(CONFIG_EFI) && !efi_enabled(EFI_RUNTIME_SERVICES))
>  		return -ENODEV;
>  
>  	/* No efivars has been registered yet */
> diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
> new file mode 100644
> index 000000000000..e0303fc517d5
> --- /dev/null
> +++ b/drivers/firmware/powerpc/Kconfig
> @@ -0,0 +1,12 @@
> +config POWER_SECVAR_SYSFS
> +	tristate "Enable sysfs interface for POWER secure variables"
> +	default n
> +	depends on PPC_SECURE_BOOT
> +	select UCS2_STRING
> +	help
> +	  POWER secure variables are managed and controlled by OPAL.
> +	  These variables are exposed to userspace via sysfs to allow
> +	  user to read/write these variables. Say Y if you have secure
> +	  boot enabled and want to expose variables to userspace.
> +
> +source "drivers/firmware/efi/Kconfig"
> diff --git a/drivers/firmware/powerpc/Makefile b/drivers/firmware/powerpc/Makefile
> new file mode 100644
> index 000000000000..d5fa3b007315
> --- /dev/null
> +++ b/drivers/firmware/powerpc/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_POWER_SECVAR_SYSFS) += ../efi/efivars.o efi_error.o ../efi/vars.o secvar.o
> diff --git a/drivers/firmware/powerpc/efi_error.c b/drivers/firmware/powerpc/efi_error.c
> new file mode 100644
> index 000000000000..b5cabd52e6b4
> --- /dev/null
> +++ b/drivers/firmware/powerpc/efi_error.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * efi_error.c
> + *      - Error codes as understood by efi based tools
> + *      Taken from drivers/firmware/efi/efi.c
> + */
> +#include<linux/efi.h>
> +
> +int efi_status_to_err(efi_status_t status)
> +{
> +	int err;
> +
> +	switch (status) {
> +	case EFI_SUCCESS:
> +		err = 0;
> +		break;
> +	case EFI_INVALID_PARAMETER:
> +		err = -EINVAL;
> +		break;
> +	case EFI_OUT_OF_RESOURCES:
> +		err = -ENOSPC;
> +		break;
> +	case EFI_DEVICE_ERROR:
> +		err = -EIO;
> +		break;
> +	case EFI_WRITE_PROTECTED:
> +		err = -EROFS;
> +		break;
> +	case EFI_SECURITY_VIOLATION:
> +		err = -EACCES;
> +		break;
> +	case EFI_NOT_FOUND:
> +		err = -ENOENT;
> +		break;
> +	case EFI_ABORTED:
> +		err = -EINTR;
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +
> +	return err;
> +}
> diff --git a/drivers/firmware/powerpc/secvar.c b/drivers/firmware/powerpc/secvar.c
> new file mode 100644
> index 000000000000..f1f134a0bb7c
> --- /dev/null
> +++ b/drivers/firmware/powerpc/secvar.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * secvar.c
> + * - wrappers to expose secure variables to userspace
> + */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/spinlock.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/ioctl.h>
> +#include <linux/uaccess.h>
> +#include <linux/kdebug.h>
> +#include <linux/efi.h>
> +#include <linux/module.h>
> +#include <linux/ucs2_string.h>
> +#include <asm/opal-secvar.h>
> +#include <asm/opal.h>
> +
> +static struct efivars efivars;
> +struct kobject *powerpc_kobj;
> +
> +efi_status_t opal_to_efi_status_log(int rc, const char *func_name)
> +{
> +	efi_status_t status;
> +
> +	switch (rc) {
> +	case OPAL_EMPTY:
> +		status = EFI_NOT_FOUND;
> +		break;
> +	case OPAL_HARDWARE:
> +		status = EFI_DEVICE_ERROR;
> +		break;
> +	case OPAL_NO_MEM:
> +		pr_err("%s: No space in the volatile storage\n", func_name);
> +		status = EFI_OUT_OF_RESOURCES;
> +		break;
> +	case OPAL_PARAMETER:
> +		status = EFI_INVALID_PARAMETER;
> +		break;
> +	case OPAL_PARTIAL:
> +		status = EFI_BUFFER_TOO_SMALL;
> +		break;
> +	case OPAL_PERMISSION:
> +		status = EFI_WRITE_PROTECTED;
> +		break;
> +	case OPAL_RESOURCE:
> +		pr_err("%s: No space in the non-volatile storage\n", func_name);
> +		status = EFI_OUT_OF_RESOURCES;
> +		break;
> +	case OPAL_SUCCESS:
> +		status = EFI_SUCCESS;
> +		break;
> +	default:
> +		pr_err("%s: Unknown OPAL error %d\n", func_name, rc);
> +		status = EFI_DEVICE_ERROR;
> +		break;
> +	}
> +
> +	return status;
> +}
> +
> +#define opal_to_efi_status(rc) opal_to_efi_status_log(rc, __func__)
> +
> +static void createkey(efi_char16_t *name, u8 **key, unsigned long *keylen)
> +{
> +	*keylen = ucs2_utf8size(name) + 1;
> +
> +	*key = kzalloc(*keylen, GFP_KERNEL);
> +	if (!*key) {
> +		*keylen = 0;
> +		*key = NULL;
> +		return;
> +	}
> +
> +	ucs2_as_utf8(*key, name, *keylen);
> +}
> +
> +static void createmetadata(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> +			   u8 **mdata, unsigned long *mdsize)
> +{
> +	int size = 0;
> +
> +	*mdsize = ucs2_strsize(name, 1024) + sizeof(efi_guid_t) + sizeof(u32);
> +	*mdata = kzalloc(*mdsize, GFP_KERNEL);
> +
> +	memcpy(*mdata, name, ucs2_strsize(name, 1024));
> +	size = ucs2_strsize(name, 1024);
> +
> +	memcpy(*mdata + size, vendor, sizeof(efi_guid_t));
> +	size += sizeof(efi_guid_t);
> +
> +	if (attr != NULL)
> +		memcpy(*mdata + size, attr, sizeof(u32));
> +	else
> +		memset(*mdata + size, 0, sizeof(u32));
> +}
> +
> +static int convert_buffer_to_efi_guid(u8 *buffer, efi_guid_t *guid)
> +{
> +	u32 *a1;
> +	u16 *a2;
> +	u16 *a3;
> +
> +	a1 = kzalloc(4, GFP_KERNEL);
> +	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_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;
> +	unsigned long dsize;
> +	unsigned long namesize;
> +
> +	if (!name)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (!vendor)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (*data_size == 0) {
> +		/* If *data_size is zero, it implies data size is being asked */
> +		createkey(name, &key, &keylen);
> +		rc = opal_get_variable_size(key, keylen, &mdsize, &dsize);
> +		*data_size = dsize;
> +		kfree(key);
> +		return opal_to_efi_status(rc);
> +	}
> +
> +	createkey(name, &key, &keylen);
> +	createmetadata(name, vendor, attr, &metadata, &mdsize);
> +
> +	rc = opal_get_variable(key, keylen, metadata, &mdsize, data, data_size);
> +
> +	if (rc)
> +		return opal_to_efi_status(rc);
> +
> +	if (mdsize > 0) {
> +		namesize = mdsize - sizeof(efi_guid_t) - sizeof(u32);
> +		if (!attr)
> +			return opal_to_efi_status(rc);
> +		memset(attr, 0, sizeof(u32));
> +		memcpy(attr, metadata + namesize + sizeof(efi_guid_t),
> +		       sizeof(u32));
> +		*attr = be32_to_cpu(*attr);
> +	}
> +
> +	kfree(key);
> +	kfree(metadata);
> +
> +	return opal_to_efi_status(rc);
> +}
> +
> +
> +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);
> +
> +	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);
> +	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);
> +
> +	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;
> +	}
> +
> +	powerpc_kobj = kobject_create_and_add("secvar", firmware_kobj);
> +	if (!powerpc_kobj) {
> +		pr_info("secvar: Failed to create firmware kobj\n");
> +		goto out_err;
> +	}
> +
> +	rc = efivars_register(&efivars, &efivar_ops, powerpc_kobj);
> +	if (rc) {
> +		pr_info("powerpc: Failed to register efivars\n");
> +		return rc;
> +	}
> +
> +	return 0;
> +out_err:
> +	kobject_put(powerpc_kobj);
> +	pr_info("powerpc: failed to load: %d\n", rc);
> +	return rc;
> +}
> +arch_initcall(power_secvar_init);
> +
> +static void __exit power_secvar_exit(void)
> +{
> +	efivars_unregister(&efivars);
> +	kobject_put(powerpc_kobj);
> +}
> +module_exit(power_secvar_exit);
> +
> +MODULE_AUTHOR("Nayna Jain");
> +MODULE_LICENSE("GPL");
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
  2019-06-14  6:34   ` Greg Kroah-Hartman
  2019-07-05  6:05   ` Michael Ellerman
@ 2019-07-22 10:19   ` Oliver O'Halloran
  2 siblings, 0 replies; 9+ messages in thread
From: Oliver O'Halloran @ 2019-07-22 10:19 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-efi
  Cc: Ard Biesheuvel, Eric Ricther, linux-kernel, Mimi Zohar,
	Claudio Carvalho, Matthew Garret, Greg Kroah-Hartman,
	Paul Mackerras, Jeremy Kerr, Elaine Palmer, linux-integrity,
	George Wilson

On Thu, 2019-06-13 at 16:50 -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.

I spent all day staring at the skiboot bits of this, so I figured I
should see what the kernel bits looked like too...

On the last version of this patch set (posted by Claudio) Matthew
Garret was pretty explicit about the interface here not conforming to
the expectations of efivars[1], so what's changed in this patch set to
fix that?

As far as I can tell it's has the same basic problems as before, just
with a slightly different OPAL interface.

[1] https://www.spinics.net/lists/linux-efi/msg15897.html

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

> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9de77bb14f54..1548dd8cf1a0 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT
>  	  allows user to enable OS Secure Boot on PowerPC systems that
>  	  have firmware secure boot support.
>  
> +source "drivers/firmware/powerpc/Kconfig"
> +
>  endmenu

All of this is *very* powernv specific, so why is it here and not in
arch/powerpc/platforms/powernv/ with the rest of the powernv platform
code? Also, nothing about this is generic to powerpc as a whole so the
Kconfig symbol should probably be change the Kconfig symbol to
POWERNV_SECURE_BOOT or OPAL_SECURE_BOOT

>  config ISA_DMA_API
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 3fa0b34eb72f..8cfaf7e6769d 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_UEFI_CPER)		+= efi/
>  obj-y				+= imx/
>  obj-y				+= tegra/
>  obj-y				+= xilinx/
> +obj-$(CONFIG_POWER_SECVAR_SYSFS) += powerpc/
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..30ef53003c24 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -664,7 +664,7 @@ int efivars_sysfs_init(void)
>  	struct kobject *parent_kobj = efivars_kobject();
>  	int error = 0;
>  
> -	if (!efi_enabled(EFI_RUNTIME_SERVICES))
> +	if (IS_ENABLED(CONFIG_EFI) && !efi_enabled(EFI_RUNTIME_SERVICES))
>  		return -ENODEV;
>  
>  	/* No efivars has been registered yet */

Seems a bit sketch.

> diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
> new file mode 100644
> index 000000000000..e0303fc517d5
> --- /dev/null
> +++ b/drivers/firmware/powerpc/Kconfig
> @@ -0,0 +1,12 @@
> +config POWER_SECVAR_SYSFS
> +	tristate "Enable sysfs interface for POWER secure variables"
> +	default n
> +	depends on PPC_SECURE_BOOT
> +	select UCS2_STRING
> +	help
> +	  POWER secure variables are managed and controlled by OPAL.
> +	  These variables are exposed to userspace via sysfs to allow
> +	  user to read/write these variables. Say Y if you have secure
> +	  boot enabled and want to expose variables to userspace.
> +
> +source "drivers/firmware/efi/Kconfig"

Again, this is all powernv specific and not generic to all powerpc
platforms.

> diff --git a/drivers/firmware/powerpc/Makefile b/drivers/firmware/powerpc/Makefile
> new file mode 100644
> index 000000000000..d5fa3b007315
> --- /dev/null
> +++ b/drivers/firmware/powerpc/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_POWER_SECVAR_SYSFS) += ../efi/efivars.o efi_error.o ../efi/vars.o secvar.o

Uh... That is going to need an Ack from someone in EFI land.

> diff --git a/drivers/firmware/powerpc/efi_error.c b/drivers/firmware/powerpc/efi_error.c
> new file mode 100644
> index 000000000000..b5cabd52e6b4
> --- /dev/null
> +++ b/drivers/firmware/powerpc/efi_error.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * efi_error.c
> + *      - Error codes as understood by efi based tools
> + *      Taken from drivers/firmware/efi/efi.c
> + */
> +#include<linux/efi.h>
> +
> +int efi_status_to_err(efi_status_t status)
> +{
> +	int err;
> +
> +	switch (status) {
> +	case EFI_SUCCESS:
> +		err = 0;
> +		break;
> +	case EFI_INVALID_PARAMETER:
> +		err = -EINVAL;
> +		break;
> +	case EFI_OUT_OF_RESOURCES:
> +		err = -ENOSPC;
> +		break;
> +	case EFI_DEVICE_ERROR:
> +		err = -EIO;
> +		break;
> +	case EFI_WRITE_PROTECTED:
> +		err = -EROFS;
> +		break;
> +	case EFI_SECURITY_VIOLATION:
> +		err = -EACCES;
> +		break;
> +	case EFI_NOT_FOUND:
> +		err = -ENOENT;
> +		break;
> +	case EFI_ABORTED:
> +		err = -EINTR;
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +
> +	return err;
> +}

what calls this?

> diff --git a/drivers/firmware/powerpc/secvar.c b/drivers/firmware/powerpc/secvar.c
> new file mode 100644
> index 000000000000..f1f134a0bb7c
> --- /dev/null
> +++ b/drivers/firmware/powerpc/secvar.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * secvar.c
> + * - wrappers to expose secure variables to userspace
> + */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/spinlock.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/ioctl.h>
> +#include <linux/uaccess.h>
> +#include <linux/kdebug.h>
> +#include <linux/efi.h>
> +#include <linux/module.h>
> +#include <linux/ucs2_string.h>
> +#include <asm/opal-secvar.h>
> +#include <asm/opal.h>
> +
> +static struct efivars efivars;
> +struct kobject *powerpc_kobj;
> +
> +efi_status_t opal_to_efi_status_log(int rc, const char *func_name)
> +{
> +	efi_status_t status;
> +
> +	switch (rc) {
> +	case OPAL_EMPTY:
> +		status = EFI_NOT_FOUND;
> +		break;
> +	case OPAL_HARDWARE:
> +		status = EFI_DEVICE_ERROR;
> +		break;
> +	case OPAL_NO_MEM:
> +		pr_err("%s: No space in the volatile storage\n", func_name);
> +		status = EFI_OUT_OF_RESOURCES;
> +		break;
> +	case OPAL_PARAMETER:
> +		status = EFI_INVALID_PARAMETER;
> +		break;
> +	case OPAL_PARTIAL:
> +		status = EFI_BUFFER_TOO_SMALL;
> +		break;
> +	case OPAL_PERMISSION:
> +		status = EFI_WRITE_PROTECTED;
> +		break;
> +	case OPAL_RESOURCE:
> +		pr_err("%s: No space in the non-volatile storage\n", func_name);
> +		status = EFI_OUT_OF_RESOURCES;
> +		break;
> +	case OPAL_SUCCESS:
> +		status = EFI_SUCCESS;
> +		break;
> +	default:
> +		pr_err("%s: Unknown OPAL error %d\n", func_name, rc);
> +		status = EFI_DEVICE_ERROR;
> +		break;
> +	}
> +
> +	return status;
> +}
> +
> +#define opal_to_efi_status(rc) opal_to_efi_status_log(rc, __func__)
> +
> +static void createkey(efi_char16_t *name, u8 **key, unsigned long *keylen)
> +{
> +	*keylen = ucs2_utf8size(name) + 1;
> +
> +	*key = kzalloc(*keylen, GFP_KERNEL);
> +	if (!*key) {
> +		*keylen = 0;
> +		*key = NULL;
> +		return;
> +	}
> +
> +	ucs2_as_utf8(*key, name, *keylen);
> +}
>
> +static void createmetadata(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> +			   u8 **mdata, unsigned long *mdsize)
> +{
> +	int size = 0;
> +
> +	*mdsize = ucs2_strsize(name, 1024) + sizeof(efi_guid_t) + sizeof(u32);
> +	*mdata = kzalloc(*mdsize, GFP_KERNEL);
> +
> +	memcpy(*mdata, name, ucs2_strsize(name, 1024));
> +	size = ucs2_strsize(name, 1024);
> +
> +	memcpy(*mdata + size, vendor, sizeof(efi_guid_t));
> +	size += sizeof(efi_guid_t);
> +
> +	if (attr != NULL)
> +		memcpy(*mdata + size, attr, sizeof(u32));
> +	else
> +		memset(*mdata + size, 0, sizeof(u32));
> +}

> +static int convert_buffer_to_efi_guid(u8 *buffer, efi_guid_t *guid)
> +{
> +	u32 *a1;
> +	u16 *a2;
> +	u16 *a3;
> +
> +	a1 = kzalloc(4, GFP_KERNEL);
> +	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);

These should be on the stack, They're small and you free them almost
immediately after allocating them.

> +	return 0;
> +}

> +static efi_status_t powerpc_get_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;
> +	unsigned long dsize;
> +	unsigned long namesize;
> +
> +	if (!name)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (!vendor)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (*data_size == 0) {
> +		/* If *data_size is zero, it implies data size is being asked */
> +		createkey(name, &key, &keylen);
> +		rc = opal_get_variable_size(key, keylen, &mdsize, &dsize);
> +		*data_size = dsize;
> +		kfree(key);
> +		return opal_to_efi_status(rc);
> +	}
> +
> +	createkey(name, &key, &keylen);
> +	createmetadata(name, vendor, attr, &metadata, &mdsize);
> +
> +	rc = opal_get_variable(key, keylen, metadata, &mdsize, data, data_size);
> +
> +	if (rc)
> +		return opal_to_efi_status(rc);
> +
> +	if (mdsize > 0) {
> +		namesize = mdsize - sizeof(efi_guid_t) - sizeof(u32);
> +		if (!attr)
> +			return opal_to_efi_status(rc);
> +		memset(attr, 0, sizeof(u32));
> +		memcpy(attr, metadata + namesize + sizeof(efi_guid_t),
> +		       sizeof(u32));
> +		*attr = be32_to_cpu(*attr);
> +	}
> +
> +	kfree(key);
> +	kfree(metadata);
> +
> +	return opal_to_efi_status(rc);
> +}
> +
> +
> +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);
> +
> +	rc = opal_get_next_variable(key, &keylen, keysize);

As far as I can tell the UEFI spec says the key namespace is really
made up of (vendor, key) tuples. This seems broken to me since it's
ignoring the vendor GUID part and treating it purely as an output, so
if we had two vendors (OS vendors maybe?) writing using the same key
name it'll cause problems.

You could fix that by concatenating the vendor GUID and the ucs-2 name
string, then passing that to OPAL as the key. Based on what the gsmi
driver does I don't think we really need to store the attributes either
so the metadata channel can be ditched entirely.

> +	if (rc) {
> +		kfree(key);
> +		return opal_to_efi_status(rc);
> +	}
> +
> +	mdsize = kzalloc(sizeof(unsigned long), GFP_KERNEL);
> +	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);
> +
> +	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;
> +	}
> +
> +	powerpc_kobj = kobject_create_and_add("secvar", firmware_kobj);
> +	if (!powerpc_kobj) {
> +		pr_info("secvar: Failed to create firmware kobj\n");
> +		goto out_err;
> +	}
> +
> +	rc = efivars_register(&efivars, &efivar_ops, powerpc_kobj);
> +	if (rc) {
> +		pr_info("powerpc: Failed to register efivars\n");
> +		return rc;
> +	}
> +
> +	return 0;
> +out_err:
> +	kobject_put(powerpc_kobj);
> +	pr_info("powerpc: failed to load: %d\n", rc);
> +	return rc;
> +}
> +arch_initcall(power_secvar_init);
> +
> +static void __exit power_secvar_exit(void)
> +{
> +	efivars_unregister(&efivars);
> +	kobject_put(powerpc_kobj);
> +}
> +module_exit(power_secvar_exit);
> +
> +MODULE_AUTHOR("Nayna Jain");
> +MODULE_LICENSE("GPL");


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-07-05  6:05   ` Michael Ellerman
@ 2019-07-23 14:35     ` Nayna
  2019-07-24  9:52       ` Oliver O'Halloran
  0 siblings, 1 reply; 9+ messages in thread
From: Nayna @ 2019-07-23 14:35 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev, linux-efi, Oliver O'Halloran
  Cc: Nayna Jain, linux-kernel, linux-integrity, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	George Wilson, Elaine Palmer, Eric Ricther



On 07/05/2019 02:05 AM, Michael Ellerman wrote:
> Hi Nayna,

Hi Michael, Oliver,


>
> Nayna Jain <nayna@linux.ibm.com> writes:
>> 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.
> Which tools? Can you include a log demonstrating how they're used, ie.
> so that I can test the sequence of commands.
>
>> 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.
> Sorry I haven't been able to keep up with all the discussions, but I
> thought the consensus was that pretending to be EFI-like was a bad idea,
> because we don't have actual EFI and we're not implementing an entirely
> compatible scheme to EFI anyway.
>
> Greg suggested just putting the variables in sysfs, why does that not
> work? Matthew mentioned "complex semantics around variable deletion and
> immutability" but do we have to emulate those semantics on powerpc?

Sorry for the delay in the response.

Yes, I agree. The purpose of the v2 version of the patchset was to try 
and quickly address Matthew's concerns. This version of the patchset:
* is not using any EFI configs
* is not exposing secure variables via efivarfs
* is based on Greg's suggestion to use sysfs
* is STILL using some of the existing EFI code, that is used by EFI to 
expose its variables via sysfs, to avoid code duplication.
* is using efivar hooks to expose secure variables for tool compatibility

Assuming we all are in agreement, the next version of this patchset will 
further improve upon these changes. It will refactor some of the sysfs 
code from drivers/firmware/efi that is common to both EFI and POWER.  
Since we do not have to emulate the complex semantics of efi on powerpc, 
the sysfs interface should work for us.

As per the tool, it will be efivar. I will provide the log demonstrating 
how it is used with the next version.

Is there something I missed in my understanding ?

Thanks & Regards,
      - Nayna

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
  2019-07-23 14:35     ` Nayna
@ 2019-07-24  9:52       ` Oliver O'Halloran
  0 siblings, 0 replies; 9+ messages in thread
From: Oliver O'Halloran @ 2019-07-24  9:52 UTC (permalink / raw)
  To: Nayna
  Cc: Michael Ellerman, linuxppc-dev, linux-efi, Nayna Jain,
	Linux Kernel Mailing List, linux-integrity, Paul Mackerras,
	Benjamin Herrenschmidt, Ard Biesheuvel, Jeremy Kerr,
	Matthew Garret, Mimi Zohar, Greg Kroah-Hartman, Claudio Carvalho,
	George Wilson, Elaine Palmer, Eric Ricther

On Wed, Jul 24, 2019 at 12:35 AM Nayna <nayna@linux.vnet.ibm.com> wrote:
>
> On 07/05/2019 02:05 AM, Michael Ellerman wrote:
> > Hi Nayna,
>
> Hi Michael, Oliver,
>
> > Nayna Jain <nayna@linux.ibm.com> writes:
> >> 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.
> > Which tools? Can you include a log demonstrating how they're used, ie.
> > so that I can test the sequence of commands.
> >
> >> 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.
> > Sorry I haven't been able to keep up with all the discussions, but I
> > thought the consensus was that pretending to be EFI-like was a bad idea,
> > because we don't have actual EFI and we're not implementing an entirely
> > compatible scheme to EFI anyway.

My read is the consensus was that pretending to be EFI is a bad idea
unless we're going to behave like EFI.

> > Greg suggested just putting the variables in sysfs, why does that not
> > work? Matthew mentioned "complex semantics around variable deletion and
> > immutability" but do we have to emulate those semantics on powerpc?
>
> Sorry for the delay in the response.
>
> Yes, I agree. The purpose of the v2 version of the patchset was to try
> and quickly address Matthew's concerns. This version of the patchset:

> * is based on Greg's suggestion to use sysfs

As far as I can tell Greg made that suggestion here:

https://lwn.net/ml/linux-fsdevel/20190603072916.GA7545@kroah.com/

Then walked back on that suggestion after Matthew pointed out that
efivars is separate because of the immutability requirement and the
odd update semantics:

https://lwn.net/ml/linux-fsdevel/20190605081301.GA23180@kroah.com/

Considering the whole point of this is to present the same user-facing
interface so shouldn't you be dealing with all the problems that
interface creates?

> * is not using any EFI configs
That's true, but...

> * is not exposing secure variables via efivarfs
> * is STILL using some of the existing EFI code, that is used by EFI to
> expose its variables via sysfs, to avoid code duplication.

We avoid some of the potential problems of selecting CONFIG_EFI and we
gain a bunch of other potential problems since you've hacked the
makefiles to build code that's normally CONFIG_EFI only.

> * is using efivar hooks to expose secure variables for tool compatibility

Here's the real problem. For compatibility with the existing userspace
tooling, which expects UEFI,  you need to present the same interface
with the same semantics. Trying to not use efivarfs means you've
already lost since you no longer have the same interface. So how is
this an improvement? I think the options here are to either:

1) Come up with a new interface, implement it, and adapt the user
tooling to deal with the new API.

*or*

2) Use efivarsfs and fix the based i-cant-believe-its-not-efi variable
backend so it behaves *exactly* like the UEFI get/setVariable APIs.
This means that you need to validate the update certificates at
runtime. I don't think this is a huge strech since you're already
implementing the validator.

1) gives you the flexibility to change the key hierarchy and whatnot,
while 2) means we've got less weird powerpc crap for users to deal
with. I have no strong opinions about which you choose to do, but
don't do this.

Oliver

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-13 20:50 [PATCH 0/2] powerpc/powernv: expose secure variables to userspace Nayna Jain
2019-06-13 20:50 ` [PATCH 1/2] powerpc/powernv: add OPAL APIs for secure variables Nayna Jain
2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
2019-06-14  6:34   ` Greg Kroah-Hartman
2019-06-14 13:13     ` Nayna
2019-07-05  6:05   ` Michael Ellerman
2019-07-23 14:35     ` Nayna
2019-07-24  9:52       ` Oliver O'Halloran
2019-07-22 10:19   ` Oliver O'Halloran

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org linux-efi@archiver.kernel.org
	public-inbox-index linux-efi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox