Re: [PATCH v2 1/4] powerpc/powernv: Add OPAL API interface to access secure variable
On Thu, Aug 22, 2019 at 3:02 PM Oliver O'Halloran wrote: > > On Wed, 2019-08-21 at 11:08 -0400, Nayna Jain wrote: > > 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 > > #include > > #include > > +#include > > +#include > > > > #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? To answer my own question, yes it depends on the series at [1] which adds IMA support. Turns out actually reading the cover letter helps, who knew. That said, I'm still not entirely sure about this. The implementation of is_powerpc_secvar_supported() in [2] parses the DT and seems to assume the DT bindings that OPAL produces. Are those common with the DT bindings produced by OF when running on pseries? [1] http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=125961 [2] http://patchwork.ozlabs.org/patch/1149257/ > > > return 0; > > } > > machine_subsys_initcall(powernv, opal_init); >
Re: [PATCH v2 1/4] powerpc/powernv: Add OPAL API interface to access secure variable
On Wed, 2019-08-21 at 11:08 -0400, Nayna Jain wrote:
> 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
> Signed-off-by: Nayna Jain
> ---
> 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.c
>
> diff --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_LAST170
> +#define OPAL_SECVAR_GET 173
> +#define OPAL_SECVAR_GET_NEXT174
> +#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.
> > +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.
> #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 ..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
> + *
> + */
> +#ifndef SECVAR_OPS_H
> +#define SECVAR_OPS_H
> +
> +#include
> +#include
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
>
[PATCH v2 1/4] powerpc/powernv: Add OPAL API interface to access secure variable
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
Signed-off-by: Nayna Jain
---
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.c
diff --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
#defineOPAL_NX_COPROC_INIT 167
#define OPAL_XIVE_GET_VP_STATE 170
-#define OPAL_LAST 170
+#define OPAL_SECVAR_GET 173
+#define OPAL_SECVAR_GET_NEXT174
+#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);
+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);
#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 ..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
+ *
+ */
+#ifndef SECVAR_OPS_H
+#define SECVAR_OPS_H
+
+#include
+#include
+
+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);
+};
+
+#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;
+}
+
+#endif
+
+#endif
diff --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 := n
diff --git a/arch/powerpc/kernel/secvar-ops.c b/arch/powerpc/kernel/secvar-ops.c
new file mode 100644
index ..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
