Re: [PATCH v4 2/4] powerpc: expose secure variables to userspace via sysfs
On Tue, 2019-10-01 at 19:41 -0400, Nayna Jain wrote: > PowerNV secure variables, which store the keys used for OS kernel > verification, are managed by the firmware. These secure variables need to > be accessed by the userspace for addition/deletion of the certificates. > > This patch adds the sysfs interface to expose secure variables for PowerNV > secureboot. The users shall use this interface for manipulating > the keys stored in the secure variables. > > Signed-off-by: Nayna Jain > Reviewed-by: Greg Kroah-Hartman > --- > Documentation/ABI/testing/sysfs-secvar | 37 + > arch/powerpc/Kconfig | 10 ++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/secvar-sysfs.c | 198 + > 4 files changed, 246 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-secvar > create mode 100644 arch/powerpc/kernel/secvar-sysfs.c > > diff --git a/Documentation/ABI/testing/sysfs-secvar > b/Documentation/ABI/testing/sysfs-secvar > new file mode 100644 > index ..815bd8ec4d5e > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-secvar > @@ -0,0 +1,37 @@ > +What:/sys/firmware/secvar > +Date:August 2019 > +Contact: Nayna Jain > +Description: This directory is created if the POWER firmware supports OS > + secureboot, thereby secure variables. It exposes interface > + for reading/writing the secure variables > + > +What:/sys/firmware/secvar/vars > +Date:August 2019 > +Contact: Nayna Jain > +Description: This directory lists all the secure variables that are supported > + by the firmware. > + > +What:/sys/firmware/secvar/vars/ > +Date:August 2019 > +Contact: Nayna Jain > +Description: Each secure variable is represented as a directory named as > + . The variable name is unique and is in ASCII > + representation. The data and size can be determined by reading > + their respective attribute files. > + > +What:/sys/firmware/secvar/vars//size > +Date:August 2019 > +Contact: Nayna Jain > +Description: An integer representation of the size of the content of the > + variable. In other words, it represents the size of the data. > + > +What:/sys/firmware/secvar/vars//data > +Date:August 2019 > +Contact: Nayna Jain > +Description: A read-only file containing the value of the variable > + > +What:/sys/firmware/secvar/vars//update > +Date:August 2019 > +Contact: Nayna Jain > +Description: A write-only file that is used to submit the new value for the > + variable. How are the update mechanism's weird requirements communicated to userspace? The design you've got for the OPAL bits is that the update requirements depends on the secvar backend, but I see nothing plumbing through what the secvar backend actually is. > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index deb19ec6ba3d..89084e4e5054 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -946,6 +946,16 @@ config PPC_SECURE_BOOT > to enable OS secure boot on systems that have firmware support for > it. If in doubt say N. > > +config SECVAR_SYSFS that should probably be PPC_SECVAR_SYSFS since it's PPC specific > + tristate "Enable sysfs interface for POWER secure variables" > + depends on PPC_SECURE_BOOT > + depends on SYSFS > + help > + POWER secure variables are managed and controlled by firmware. > + These variables are exposed to userspace via sysfs to enable > + read/write operations on these variables. Say Y if you have > + secure boot enabled and want to expose variables to userspace. > + > endmenu > > config ISA_DMA_API > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index 3cf26427334f..116a3a5c0557 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -162,6 +162,7 @@ obj-y += ucall.o > endif > > obj-$(CONFIG_PPC_SECURE_BOOT)+= secure_boot.o ima_arch.o secvar-ops.o > +obj-$(CONFIG_SECVAR_SYSFS) += secvar-sysfs.o > > # Disable GCOV, KCOV & sanitizers in odd or sensitive code > GCOV_PROFILE_prom_init.o := n > diff --git a/arch/powerpc/kernel/secvar-sysfs.c > b/arch/powerpc/kernel/secvar-sysfs.c > new file mode 100644 > index ..87a7cea41523 > --- /dev/null > +++ b/arch/powerpc/kernel/secvar-sysfs.c > @@ -0,0 +1,198 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2019 IBM Corporation > + * > + * This code exposes secure variables to user via sysfs > + */ Adding a pr_fmt for this file would be a good idea. There's a few pr_err()s in here that would benefit from some context. > +#include > +#include > +#include > +#include >
Re: [PATCH v4 1/4] powerpc/powernv: Add OPAL API interface to access secure variable
On Tue, 2019-10-01 at 19:41 -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 | 8 ++
> arch/powerpc/include/asm/powernv.h | 2 +
> arch/powerpc/include/asm/secvar.h| 35 +
> arch/powerpc/kernel/Makefile | 2 +-
> arch/powerpc/kernel/secvar-ops.c | 19 +++
> 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 | 137 +++
> arch/powerpc/platforms/powernv/opal.c| 5 +
> 11 files changed, 221 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 378e3997845a..c1f25a760eb1 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -211,7 +211,10 @@
> #define OPAL_MPIPL_UPDATE173
> #define OPAL_MPIPL_REGISTER_TAG 174
> #define OPAL_MPIPL_QUERY_TAG 175
> -#define OPAL_LAST175
> +#define OPAL_SECVAR_GET 176
> +#define OPAL_SECVAR_GET_NEXT 177
> +#define OPAL_SECVAR_ENQUEUE_UPDATE 178
> +#define OPAL_LAST178
>
> #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 a0cf8fba4d12..03392dc3f5e2 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -298,6 +298,13 @@ int opal_sensor_group_clear(u32 group_hndl, int token);
> int opal_sensor_group_enable(u32 group_hndl, int token, bool enable);
> int opal_nx_coproc_init(uint32_t chip_id, uint32_t ct);
>
> +int opal_secvar_get(const char *key, uint64_t key_len, u8 *data,
> + uint64_t *data_size);
> +int opal_secvar_get_next(const char *key, uint64_t *key_len,
> + uint64_t key_buf_size);
> +int opal_secvar_enqueue_update(const char *key, uint64_t key_len, u8 *data,
> +uint64_t data_size);
> +
> s64 opal_mpipl_update(enum opal_mpipl_ops op, u64 src, u64 dest, u64 size);
> s64 opal_mpipl_register_tag(enum opal_mpipl_tags tag, u64 addr);
> s64 opal_mpipl_query_tag(enum opal_mpipl_tags tag, u64 *addr);
> @@ -392,6 +399,7 @@ void opal_wake_poller(void);
> void opal_powercap_init(void);
> void opal_psr_init(void);
> void opal_sensor_groups_init(void);
> +void opal_secvar_init(void);
>
> #endif /* __ASSEMBLY__ */
>
> diff --git a/arch/powerpc/include/asm/powernv.h
> b/arch/powerpc/include/asm/powernv.h
> index e1a858718716..cff980a85dd2 100644
> --- a/arch/powerpc/include/asm/powernv.h
> +++ b/arch/powerpc/include/asm/powernv.h
> @@ -12,10 +12,12 @@ extern void powernv_set_nmmu_ptcr(unsigned long ptcr);
> void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val);
>
> void pnv_tm_init(void);
> +
> #else
> static inline void powernv_set_nmmu_ptcr(unsigned long ptcr) { }
>
> static inline void pnv_tm_init(void) { }
> +
> #endif
>
> #endif /* _ASM_POWERNV_H */
> diff --git a/arch/powerpc/include/asm/secvar.h
> b/arch/powerpc/include/asm/secvar.h
> new file mode 100644
> index ..4cc35b58b986
> --- /dev/null
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + *
> + * PowerPC secure variable operations.
> + */
> +#ifndef SECVAR_OPS_H
> +#define SECVAR_OPS_H
> +
> +#include
> +#include
> +
> +extern const struct secvar_operations *secvar_ops;
> +
> +struct secvar_operations {
> + int (*get)(const char *key, uint64_t key_len, u8 *data,
> +uint64_t *data_size);
> + int (*get_next)(const char *key, uint64_t *key_len,
> + uint64_t keybufsize);
> + int (*set)(const char *key, uint64_t key_len, u8 *data,
> +uint64_t data_size);
> +};
> +
> +#ifdef CONFIG_PPC_SECURE_BOOT
> +
> +extern void set_secvar_ops(const struct secvar_operations *ops);
> +
> +#else
> +
> +static inline void
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 2/4] powerpc: expose secure variables to userspace via sysfs
On Wed, 2019-08-21 at 11:08 -0400, Nayna Jain wrote: > PowerNV secure variables, which store the keys used for OS kernel > verification, are managed by the firmware. These secure variables need to > be accessed by the userspace for addition/deletion of the certificates. > > This patch adds the sysfs interface to expose secure variables for PowerNV > secureboot. The users shall use this interface for manipulating > the keys stored in the secure variables. > > Signed-off-by: Nayna Jain > --- > Documentation/ABI/testing/sysfs-secvar | 27 > arch/powerpc/Kconfig | 9 ++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/secvar-sysfs.c | 210 + > 4 files changed, 247 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-secvar > create mode 100644 arch/powerpc/kernel/secvar-sysfs.c > > diff --git a/Documentation/ABI/testing/sysfs-secvar > b/Documentation/ABI/testing/sysfs-secvar > new file mode 100644 > index ..68f0e03d873d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-secvar > @@ -0,0 +1,27 @@ > +What:/sys/firmware/secvar > +Date:August 2019 > +Contact: Nayna Jain > +Description: > + This directory exposes interfaces for interacting with > + the secure variables managed by OPAL firmware. > + > + This is only for the powerpc/powernv platform. > + > + Directory: > + vars: This directory lists all the variables that > + are supported by the OPAL. The variables are > + represented in the form of directories with > + their variable names. The variable name is > + unique and is in ASCII representation. The data > + and size can be determined by reading their > + respective attribute files. > + > + Each variable directory has the following files: > + name: An ASCII representation of the variable name > + data: A read-only file containing the value of the > + variable > + size: An integer representation of the size of the > + content of the variable. In other works, it > + represents the size of the data > + update: A write-only file that is used to submit the new > + value for the variable. > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 42109682b727..b4bdf77837b2 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -925,6 +925,15 @@ config PPC_SECURE_BOOT > allows user to enable OS Secure Boot on PowerPC systems that > have firmware secure boot support. > > +config SECVAR_SYSFS > +tristate "Enable sysfs interface for POWER secure variables" > +depends on PPC_SECURE_BOOT > +help > + POWER secure variables are managed and controlled by firmware. > + These variables are exposed to userspace via sysfs to enable > + read/write operations on these variables. Say Y if you have > + secure boot enabled and want to expose variables to userspace. > + > endmenu > > config ISA_DMA_API > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index 9041563f1c74..4ea7b738c3a3 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -158,6 +158,7 @@ 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 secvar-ops.o > +obj-$(CONFIG_SECVAR_SYSFS) += secvar-sysfs.o > > # Disable GCOV, KCOV & sanitizers in odd or sensitive code > GCOV_PROFILE_prom_init.o := n > diff --git a/arch/powerpc/kernel/secvar-sysfs.c > b/arch/powerpc/kernel/secvar-sysfs.c > new file mode 100644 > index ..e46986bb29a0 > --- /dev/null > +++ b/arch/powerpc/kernel/secvar-sysfs.c > @@ -0,0 +1,210 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2019 IBM Corporation > + * > + * This code exposes secure variables to user via sysfs > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +//Approximating it for now, it is bound to change. > +#define VARIABLE_MAX_SIZE 32000 this needs to be communicated from the secvar backend, maybe via a field in the ops structure? > + > +static struct kobject *powerpc_kobj; Call it secvar_kobj or something. > +static struct secvar_operations *secvarops; Ah, the old I-can't-believe-it's-not-global trick. > +struct kset *secvar_kset; shouldn't that be static too? > + > +static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr, > +
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
>
Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
On Wed, Jul 24, 2019 at 12:35 AM Nayna wrote: > > On 07/05/2019 02:05 AM, Michael Ellerman wrote: > > Hi Nayna, > > Hi Michael, Oliver, > > > Nayna Jain 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.ga7...@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.ga23...@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
Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
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 > --- > 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 ..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 ..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 ..b5cabd52e6b4 > --- /dev/null > +++
