Re: [PATCH V2 1/2] Abstract out support for locating an EFI config table

2019-06-12 Thread Jarkko Sakkinen
On Mon, Jun 10, 2019 at 10:46:35AM -0700, Matthew Garrett wrote:
> On Mon, Jun 10, 2019 at 9:58 AM Jarkko Sakkinen
>  wrote:
> >
> > On Fri, Jun 07, 2019 at 01:51:46PM -0700, Matthew Garrett wrote:
> > > We want to grab a pointer to the TPM final events table, so abstract out
> > > the existing code for finding an FDT table and make it generic.
> > >
> > > Signed-off-by: Matthew Garrett 
> >
> > Just to clarify are these extensions to what you did before and not
> > something that needs be squashed your commits pipelined for v5.3?
> 
> Correct - they handle a corner case. Ideally they'd hit 5.3 as well,
> but if you'd prefer I'm ok waiting.

Probably makes sense to have them in the PR.

/Jarkko


[[efi boot control]] efibc: Replace variable set function in notifier call

2019-06-12 Thread baofeng . tian
From: Tian Baofeng 

Replace the variable set function from "efivar_entry_set" to
"efivar_entry_set_safe" in efibc panic notifier.
In safe function parameter "block" will set to false
and will call "efivar_entry_set_nonblocking"to set efi variables.
efivar_entry_set_nonblocking is guaranteed to
not block and is suitable for calling from crash/panic handlers.
In UEFI android platform, when warm reset happens,
with this change, efibc will not block the reboot process.
Otherwise, set variable will call queue work and send to other offlined
cpus then cause another panic, finally will cause reboot failure.

Signed-off-by: Tian Baofeng 
Signed-off-by: Luo XinanX 
---
 drivers/firmware/efi/efibc.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 61e099826cbb..35dccc88ac0a 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -43,11 +43,13 @@ static int efibc_set_variable(const char *name, const char 
*value)
efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
memcpy(>var.VendorGuid, , sizeof(guid));
 
-   ret = efivar_entry_set(entry,
-  EFI_VARIABLE_NON_VOLATILE
-  | EFI_VARIABLE_BOOTSERVICE_ACCESS
-  | EFI_VARIABLE_RUNTIME_ACCESS,
-  size, entry->var.Data, NULL);
+   ret = efivar_entry_set_safe(entry->var.VariableName,
+   entry->var.VendorGuid,
+   EFI_VARIABLE_NON_VOLATILE
+   | EFI_VARIABLE_BOOTSERVICE_ACCESS
+   | EFI_VARIABLE_RUNTIME_ACCESS,
+   false, size, entry->var.Data);
+
if (ret)
pr_err("failed to set %s EFI variable: 0x%x\n",
   name, ret);
-- 
2.21.0



[[efi boot control]] efibc: Replace variable set function in notifier call

2019-06-12 Thread baofeng . tian
From: Tian Baofeng 

Replace the variable set function from "efivar_entry_set" to
"efivar_entry_set_safe" in efibc panic notifier.
In safe function parameter "block" will set to false
and will call "efivar_entry_set_nonblocking"to set efi variables.
efivar_entry_set_nonblocking is guaranteed to
not block and is suitable for calling from crash/panic handlers.
In UEFI android platform, when warm reset happens,
with this change, efibc will not block the reboot process.
Otherwise, set variable will call queue work and send to other offlined
cpus then cause another panic, finally will cause reboot failure.

Change-Id: I82d182ef6c036a1427d65d090a97469e2d51a5e1
Signed-off-by: Tian Baofeng 
Signed-off-by: Luo XinanX 
---
 drivers/firmware/efi/efibc.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 61e099826cbb..35dccc88ac0a 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -43,11 +43,13 @@ static int efibc_set_variable(const char *name, const char 
*value)
efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
memcpy(>var.VendorGuid, , sizeof(guid));
 
-   ret = efivar_entry_set(entry,
-  EFI_VARIABLE_NON_VOLATILE
-  | EFI_VARIABLE_BOOTSERVICE_ACCESS
-  | EFI_VARIABLE_RUNTIME_ACCESS,
-  size, entry->var.Data, NULL);
+   ret = efivar_entry_set_safe(entry->var.VariableName,
+   entry->var.VendorGuid,
+   EFI_VARIABLE_NON_VOLATILE
+   | EFI_VARIABLE_BOOTSERVICE_ACCESS
+   | EFI_VARIABLE_RUNTIME_ACCESS,
+   false, size, entry->var.Data);
+
if (ret)
pr_err("failed to set %s EFI variable: 0x%x\n",
   name, ret);
-- 
2.21.0



Re: [PATCH v3 1/3] powerpc/powernv: Add OPAL API interface to get secureboot state

2019-06-12 Thread Daniel Axtens
Nayna Jain  writes:

> From: Claudio Carvalho 
>
> 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.
>
> This patch adds support to read OPAL secure variables through
> OPAL_SECVAR_GET call. It returns the metadata and data for a given secure
> variable based on the unique key.
>
> Since OPAL can support different types of backend which can vary in the
> variable interpretation, a new OPAL API call named OPAL_SECVAR_BACKEND, is
> added to retrieve the supported backend version. This helps the consumer
> to know how to interpret the variable.
>

(Firstly, apologies that I haven't got around to asking about this yet!)

Are pluggable/versioned backend a good idea?

There are a few things that worry me about the idea:

 - It adds complexity in crypto (or crypto-adjacent) code, and that
   increases the likelihood that we'll accidentally add a bug with bad
   consequences.

 - Under what circumstances would would we change the kernel-visible
   behaviour of skiboot? Are we expecting to change the behaviour,
   content or names of the variables in future? Otherwise the only
   relevant change I can think of is a change to hardware platforms, and
   I'm not sure how a change in hardware would lead to change in
   behaviour in the kernel. Wouldn't Skiboot hide h/w differences?

 - If we are worried about a long-term-future change to how secure-boot
   works, would it be better to just add more get/set calls to opal at
   the point at which we actually implement the new system?
   
 - UEFI added EFI_VARIABLE_AUTHENTICATION_3 in a way that - as far
   as I know - didn't break backwards compatibility. Is there a reason
   we cannot add features that way instead? (It also dropped v1 of the
   authentication header.)
   
 - What is the correct fallback behaviour if a kernel receives a result
   that it does not expect? If a kernel expecting BackendV1 is instead
   informed that it is running on BackendV2, then the cannot access the
   secure variable at all, so it cannot load keys that are potentially
   required to successfully boot (e.g. to validate the module for
   network card or graphics!)

Kind regards,
Daniel

> This support can be enabled using CONFIG_OPAL_SECVAR
>
> Signed-off-by: Claudio Carvalho 
> Signed-off-by: Nayna Jain 
> ---
> This patch depends on a new OPAL call that is being added to skiboot.
> The patch set that implements the new call has been posted to
> https://patchwork.ozlabs.org/project/skiboot/list/?series=112868
>
>  arch/powerpc/include/asm/opal-api.h  |  4 +-
>  arch/powerpc/include/asm/opal-secvar.h   | 23 ++
>  arch/powerpc/include/asm/opal.h  |  6 ++
>  arch/powerpc/platforms/powernv/Kconfig   |  6 ++
>  arch/powerpc/platforms/powernv/Makefile  |  1 +
>  arch/powerpc/platforms/powernv/opal-call.c   |  2 +
>  arch/powerpc/platforms/powernv/opal-secvar.c | 85 
>  7 files changed, 126 insertions(+), 1 deletion(-)
>  create mode 100644 arch/powerpc/include/asm/opal-secvar.h
>  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 e1577cfa7186..a505e669b4b6 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -212,7 +212,9 @@
>  #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_BACKEND 177
> +#define OPAL_LAST177
>  
>  #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-secvar.h 
> b/arch/powerpc/include/asm/opal-secvar.h
> new file mode 100644
> index ..b677171a0368
> --- /dev/null
> +++ b/arch/powerpc/include/asm/opal-secvar.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PowerNV definitions for secure variables OPAL API.
> + *
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Claudio Carvalho 
> + *
> + */
> +#ifndef OPAL_SECVAR_H
> +#define OPAL_SECVAR_H
> +
> +enum {
> + BACKEND_NONE = 0,
> + BACKEND_TC_COMPAT_V1,
> +};
> +
> +extern int opal_get_variable(u8 *key, unsigned long ksize,
> +  u8 *metadata, unsigned long *mdsize,
> +  u8 *data, unsigned long *dsize);
> +
> +extern int opal_variable_version(unsigned long *backend);
> +
> +#endif
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 4cc37e708bc7..57d2c2356eda 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++