| Message ID | 148587564847.4026.5759345672956585977.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 31 January 2017 at 15:14, David Howells <dhowells@redhat.com> 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 <James.Bottomley@HansenPartnership.com> > Signed-off-by: David Howells <dhowells@redhat.com> 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)) >
Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 <James.Bottomley@HansenPartnership.com> > > Signed-off-by: David Howells <dhowells@redhat.com> > > 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
David Howells <dhowells@redhat.com> wrote:
> Ummm... This might conflict what said:
Might conflict with what James said, I should've said.
David
On 31 January 2017 at 18:59, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 <James.Bottomley@HansenPartnership.com> >> > Signed-off-by: David Howells <dhowells@redhat.com> >> >> 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.
Ard Biesheuvel <ard.biesheuvel@linaro.org> 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
On 1 February 2017 at 12:33, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
Ard Biesheuvel <ard.biesheuvel@linaro.org> 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
On 1 February 2017 at 15:00, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
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?
> On 2 Feb 2017, at 21:36, Matt Fleming <matt@codeblueprint.co.uk> 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
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))
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 <James.Bottomley@HansenPartnership.com> Signed-off-by: David Howells <dhowells@redhat.com> --- drivers/firmware/efi/libstub/secureboot.c | 16 +++++++++++++++- include/linux/efi.h | 4 ++++ 2 files changed, 19 insertions(+), 1 deletion(-)