* [PATCH] efi: libstub: check Shim mode using MokSBStateRT
@ 2022-09-20 15:37 Ard Biesheuvel
2022-09-21 15:52 ` Ilias Apalodimas
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2022-09-20 15:37 UTC (permalink / raw)
To: linux-efi; +Cc: pjones, ilias.apalodimas, Ard Biesheuvel
We currently check the MokSBState variable to decide whether we should
treat UEFI secure boot as being disabled, even if the firmware thinks
otherwise. This is used by shim to indicate that it is not checking
signatures on boot images. In the kernel, we use this to relax lockdown
policies.
However, in cases where shim is not even being used, we don't want this
variable to interfere with lockdown, given that the variable is
non-volatile variable and therefore persists across a reboot. This means
setting it once will persistently disable lockdown checks on a given
system.
So switch to the mirrored version of this variable, called MokSBStateRT,
which is supposed to be volatile, and this is something we can check.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/xen/efi.c | 5 +++--
drivers/firmware/efi/libstub/secureboot.c | 8 ++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 7d7ffb9c826a..8bd65f2900b9 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -100,6 +100,7 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
enum efi_secureboot_mode mode;
efi_status_t status;
u8 moksbstate;
+ u32 attr;
unsigned long size;
mode = efi_get_secureboot_mode(efi.get_variable);
@@ -113,13 +114,13 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
/* See if a user has put the shim into insecure mode. */
size = sizeof(moksbstate);
status = efi.get_variable(L"MokSBStateRT", &shim_guid,
- NULL, &size, &moksbstate);
+ &attr, &size, &moksbstate);
/* If it fails, we don't care why. Default to secure. */
if (status != EFI_SUCCESS)
goto secure_boot_enabled;
- if (moksbstate == 1)
+ if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
return efi_secureboot_mode_disabled;
secure_boot_enabled:
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 8a18930f3eb6..516f4f0069bd 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -14,7 +14,7 @@
/* SHIM variables */
static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
-static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
+static const efi_char16_t shim_MokSBState_name[] = L"MokSBStateRT";
static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
unsigned long *data_size, void *data)
@@ -43,8 +43,8 @@ enum efi_secureboot_mode efi_get_secureboot(void)
/*
* See if a user has put the shim into insecure mode. If so, and if the
- * variable doesn't have the runtime attribute set, we might as well
- * honor that.
+ * variable doesn't have the non-volatile attribute set, we might as
+ * well honor that.
*/
size = sizeof(moksbstate);
status = get_efi_var(shim_MokSBState_name, &shim_guid,
@@ -53,7 +53,7 @@ enum efi_secureboot_mode efi_get_secureboot(void)
/* If it fails, we don't care why. Default to secure */
if (status != EFI_SUCCESS)
goto secure_boot_enabled;
- if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
+ if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
return efi_secureboot_mode_disabled;
secure_boot_enabled:
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: libstub: check Shim mode using MokSBStateRT
2022-09-20 15:37 [PATCH] efi: libstub: check Shim mode using MokSBStateRT Ard Biesheuvel
@ 2022-09-21 15:52 ` Ilias Apalodimas
2022-09-21 19:10 ` Peter Jones
0 siblings, 1 reply; 4+ messages in thread
From: Ilias Apalodimas @ 2022-09-21 15:52 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, pjones
Hi Ard
On Tue, 20 Sept 2022 at 17:37, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> We currently check the MokSBState variable to decide whether we should
> treat UEFI secure boot as being disabled, even if the firmware thinks
> otherwise. This is used by shim to indicate that it is not checking
> signatures on boot images. In the kernel, we use this to relax lockdown
> policies.
>
> However, in cases where shim is not even being used, we don't want this
> variable to interfere with lockdown, given that the variable is
> non-volatile variable and therefore persists across a reboot. This means
> setting it once will persistently disable lockdown checks on a given
> system.
>
> So switch to the mirrored version of this variable, called MokSBStateRT,
> which is supposed to be volatile, and this is something we can check.
>
Just out of curiosity was the mirroring implemented at the same time
in SHIM? IOW does MokSBState guarantee the presence of the -RT?
Regardless of the answer this fixes an actual problem, so fwiw
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> arch/x86/xen/efi.c | 5 +++--
> drivers/firmware/efi/libstub/secureboot.c | 8 ++++----
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
> index 7d7ffb9c826a..8bd65f2900b9 100644
> --- a/arch/x86/xen/efi.c
> +++ b/arch/x86/xen/efi.c
> @@ -100,6 +100,7 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> enum efi_secureboot_mode mode;
> efi_status_t status;
> u8 moksbstate;
> + u32 attr;
> unsigned long size;
>
> mode = efi_get_secureboot_mode(efi.get_variable);
> @@ -113,13 +114,13 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> /* See if a user has put the shim into insecure mode. */
> size = sizeof(moksbstate);
> status = efi.get_variable(L"MokSBStateRT", &shim_guid,
> - NULL, &size, &moksbstate);
> + &attr, &size, &moksbstate);
>
> /* If it fails, we don't care why. Default to secure. */
> if (status != EFI_SUCCESS)
> goto secure_boot_enabled;
>
> - if (moksbstate == 1)
> + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> return efi_secureboot_mode_disabled;
>
> secure_boot_enabled:
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index 8a18930f3eb6..516f4f0069bd 100644
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -14,7 +14,7 @@
>
> /* SHIM variables */
> static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
> +static const efi_char16_t shim_MokSBState_name[] = L"MokSBStateRT";
>
> static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> unsigned long *data_size, void *data)
> @@ -43,8 +43,8 @@ enum efi_secureboot_mode efi_get_secureboot(void)
>
> /*
> * See if a user has put the shim into insecure mode. If so, and if the
> - * variable doesn't have the runtime attribute set, we might as well
> - * honor that.
> + * variable doesn't have the non-volatile attribute set, we might as
> + * well honor that.
> */
> size = sizeof(moksbstate);
> status = get_efi_var(shim_MokSBState_name, &shim_guid,
> @@ -53,7 +53,7 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> /* If it fails, we don't care why. Default to secure */
> if (status != EFI_SUCCESS)
> goto secure_boot_enabled;
> - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
> + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> return efi_secureboot_mode_disabled;
>
> secure_boot_enabled:
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: libstub: check Shim mode using MokSBStateRT
2022-09-21 15:52 ` Ilias Apalodimas
@ 2022-09-21 19:10 ` Peter Jones
2022-09-22 7:47 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Peter Jones @ 2022-09-21 19:10 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: Ard Biesheuvel, linux-efi
On Wed, Sep 21, 2022 at 05:52:31PM +0200, Ilias Apalodimas wrote:
> Hi Ard
>
> On Tue, 20 Sept 2022 at 17:37, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > We currently check the MokSBState variable to decide whether we should
> > treat UEFI secure boot as being disabled, even if the firmware thinks
> > otherwise. This is used by shim to indicate that it is not checking
> > signatures on boot images. In the kernel, we use this to relax lockdown
> > policies.
> >
> > However, in cases where shim is not even being used, we don't want this
> > variable to interfere with lockdown, given that the variable is
> > non-volatile variable and therefore persists across a reboot. This means
> > setting it once will persistently disable lockdown checks on a given
> > system.
> >
> > So switch to the mirrored version of this variable, called MokSBStateRT,
> > which is supposed to be volatile, and this is something we can check.
> >
>
> Just out of curiosity was the mirroring implemented at the same time
> in SHIM? IOW does MokSBState guarantee the presence of the -RT?
> Regardless of the answer this fixes an actual problem, so fwiw
Yes, since 2016.
Reviewed-by: Peter Jones <pjones@redhat.com>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > arch/x86/xen/efi.c | 5 +++--
> > drivers/firmware/efi/libstub/secureboot.c | 8 ++++----
> > 2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
> > index 7d7ffb9c826a..8bd65f2900b9 100644
> > --- a/arch/x86/xen/efi.c
> > +++ b/arch/x86/xen/efi.c
> > @@ -100,6 +100,7 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> > enum efi_secureboot_mode mode;
> > efi_status_t status;
> > u8 moksbstate;
> > + u32 attr;
> > unsigned long size;
> >
> > mode = efi_get_secureboot_mode(efi.get_variable);
> > @@ -113,13 +114,13 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> > /* See if a user has put the shim into insecure mode. */
> > size = sizeof(moksbstate);
> > status = efi.get_variable(L"MokSBStateRT", &shim_guid,
> > - NULL, &size, &moksbstate);
> > + &attr, &size, &moksbstate);
> >
> > /* If it fails, we don't care why. Default to secure. */
> > if (status != EFI_SUCCESS)
> > goto secure_boot_enabled;
> >
> > - if (moksbstate == 1)
> > + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> > return efi_secureboot_mode_disabled;
> >
> > secure_boot_enabled:
> > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> > index 8a18930f3eb6..516f4f0069bd 100644
> > --- a/drivers/firmware/efi/libstub/secureboot.c
> > +++ b/drivers/firmware/efi/libstub/secureboot.c
> > @@ -14,7 +14,7 @@
> >
> > /* SHIM variables */
> > static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> > -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
> > +static const efi_char16_t shim_MokSBState_name[] = L"MokSBStateRT";
> >
> > static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> > unsigned long *data_size, void *data)
> > @@ -43,8 +43,8 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> >
> > /*
> > * See if a user has put the shim into insecure mode. If so, and if the
> > - * variable doesn't have the runtime attribute set, we might as well
> > - * honor that.
> > + * variable doesn't have the non-volatile attribute set, we might as
> > + * well honor that.
> > */
> > size = sizeof(moksbstate);
> > status = get_efi_var(shim_MokSBState_name, &shim_guid,
> > @@ -53,7 +53,7 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> > /* If it fails, we don't care why. Default to secure */
> > if (status != EFI_SUCCESS)
> > goto secure_boot_enabled;
> > - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
> > + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> > return efi_secureboot_mode_disabled;
> >
> > secure_boot_enabled:
> > --
> > 2.35.1
> >
>
--
Peter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: libstub: check Shim mode using MokSBStateRT
2022-09-21 19:10 ` Peter Jones
@ 2022-09-22 7:47 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2022-09-22 7:47 UTC (permalink / raw)
To: Peter Jones; +Cc: Ilias Apalodimas, linux-efi
On Wed, 21 Sept 2022 at 21:10, Peter Jones <pjones@redhat.com> wrote:
>
> On Wed, Sep 21, 2022 at 05:52:31PM +0200, Ilias Apalodimas wrote:
> > Hi Ard
> >
> > On Tue, 20 Sept 2022 at 17:37, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > We currently check the MokSBState variable to decide whether we should
> > > treat UEFI secure boot as being disabled, even if the firmware thinks
> > > otherwise. This is used by shim to indicate that it is not checking
> > > signatures on boot images. In the kernel, we use this to relax lockdown
> > > policies.
> > >
> > > However, in cases where shim is not even being used, we don't want this
> > > variable to interfere with lockdown, given that the variable is
> > > non-volatile variable and therefore persists across a reboot. This means
> > > setting it once will persistently disable lockdown checks on a given
> > > system.
> > >
> > > So switch to the mirrored version of this variable, called MokSBStateRT,
> > > which is supposed to be volatile, and this is something we can check.
> > >
> >
> > Just out of curiosity was the mirroring implemented at the same time
> > in SHIM? IOW does MokSBState guarantee the presence of the -RT?
> > Regardless of the answer this fixes an actual problem, so fwiw
>
> Yes, since 2016.
>
> Reviewed-by: Peter Jones <pjones@redhat.com>
>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
Thanks, I've queued this as a fix.
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > arch/x86/xen/efi.c | 5 +++--
> > > drivers/firmware/efi/libstub/secureboot.c | 8 ++++----
> > > 2 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
> > > index 7d7ffb9c826a..8bd65f2900b9 100644
> > > --- a/arch/x86/xen/efi.c
> > > +++ b/arch/x86/xen/efi.c
> > > @@ -100,6 +100,7 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> > > enum efi_secureboot_mode mode;
> > > efi_status_t status;
> > > u8 moksbstate;
> > > + u32 attr;
> > > unsigned long size;
> > >
> > > mode = efi_get_secureboot_mode(efi.get_variable);
> > > @@ -113,13 +114,13 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> > > /* See if a user has put the shim into insecure mode. */
> > > size = sizeof(moksbstate);
> > > status = efi.get_variable(L"MokSBStateRT", &shim_guid,
> > > - NULL, &size, &moksbstate);
> > > + &attr, &size, &moksbstate);
> > >
> > > /* If it fails, we don't care why. Default to secure. */
> > > if (status != EFI_SUCCESS)
> > > goto secure_boot_enabled;
> > >
> > > - if (moksbstate == 1)
> > > + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> > > return efi_secureboot_mode_disabled;
> > >
> > > secure_boot_enabled:
> > > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> > > index 8a18930f3eb6..516f4f0069bd 100644
> > > --- a/drivers/firmware/efi/libstub/secureboot.c
> > > +++ b/drivers/firmware/efi/libstub/secureboot.c
> > > @@ -14,7 +14,7 @@
> > >
> > > /* SHIM variables */
> > > static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> > > -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
> > > +static const efi_char16_t shim_MokSBState_name[] = L"MokSBStateRT";
> > >
> > > static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> > > unsigned long *data_size, void *data)
> > > @@ -43,8 +43,8 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> > >
> > > /*
> > > * See if a user has put the shim into insecure mode. If so, and if the
> > > - * variable doesn't have the runtime attribute set, we might as well
> > > - * honor that.
> > > + * variable doesn't have the non-volatile attribute set, we might as
> > > + * well honor that.
> > > */
> > > size = sizeof(moksbstate);
> > > status = get_efi_var(shim_MokSBState_name, &shim_guid,
> > > @@ -53,7 +53,7 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> > > /* If it fails, we don't care why. Default to secure */
> > > if (status != EFI_SUCCESS)
> > > goto secure_boot_enabled;
> > > - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
> > > + if (!(attr & EFI_VARIABLE_NON_VOLATILE) && moksbstate == 1)
> > > return efi_secureboot_mode_disabled;
> > >
> > > secure_boot_enabled:
> > > --
> > > 2.35.1
> > >
> >
>
> --
> Peter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-22 7:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 15:37 [PATCH] efi: libstub: check Shim mode using MokSBStateRT Ard Biesheuvel
2022-09-21 15:52 ` Ilias Apalodimas
2022-09-21 19:10 ` Peter Jones
2022-09-22 7:47 ` Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.