| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eda9210b56ab220519642d272079eeca60a18265.camel@gmail.com>
Date: Thu, 22 Aug 2019 15:02:32 +1000
From: Oliver O'Halloran <oohall@...il.com>
To: Nayna Jain <nayna@...ux.ibm.com>, linuxppc-dev@...abs.org,
linux-efi@...r.kernel.org, linux-integrity@...r.kernel.org
Cc: linux-kernel@...r.kernel.org,
Michael Ellerman <mpe@...erman.id.au>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Jeremy Kerr <jk@...abs.org>,
Matthew Garret <matthew.garret@...ula.com>,
Mimi Zohar <zohar@...ux.ibm.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Claudio Carvalho <cclaudio@...ux.ibm.com>,
George Wilson <gcwilson@...ux.ibm.com>,
Elaine Palmer <erpalmer@...ibm.com>,
Eric Ricther <erichte@...ux.ibm.com>
Subject: 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 <cclaudio@...ux.ibm.com>
> Signed-off-by: Nayna Jain <nayna@...ux.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.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_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.
> > +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 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@...ux.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;
> +}
> +
> +#endif
The 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.
> +
> +#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 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@...ux.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?
> 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.o
> diff --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@...ux.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
> 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);
Powered by blists - more mailing lists