Re: [PATCH v2 1/4] powerpc/powernv: Add OPAL API interface to access secure variable
From: Oliver O'Halloran <oohall@gmail.com>
Date: 2019-08-22 05:02:44
Also in:
linux-efi, linux-integrity, lkml
On Wed, 2019-08-21 at 11:08 -0400, Nayna Jain wrote:
quoted hunk ↗ jump to hunk
The X.509 certificates trusted by the platform and required to secure boot the OS kernel are wrapped in secure variables, which are controlled by OPAL. This patch adds firmware/kernel interface to read and write OPAL secure variables based on the unique key. This support can be enabled using CONFIG_OPAL_SECVAR. Signed-off-by: Claudio Carvalho <redacted> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> --- arch/powerpc/include/asm/opal-api.h | 5 +- arch/powerpc/include/asm/opal.h | 6 ++ arch/powerpc/include/asm/secvar.h | 55 ++++++++++ arch/powerpc/kernel/Makefile | 2 +- arch/powerpc/kernel/secvar-ops.c | 25 +++++ arch/powerpc/platforms/powernv/Kconfig | 6 ++ arch/powerpc/platforms/powernv/Makefile | 1 + arch/powerpc/platforms/powernv/opal-call.c | 3 + arch/powerpc/platforms/powernv/opal-secvar.c | 102 +++++++++++++++++++ arch/powerpc/platforms/powernv/opal.c | 5 + 10 files changed, 208 insertions(+), 2 deletions(-) create mode 100644 arch/powerpc/include/asm/secvar.h create mode 100644 arch/powerpc/kernel/secvar-ops.c create mode 100644 arch/powerpc/platforms/powernv/opal-secvar.cdiff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h index 383242eb0dea..b238b4f26c5b 100644 --- a/arch/powerpc/include/asm/opal-api.h +++ b/arch/powerpc/include/asm/opal-api.h@@ -208,7 +208,10 @@ #define OPAL_HANDLE_HMI2 166 #define OPAL_NX_COPROC_INIT 167 #define OPAL_XIVE_GET_VP_STATE 170 -#define OPAL_LAST 170 +#define OPAL_SECVAR_GET 173 +#define OPAL_SECVAR_GET_NEXT 174 +#define OPAL_SECVAR_ENQUEUE_UPDATE 175 +#define OPAL_LAST 175 #define QUIESCE_HOLD 1 /* Spin all calls at entry */ #define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h index 57bd029c715e..247adec2375f 100644 --- a/arch/powerpc/include/asm/opal.h +++ b/arch/powerpc/include/asm/opal.h@@ -388,6 +388,12 @@ void opal_powercap_init(void); void opal_psr_init(void); void opal_sensor_groups_init(void);
Put these with the rest of the OPAL API wrapper prototypes (i.e. just after opal_nx_coproc_init()) rather than with the internal functions.
quoted
+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_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_data, uint64_t k_data_size);
Everything in asm/opal.h is intended for consumption by the kernel, so use a useful kernel type (or annotation) rather than blank uint64_t for the parameters that are actually pointers. You should also ditch the k_ prefix since it doesn't make much sense having it inside the kernel. As a general comment, don't use extern on function prototypes. They're extern by default and, more importantly, it's contrary to the normal kernel style.
quoted hunk ↗ jump to hunk
#endif /* __ASSEMBLY__ */ #endif /* _ASM_POWERPC_OPAL_H */diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h new file mode 100644 index 000000000000..645654456265 --- /dev/null +++ b/arch/powerpc/include/asm/secvar.h@@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * PowerPC secure variable operations. + * + * Copyright (C) 2019 IBM Corporation + * Author: Nayna Jain <nayna@linux.ibm.com> + * + */ +#ifndef SECVAR_OPS_H +#define SECVAR_OPS_H + +#include<linux/types.h> +#include<linux/errno.h>
missing space
+
+struct secvar_operations {
+ int (*get_variable)(const char *key, unsigned long key_len, u8 *data,
+ unsigned long *data_size);
+ int (*get_next_variable)(const char *key, unsigned long *key_len,
+ unsigned long keysize);
+ int (*set_variable)(const char *key, unsigned long key_len, u8 *data,
+ unsigned long data_size);
+};Calling them requires writing code like: secvar_ops->get_variable(blah); Why not shorten it to: secvar_ops->get(blah);
+#ifdef CONFIG_PPC_SECURE_BOOT
+
+extern void set_secvar_ops(struct secvar_operations *ops);
+extern struct secvar_operations *get_secvar_ops(void);
+
+#else
+
+static inline void set_secvar_ops(struct secvar_operations *ops)
+{
+}
+
+static inline struct secvar_operations *get_secvar_ops(void)
+{
+ return NULL;
+}
+
+#endif
+#ifdef CONFIG_OPAL_SECVAR
+
+extern int secvar_init(void);
+
+#else
+
+static inline int secvar_init(void)
+{
+ return -EINVAL;
+}
+
+#endifThe init function is always going to be platform specific so having an opal-specific secvar_init() in a generic header doesn't make much sense to me.
quoted hunk ↗ jump to hunk
+ +#endifdiff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 520b1c814197..9041563f1c74 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile@@ -157,7 +157,7 @@ endif obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o -obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o +obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o secvar-ops.o # Disable GCOV, KCOV & sanitizers in odd or sensitive code GCOV_PROFILE_prom_init.o := ndiff --git a/arch/powerpc/kernel/secvar-ops.c b/arch/powerpc/kernel/secvar-ops.c new file mode 100644 index 000000000000..198222499848 --- /dev/null +++ b/arch/powerpc/kernel/secvar-ops.c@@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 IBM Corporation + * Author: Nayna Jain <nayna@linux.ibm.com> + * + * secvar-ops.c + * - initialize secvar operations for PowerPC Secureboot + */ +
+#include<stddef.h> +#include<asm/secvar.h>
missing space
+ +static struct secvar_operations *secvars_ops;
should be const
+
+void set_secvar_ops(struct secvar_operations *ops)
+{
+ if (!ops)
+ secvars_ops = NULL;
+ secvars_ops = ops;
+}
+
+struct secvar_operations *get_secvar_ops(void)
+{
+ return secvars_ops;
+}Is this really any better than just making the ops pointer global?
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig index 850eee860cf2..65b060539b5c 100644 --- a/arch/powerpc/platforms/powernv/Kconfig +++ b/arch/powerpc/platforms/powernv/Kconfig@@ -47,3 +47,9 @@ config PPC_VAS VAS adapters are found in POWER9 based systems. If unsure, say N. + +config OPAL_SECVAR + bool "OPAL Secure Variables" + depends on PPC_POWERNV + help + This enables the kernel to access OPAL secure variables.diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile index da2e99efbd04..6651c742e530 100644 --- a/arch/powerpc/platforms/powernv/Makefile +++ b/arch/powerpc/platforms/powernv/Makefile@@ -16,3 +16,4 @@ obj-$(CONFIG_PERF_EVENTS) += opal-imc.o obj-$(CONFIG_PPC_MEMTRACE) += memtrace.o obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o obj-$(CONFIG_OCXL_BASE) += ocxl.o +obj-$(CONFIG_OPAL_SECVAR) += opal-secvar.odiff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c index 29ca523c1c79..93106e867924 100644 --- a/arch/powerpc/platforms/powernv/opal-call.c +++ b/arch/powerpc/platforms/powernv/opal-call.c@@ -287,3 +287,6 @@ OPAL_CALL(opal_pci_set_pbcq_tunnel_bar, OPAL_PCI_SET_PBCQ_TUNNEL_BAR); OPAL_CALL(opal_sensor_read_u64, OPAL_SENSOR_READ_U64); 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_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 new file mode 100644 index 000000000000..b0f97cea7675 --- /dev/null +++ b/arch/powerpc/platforms/powernv/opal-secvar.c@@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PowerNV code for secure variables + * + * Copyright (C) 2019 IBM Corporation + * Author: Claudio Carvalho <cclaudio@linux.ibm.com> + * + * APIs to access secure variables managed by OPAL. + * + */ + +#define pr_fmt(fmt) "secvar: "fmt + +#include <linux/types.h> +#include <asm/opal.h> +#include <asm/secvar.h> + +static bool is_opal_secvar_supported(void) +{ + static bool opal_secvar_supported; + static bool initialized; + + if (initialized) + return opal_secvar_supported; + + if (!opal_check_token(OPAL_SECVAR_GET) + || !opal_check_token(OPAL_SECVAR_GET_NEXT) + || !opal_check_token(OPAL_SECVAR_ENQUEUE_UPDATE)) {
+ pr_err("OPAL doesn't support secure variables\n");This should only print an error if OPAL has advertised support for secure variables through the DT, but doesn't support the OPAL calls. Otherwise we'll get a spurious error message on any system running currently released firmware.
+ opal_secvar_supported = false;
+ } else {
+ opal_secvar_supported = true;
+ }
+
+ initialized = true;
+
+ return opal_secvar_supported;
+}
+
+static int opal_get_variable(const char *key, unsigned long ksize,
+ u8 *data, unsigned long *dsize)
+{
+ int rc;
+
+ if (!is_opal_secvar_supported())
+ return OPAL_UNSUPPORTED;This should be -ENXIO or -ENOSUPP. OPAL_UNSUPPORTED is an OPAL return code, not a kernel one. That said, if the firmware doesn't support secure variables we should never be calling this function anyway since the ops pointer is never set.
+
+ if (dsize)
+ *dsize = cpu_to_be64(*dsize);
+
+ rc = opal_secvar_get(__pa(key), ksize,
+ __pa(data), __pa(dsize));
+
+ if (dsize)
+ *dsize = be64_to_cpu(*dsize);
+
+ return rc;
+}
+
+static int opal_get_next_variable(const char *key, unsigned long *keylen,
+ unsigned long keysize)
+{
+ int rc;
+
+ if (!is_opal_secvar_supported())
+ return OPAL_UNSUPPORTED;
+
+ if (keylen)
+ *keylen = cpu_to_be64(*keylen);
+
+ rc = opal_secvar_get_next(__pa(key), __pa(keylen), keysize);
+
+ if (keylen)
+ *keylen = be64_to_cpu(*keylen);
+
+ return rc;
+}
+
+static int opal_set_variable(const char *key, unsigned long ksize, u8 *data,
+ unsigned long dsize)
+{
+ int rc;
+
+ if (!is_opal_secvar_supported())
+ return OPAL_UNSUPPORTED;
+
+ rc = opal_secvar_enqueue_update(__pa(key), ksize, __pa(data), dsize);
+
+ return rc;
+}
++static struct secvar_operations secvar_ops = {
+ .get_variable = opal_get_variable,
+ .get_next_variable = opal_get_next_variable,
+ .set_variable = opal_set_variable,
+};should be const
+int secvar_init(void)
+{
+ set_secvar_ops(&secvar_ops);
+ return 0;
+}Rename this to opal_secvar_init() and put the prototype in arch/powerpc/platforms/powernv/powernv.h
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c index aba443be7daa..ffe6f1cf0830 100644 --- a/arch/powerpc/platforms/powernv/opal.c +++ b/arch/powerpc/platforms/powernv/opal.c@@ -32,6 +32,8 @@ #include <asm/mce.h> #include <asm/imc-pmu.h> #include <asm/bug.h> +#include <asm/secvar.h> +#include <asm/secboot.h> #include "powernv.h"@@ -988,6 +990,9 @@ static int __init opal_init(void) /* Initialise OPAL Power control interface */ opal_power_control_init(); + if (is_powerpc_secvar_supported()) + secvar_init(); +
The usual pattern here is to have the init function check for support internally. Also, is_powerpc_secvar_supported() doesn't appear to be defined anywhere. Is that supposed to be is_opal_secvar_supported()? Or is this series supposed to be applied on top of another series?
return 0; } machine_subsys_initcall(powernv, opal_init);