| Message ID | 31374.1480078855@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 November 2016 at 13:00, David Howells <dhowells@redhat.com> wrote: > Okay, how about the attached? > > Can these variables every be anything other than 1 or 0? E.g. should the > check on SetupMode be that it isn't 0 rather than it is 1? > The firmware will ensure that these variables only ever assume the documented values. > David > --- > commit 6d4bb08e376045e27706c2740c0fdff0a6ec43f7 > Author: David Howells <dhowells@redhat.com> > Date: Fri Nov 25 11:52:05 2016 +0000 > > efi: Handle secure boot from UEFI-2.6 > > 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> > > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c > index ca643eba5a4b..157782d1c552 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; > @@ -62,6 +65,17 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg) > if (val == 1) > return 0; > > + /* UEFI-2.6 requires DeployedMode to be 1. */ > + if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) { >= > + status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid, > + NULL, &size, &val); > + if (status != EFI_SUCCESS) > + goto out_efi_err; > + > + if (val != 1) > + return 0; val == 0 is better imo, since that will prevent unexpected values from being interpreted as 'secure boot 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 333d31bf55bf..563abb37f03f 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -645,6 +645,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: > > + if (val != 1) > > + return 0; > > val == 0 is better imo, since that will prevent unexpected values from > being interpreted as 'secure boot disabled' I've made that change. David
David Howells <dhowells@redhat.com> wrote: > + /* UEFI-2.6 requires DeployedMode to be 1. */ > + if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) { Actually, I suspect that this should be '>='. David
On 25 November 2016 at 16:50, David Howells <dhowells@redhat.com> wrote: > David Howells <dhowells@redhat.com> wrote: > >> + /* UEFI-2.6 requires DeployedMode to be 1. */ >> + if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) { > > Actually, I suspect that this should be '>='. > yes, I actually pointed that out in my reply (although somewhat tersely)
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> yes, I actually pointed that out in my reply (although somewhat tersely)
Ah! Sorry, it went unnoticed amongst all the other lines beginning with '>'.
David
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c index ca643eba5a4b..157782d1c552 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; @@ -62,6 +65,17 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg) if (val == 1) return 0; + /* UEFI-2.6 requires DeployedMode to be 1. */ + if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) { + status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid, + NULL, &size, &val); + if (status != EFI_SUCCESS) + goto out_efi_err; + + if (val != 1) + return 0; + } + /* 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 333d31bf55bf..563abb37f03f 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -645,6 +645,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))
Okay, how about the attached? Can these variables every be anything other than 1 or 0? E.g. should the check on SetupMode be that it isn't 0 rather than it is 1? David --- commit 6d4bb08e376045e27706c2740c0fdff0a6ec43f7 Author: David Howells <dhowells@redhat.com> Date: Fri Nov 25 11:52:05 2016 +0000 efi: Handle secure boot from UEFI-2.6 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>