[6/7] efi: Handle secure boot from UEFI-2.6 [ver #7]

Commit Message

David Howells Jan. 31, 2017, 3:14 p.m. UTC

UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
if we're to engage lockdown mode.

Reported-by: James Bottomley <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
---

 drivers/firmware/efi/libstub/secureboot.c |   16 +++++++++++++++-
 include/linux/efi.h                       |    4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ard Biesheuvel Jan. 31, 2017, 6:19 p.m. UTC | #1

On 31 January 2017 at 15:14, David Howells <[e-mail]> wrote:
> UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
> if we're to engage lockdown mode.
>
> Reported-by: James Bottomley <[e-mail]>
> Signed-off-by: David Howells <[e-mail]>

Interestingly, the string 'DeployedMode' appears zero times in the
EDK2 codebase, so I wonder if it makes any sense to merge this now.
The string 'AuditMode' does appear once, but in a comment

In any case, the logic is not entirely correct either: apologies if it
was me who caused any confusion here, but it seems DeployedMode could
legally be 0 or 1 while secure boot is in fact enabled. It is actually
AuditMode that should be taken into account here, i.e., if AuditMode
== 1, the firmware ignores invalid or missing signatures. If
SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
non-existent), signature verification is performed regardless of the
value (or existence) of DeployedMode.

So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
boot disabled', and ignore if it is missing.

> ---
>
>  drivers/firmware/efi/libstub/secureboot.c |   16 +++++++++++++++-
>  include/linux/efi.h                       |    4 ++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index 39c91e091f6a..d653f76b9725 100644
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -22,6 +22,9 @@ static const efi_char16_t const efi_SecureBoot_name[] = {
>  static const efi_char16_t const efi_SetupMode_name[] = {
>         'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
>  };
> +static const efi_char16_t const efi_DeployedMode_name[] = {
> +       'D', 'e', 'p', 'l', 'o', 'y', 'e', 'd', 'M', 'o', 'd', 'e', 0
> +};
>
>  /* SHIM variables */
>  static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> @@ -40,7 +43,7 @@ static efi_char16_t const shim_MokSBState_name[] = {
>  enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
>  {
>         u32 attr;
> -       u8 secboot, setupmode, moksbstate;
> +       u8 secboot, setupmode, deployedmode, moksbstate;
>         unsigned long size;
>         efi_status_t status;
>
> @@ -59,6 +62,17 @@ enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
>         if (secboot == 0 || setupmode == 1)
>                 return efi_secureboot_mode_disabled;
>
> +       /* UEFI-2.6 requires DeployedMode to be 1. */
> +       if (sys_table_arg->hdr.revision >= EFI_2_60_SYSTEM_TABLE_REVISION) {
> +               size = sizeof(deployedmode);
> +               status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
> +                                    NULL, &size, &deployedmode);
> +               if (status != EFI_SUCCESS)
> +                       goto out_efi_err;
> +               if (deployedmode == 0)
> +                       return efi_secureboot_mode_disabled;
> +       }
> +
>         /* See if a user has put shim into insecure mode.  If so, and if the
>          * variable doesn't have the runtime attribute set, we might as well
>          * honor that.
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 1c200cdbdc05..87c1a6993f17 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -646,6 +646,10 @@ typedef struct {
>
>  #define EFI_SYSTEM_TABLE_SIGNATURE ((u64)0x5453595320494249ULL)
>
> +#define EFI_2_60_SYSTEM_TABLE_REVISION  ((2 << 16) | (60))
> +#define EFI_2_50_SYSTEM_TABLE_REVISION  ((2 << 16) | (50))
> +#define EFI_2_40_SYSTEM_TABLE_REVISION  ((2 << 16) | (40))
> +#define EFI_2_31_SYSTEM_TABLE_REVISION  ((2 << 16) | (31))
>  #define EFI_2_30_SYSTEM_TABLE_REVISION  ((2 << 16) | (30))
>  #define EFI_2_20_SYSTEM_TABLE_REVISION  ((2 << 16) | (20))
>  #define EFI_2_10_SYSTEM_TABLE_REVISION  ((2 << 16) | (10))
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Jan. 31, 2017, 6:59 p.m. UTC | #2

Ard Biesheuvel <[e-mail]> wrote:

> > UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
> > if we're to engage lockdown mode.
> >
> > Reported-by: James Bottomley <[e-mail]>
> > Signed-off-by: David Howells <[e-mail]>
> 
> Interestingly, the string 'DeployedMode' appears zero times in the
> EDK2 codebase, so I wonder if it makes any sense to merge this now.
> The string 'AuditMode' does appear once, but in a comment

It's in the standard, so shouldn't we check for it?

> In any case, the logic is not entirely correct either: apologies if it
> was me who caused any confusion here, but it seems DeployedMode could
> legally be 0 or 1 while secure boot is in fact enabled. It is actually
> AuditMode that should be taken into account here, i.e., if AuditMode
> == 1, the firmware ignores invalid or missing signatures. If
> SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
> non-existent), signature verification is performed regardless of the
> value (or existence) of DeployedMode.
> 
> So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
> boot disabled', and ignore if it is missing.

Ummm...  This might conflict what said:

| Since you seem to be using this to mean "is the platform locked down?",
| this looks to be no longer complete in the UEFI 2.6 world.  If
| DeployedMode == 0, even if SecureBoot == 1 and SetupMode == 0, you can
| remove the platform key by writing 1 to AuditMode and gain control of
| the secure variables.  The lock down state becomes DeployedMode == 1,
| SecureBoot == 1 and SetupMode == 0
| 
| See the diagram on page 1817
| 
| http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf

Looking again at that diagram, should I be checking all four variables
(SecureBoot, SetupMode, DeployedMode and AuditMode)?  And/or should I treat
audit mode differently to deployed mode?

Further, there doesn't seem to be a state in which SecureBoot is shown as
being 1.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Feb. 1, 2017, 10:02 a.m. UTC | #3

David Howells <[e-mail]> wrote:

> Ummm...  This might conflict what said:

Might conflict with what James said, I should've said.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ard Biesheuvel Feb. 1, 2017, 10:15 a.m. UTC | #4

On 31 January 2017 at 18:59, David Howells <[e-mail]> wrote:
> Ard Biesheuvel <[e-mail]> wrote:
>
>> > UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
>> > if we're to engage lockdown mode.
>> >
>> > Reported-by: James Bottomley <[e-mail]>
>> > Signed-off-by: David Howells <[e-mail]>
>>
>> Interestingly, the string 'DeployedMode' appears zero times in the
>> EDK2 codebase, so I wonder if it makes any sense to merge this now.
>> The string 'AuditMode' does appear once, but in a comment
>
> It's in the standard, so shouldn't we check for it?
>
>> In any case, the logic is not entirely correct either: apologies if it
>> was me who caused any confusion here, but it seems DeployedMode could
>> legally be 0 or 1 while secure boot is in fact enabled. It is actually
>> AuditMode that should be taken into account here, i.e., if AuditMode
>> == 1, the firmware ignores invalid or missing signatures. If
>> SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
>> non-existent), signature verification is performed regardless of the
>> value (or existence) of DeployedMode.
>>
>> So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
>> boot disabled', and ignore if it is missing.
>
> Ummm...  This might conflict what said:
>
> | Since you seem to be using this to mean "is the platform locked down?",
> | this looks to be no longer complete in the UEFI 2.6 world.  If
> | DeployedMode == 0, even if SecureBoot == 1 and SetupMode == 0, you can
> | remove the platform key by writing 1 to AuditMode and gain control of
> | the secure variables.  The lock down state becomes DeployedMode == 1,
> | SecureBoot == 1 and SetupMode == 0
> |
> | See the diagram on page 1817
> |
> | http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf
>
> Looking again at that diagram, should I be checking all four variables
> (SecureBoot, SetupMode, DeployedMode and AuditMode)?  And/or should I treat
> audit mode differently to deployed mode?
>

Well, we are trying to decide whether the system is locked down or
not. AuditMode is only writable before ExitBootServices(), and when
AuditMode == 0, signature verification occurs as usual, regardless of
the value of DeployedMode. Whether someone could turn on AuditMode on
the *next* boot does not sound that relevant to me, since someone
could also re-enter SetupMode in the same way.

So this patch should take AuditMode into account, but not DeployedMode, i.e.,

SecureBoot == 0x1
SetupMode == 0x0
AuditMode == 0x0 (or non-existent)

implies a locked down state.

> Further, there doesn't seem to be a state in which SecureBoot is shown as
> being 1.
>

Yes, that is sloppy. But the fact that EDK2, being the v2.6 reference,
does not implement any of this *at all* is much more worrying to me,
given that UDK2017 based firmware will certainly turn up in production
systems.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Feb. 1, 2017, 12:33 p.m. UTC | #5

Ard Biesheuvel <[e-mail]> wrote:

> So this patch should take AuditMode into account, but not DeployedMode, i.e.,
> 
> SecureBoot == 0x1
> SetupMode == 0x0
> AuditMode == 0x0 (or non-existent)

If we're in audit mode or setup mode SecureBoot==0 and SetupMode==1 according
to the flowchart, so the check of AuditMode would seem redundant.

Further, the checks above don't seem to differentiate deployed mode from user
mode.  Should they?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ard Biesheuvel Feb. 1, 2017, 2:44 p.m. UTC | #6

On 1 February 2017 at 12:33, David Howells <[e-mail]> wrote:
> Ard Biesheuvel <[e-mail]> wrote:
>
>> So this patch should take AuditMode into account, but not DeployedMode, i.e.,
>>
>> SecureBoot == 0x1
>> SetupMode == 0x0
>> AuditMode == 0x0 (or non-existent)
>
> If we're in audit mode or setup mode SecureBoot==0 and SetupMode==1 according
> to the flowchart, so the check of AuditMode would seem redundant.
>
> Further, the checks above don't seem to differentiate deployed mode from user
> mode.  Should they?
>

From the OS pov, UserMode and DeployedMode are the same, the only
difference being that AuditMode may be entered from UserMode simply by
setting the variable to 0x1 (which can only be done before
ExitBootServices()). And since AuditMode implies SetupMode (according
to the diagram), you are right that we don't need to care about
AuditMode either. AFAICT, that makes the entire patch unnecessary, so
let's drop it for now.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells Feb. 1, 2017, 3 p.m. UTC | #7

Ard Biesheuvel <[e-mail]> wrote:

> From the OS pov, UserMode and DeployedMode are the same, the only
> difference being that AuditMode may be entered from UserMode simply by
> setting the variable to 0x1 (which can only be done before
> ExitBootServices()). And since AuditMode implies SetupMode (according
> to the diagram), you are right that we don't need to care about
> AuditMode either. AFAICT, that makes the entire patch unnecessary, so
> let's drop it for now.

Okay, in that case, do you want me to reissue and place a signed tag on my
patchset without that patch, or can you pull the other patches individually?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ard Biesheuvel Feb. 1, 2017, 3:02 p.m. UTC | #8

On 1 February 2017 at 15:00, David Howells <[e-mail]> wrote:
> Ard Biesheuvel <[e-mail]> wrote:
>
>> From the OS pov, UserMode and DeployedMode are the same, the only
>> difference being that AuditMode may be entered from UserMode simply by
>> setting the variable to 0x1 (which can only be done before
>> ExitBootServices()). And since AuditMode implies SetupMode (according
>> to the diagram), you are right that we don't need to care about
>> AuditMode either. AFAICT, that makes the entire patch unnecessary, so
>> let's drop it for now.
>
> Okay, in that case, do you want me to reissue and place a signed tag on my
> patchset without that patch, or can you pull the other patches individually?
>

Let's wait for Matt to comment on the x86 bits before reissuing
anything, but if the subsequent patches still apply cleanly, I don't
think there is a need to resend or re-sign.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Fleming Feb. 2, 2017, 9:36 p.m. UTC | #9

On Wed, 01 Feb, at 03:02:14PM, Ard Biesheuvel wrote:
> 
> Let's wait for Matt to comment on the x86 bits before reissuing
> anything, but if the subsequent patches still apply cleanly, I don't
> think there is a need to resend or re-sign.

This all looks fine to me now, thanks for the re-work David.

Ard, do you want to apply these patches along with a fixup for the
multi-line comments and send a second v4.11 pull to tip?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ard Biesheuvel Feb. 2, 2017, 9:45 p.m. UTC | #10

> On 2 Feb 2017, at 21:36, Matt Fleming <[e-mail]> wrote:
> 
>> On Wed, 01 Feb, at 03:02:14PM, Ard Biesheuvel wrote:
>> 
>> Let's wait for Matt to comment on the x86 bits before reissuing
>> anything, but if the subsequent patches still apply cleanly, I don't
>> think there is a need to resend or re-sign.
> 
> This all looks fine to me now, thanks for the re-work David.
> 
> Ard, do you want to apply these patches along with a fixup for the
> multi-line comments and send a second v4.11 pull to tip?

Sure, I'll take care of it tomorrow
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 39c91e091f6a..d653f76b9725 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -22,6 +22,9 @@  static const efi_char16_t const efi_SecureBoot_name[] = {
 static const efi_char16_t const efi_SetupMode_name[] = {
 	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
 };
+static const efi_char16_t const efi_DeployedMode_name[] = {
+	'D', 'e', 'p', 'l', 'o', 'y', 'e', 'd', 'M', 'o', 'd', 'e', 0
+};
 
 /* SHIM variables */
 static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
@@ -40,7 +43,7 @@  static efi_char16_t const shim_MokSBState_name[] = {
 enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	u32 attr;
-	u8 secboot, setupmode, moksbstate;
+	u8 secboot, setupmode, deployedmode, moksbstate;
 	unsigned long size;
 	efi_status_t status;
 
@@ -59,6 +62,17 @@  enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (secboot == 0 || setupmode == 1)
 		return efi_secureboot_mode_disabled;
 
+	/* UEFI-2.6 requires DeployedMode to be 1. */
+	if (sys_table_arg->hdr.revision >= EFI_2_60_SYSTEM_TABLE_REVISION) {
+		size = sizeof(deployedmode);
+		status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
+				     NULL, &size, &deployedmode);
+		if (status != EFI_SUCCESS)
+			goto out_efi_err;
+		if (deployedmode == 0)
+			return efi_secureboot_mode_disabled;
+	}
+
 	/* See if a user has put shim into insecure mode.  If so, and if the
 	 * variable doesn't have the runtime attribute set, we might as well
 	 * honor that.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1c200cdbdc05..87c1a6993f17 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -646,6 +646,10 @@  typedef struct {
 
 #define EFI_SYSTEM_TABLE_SIGNATURE ((u64)0x5453595320494249ULL)
 
+#define EFI_2_60_SYSTEM_TABLE_REVISION  ((2 << 16) | (60))
+#define EFI_2_50_SYSTEM_TABLE_REVISION  ((2 << 16) | (50))
+#define EFI_2_40_SYSTEM_TABLE_REVISION  ((2 << 16) | (40))
+#define EFI_2_31_SYSTEM_TABLE_REVISION  ((2 << 16) | (31))
 #define EFI_2_30_SYSTEM_TABLE_REVISION  ((2 << 16) | (30))
 #define EFI_2_20_SYSTEM_TABLE_REVISION  ((2 << 16) | (20))
 #define EFI_2_10_SYSTEM_TABLE_REVISION  ((2 << 16) | (10))