| Message ID | 147933285147.19316.11046583275861569558.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 16, 2016 at 09:47:31PM +0000, David Howells wrote: > Get the firmware's secure-boot status in the kernel boot wrapper and stash > it somewhere that the main kernel image can find. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > Documentation/x86/zero-page.txt | 2 ++ > arch/x86/boot/compressed/eboot.c | 35 +++++++++++++++++++++++++++++++++ > arch/x86/include/uapi/asm/bootparam.h | 3 ++- > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt > index 95a4d34af3fd..b8527c6b7646 100644 > --- a/Documentation/x86/zero-page.txt > +++ b/Documentation/x86/zero-page.txt > @@ -31,6 +31,8 @@ Offset Proto Name Meaning > 1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below) > 1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in edd_mbr_sig_buffer > (below) > +1EB/001 ALL kbd_status Numlock is enabled > +1EC/001 ALL secure_boot Secure boot is enabled in the firmware > 1EF/001 ALL sentinel Used to detect broken bootloaders > 290/040 ALL edd_mbr_sig_buffer EDD MBR signatures > 2D0/A00 ALL e820_map E820 memory map table > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index cc69e37548db..17b376596c96 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -12,6 +12,7 @@ > #include <asm/efi.h> > #include <asm/setup.h> > #include <asm/desc.h> > +#include <asm/bootparam_utils.h> > > #include "../string.h" > #include "eboot.h" > @@ -537,6 +538,36 @@ static void setup_efi_pci(struct boot_params *params) > efi_call_early(free_pool, pci_handle); > } > > +static int get_secure_boot(void) > +{ This function is very similar to the existing efi_get_secureboot() in drivers/firmware/efi/libstub/arm-stub.c. Please avoid adding more duplicate code to the EFI stub and try to reuse the existing code. I suggest moving the existing efi_get_secureboot() to a new file drivers/firmware/efi/libstub/secureboot.c which gets linked into libstub, perhaps dependent on a new config option. > + u8 sb, setup; > + unsigned long datasize = sizeof(sb); > + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > + efi_status_t status; > + > + status = efi_early->call((unsigned long)sys_table->runtime->get_variable, > + L"SecureBoot", &var_guid, NULL, &datasize, &sb); This doesn't work in mixed mode. We already have the efi_call_early() macro to call boot services in a manner that works across all arches and bitness variants. In 4.10 there will be an efi_call_proto() macro to allow the same for protocol calls: http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=efi/core&id=3552fdf29f01 I suggest adding an efi_call_runtime() macro for arch- and bitness- agnostic runtime services calls, like this: #define efi_call_runtime(f, ...) \ __efi_early()->call(efi_table_attr(efi_runtime_services, f, \ __efi_early()->runtime_services), __VA_ARGS__) For this to work you need to add a runtime_services attribute to struct efi_config, this requires modifying head_32.S and head_64.S, use commit 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") as a template. If you define corresponding efi_call_runtime() macros for ARM, you should indeed be able to share this function across arches. Thanks, Lukas > + > + if (status != EFI_SUCCESS) > + return 0; > + > + if (sb == 0) > + return 0; > + > + > + status = efi_early->call((unsigned long)sys_table->runtime->get_variable, > + L"SetupMode", &var_guid, NULL, &datasize, > + &setup); > + > + if (status != EFI_SUCCESS) > + return 0; > + > + if (setup == 1) > + return 0; > + > + return 1; > +} > + > static efi_status_t > setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) > { > @@ -1094,6 +1125,10 @@ struct boot_params *efi_main(struct efi_config *c, > else > setup_boot_services32(efi_early); > > + sanitize_boot_params(boot_params); > + > + boot_params->secure_boot = get_secure_boot(); > + > setup_graphics(boot_params); > > setup_efi_pci(boot_params); > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h > index c18ce67495fa..2b3e5427097b 100644 > --- a/arch/x86/include/uapi/asm/bootparam.h > +++ b/arch/x86/include/uapi/asm/bootparam.h > @@ -134,7 +134,8 @@ struct boot_params { > __u8 eddbuf_entries; /* 0x1e9 */ > __u8 edd_mbr_sig_buf_entries; /* 0x1ea */ > __u8 kbd_status; /* 0x1eb */ > - __u8 _pad5[3]; /* 0x1ec */ > + __u8 secure_boot; /* 0x1ec */ > + __u8 _pad5[2]; /* 0x1ed */ > /* > * The sentinel is set to a nonzero value (0xff) in header.S. > * > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lukas, Looking in efi_get_secureboot(), is there a reason: efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; isn't static const? David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lukas Wunner <lukas@wunner.de> wrote: > We already have the efi_call_early() macro to call boot services > in a manner that works across all arches and bitness variants. > > In 4.10 there will be an efi_call_proto() macro to allow the same > for protocol calls: > http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=efi/core&id=3552fdf29f01 > > I suggest adding an efi_call_runtime() macro for arch- and bitness- > agnostic runtime services calls, like this: > > #define efi_call_runtime(f, ...) \ > __efi_early()->call(efi_table_attr(efi_runtime_services, f, \ > __efi_early()->runtime_services), __VA_ARGS__) > > For this to work you need to add a runtime_services attribute to struct > efi_config, this requires modifying head_32.S and head_64.S, use commit > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") > as a template. > > If you define corresponding efi_call_runtime() macros for ARM, you > should indeed be able to share this function across arches. I'm not sure why I need to do this if I replace get_secure_boot() from my patch with a call to efi_get_secureboot(). David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(+ Linn) On 21 November 2016 at 11:42, David Howells <dhowells@redhat.com> wrote: > Hi Lukas, > > Looking in efi_get_secureboot(), is there a reason: > > efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > > isn't static const? > Not a good one, no. It used to be static const, but for some reason, commit 30d7bf034c03 ("efi/arm64: Check SetupMode when determining Secure Boot status") removed the static and the const (and I reviewed it and did not complain AFAIR) I'll gladly take a patch that reinstates that, though. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Looking in efi_get_secureboot(), is there a reason: > > > > efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > > > > isn't static const? > > > > Not a good one, no. It used to be static const, but for some reason, > commit 30d7bf034c03 ("efi/arm64: Check SetupMode when determining > Secure Boot status") removed the static and the const (and I reviewed > it and did not complain AFAIR) > I'll gladly take a patch that reinstates that, though. Also, is there a reason that: typedef efi_status_t efi_get_variable_t (efi_char16_t *name, efi_guid_t *vendor, u32 *attr, unsigned long *data_size, void *data); Doesn't have const name and vendor? David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21 November 2016 at 12:41, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > Looking in efi_get_secureboot(), is there a reason: >> > >> > efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; >> > >> > isn't static const? >> > >> >> Not a good one, no. It used to be static const, but for some reason, >> commit 30d7bf034c03 ("efi/arm64: Check SetupMode when determining >> Secure Boot status") removed the static and the const (and I reviewed >> it and did not complain AFAIR) >> I'll gladly take a patch that reinstates that, though. > > Also, is there a reason that: > > typedef efi_status_t efi_get_variable_t (efi_char16_t *name, efi_guid_t *vendor, u32 *attr, > unsigned long *data_size, void *data); > > Doesn't have const name and vendor? > Yes, but not a good one either. Sadly, the prototypes in the UEFI spec completely ignore constness, and these definitions are intended to be identical to the ones in the spec. This also means, for instance, that most UEFI firmwares stores these kinds of GUIDs in read-write memory, which is a potential goldmine for hackers, given how GUIDs are UEFI's duct tape, i.e., keeping the world together. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 21, 2016 at 01:14:52PM +0000, Ard Biesheuvel wrote: > On 21 November 2016 at 12:41, David Howells <dhowells@redhat.com> wrote: > > Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > Looking in efi_get_secureboot(), is there a reason: > >> > > >> > efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > >> > > >> > isn't static const? > >> > >> Not a good one, no. It used to be static const, but for some reason, > >> commit 30d7bf034c03 ("efi/arm64: Check SetupMode when determining > >> Secure Boot status") removed the static and the const (and I reviewed > >> it and did not complain AFAIR) > >> I'll gladly take a patch that reinstates that, though. > > > > Also, is there a reason that: > > > > typedef efi_status_t efi_get_variable_t (efi_char16_t *name, efi_guid_t *vendor, u32 *attr, > > unsigned long *data_size, void *data); > > > > Doesn't have const name and vendor? > > Yes, but not a good one either. > > Sadly, the prototypes in the UEFI spec completely ignore constness, > and these definitions are intended to be identical to the ones in the > spec. This also means, for instance, that most UEFI firmwares stores > these kinds of GUIDs in read-write memory, which is a potential > goldmine for hackers, given how GUIDs are UEFI's duct tape, i.e., > keeping the world together. But the spec declares these two parameters as "IN", so it would seem legal to declare them const, no? Incidentally I've already prepared commits a couple of days ago to change the GUID declarations to const everywhere and also change the get_variable prototype, I was planning to submit them for 4.11... :-) Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 21 Nov 2016, at 15:17, Lukas Wunner <lukas@wunner.de> wrote: > >> On Mon, Nov 21, 2016 at 01:14:52PM +0000, Ard Biesheuvel wrote: >>> On 21 November 2016 at 12:41, David Howells <dhowells@redhat.com> wrote: >>> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>>> Looking in efi_get_secureboot(), is there a reason: >>>>> >>>>> efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; >>>>> >>>>> isn't static const? >>>> >>>> Not a good one, no. It used to be static const, but for some reason, >>>> commit 30d7bf034c03 ("efi/arm64: Check SetupMode when determining >>>> Secure Boot status") removed the static and the const (and I reviewed >>>> it and did not complain AFAIR) >>>> I'll gladly take a patch that reinstates that, though. >>> >>> Also, is there a reason that: >>> >>> typedef efi_status_t efi_get_variable_t (efi_char16_t *name, efi_guid_t *vendor, u32 *attr, >>> unsigned long *data_size, void *data); >>> >>> Doesn't have const name and vendor? >> >> Yes, but not a good one either. >> >> Sadly, the prototypes in the UEFI spec completely ignore constness, >> and these definitions are intended to be identical to the ones in the >> spec. This also means, for instance, that most UEFI firmwares stores >> these kinds of GUIDs in read-write memory, which is a potential >> goldmine for hackers, given how GUIDs are UEFI's duct tape, i.e., >> keeping the world together. > > But the spec declares these two parameters as "IN", so it would seem > legal to declare them const, no? > Good point. > Incidentally I've already prepared commits a couple of days ago to > change the GUID declarations to const everywhere and also change the > get_variable prototype, I was planning to submit them for 4.11... :-) > I would like to take those, provided that they only modify IN pointer arguments.-- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 21, 2016 at 11:46:51AM +0000, David Howells wrote: > Lukas Wunner <lukas@wunner.de> wrote: > > We already have the efi_call_early() macro to call boot services > > in a manner that works across all arches and bitness variants. > > > > In 4.10 there will be an efi_call_proto() macro to allow the same > > for protocol calls: > > http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=efi/core&id=3552fdf29f01 > > > > I suggest adding an efi_call_runtime() macro for arch- and bitness- > > agnostic runtime services calls, like this: > > > > #define efi_call_runtime(f, ...) \ > > __efi_early()->call(efi_table_attr(efi_runtime_services, f, \ > > __efi_early()->runtime_services), __VA_ARGS__) > > > > For this to work you need to add a runtime_services attribute to struct > > efi_config, this requires modifying head_32.S and head_64.S, use commit > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services") > > as a template. > > > > If you define corresponding efi_call_runtime() macros for ARM, you > > should indeed be able to share this function across arches. > > I'm not sure why I need to do this if I replace get_secure_boot() from my > patch with a call to efi_get_secureboot(). You need to do this to make the code run correctly in mixed mode (64 bit CPU, but 32-bit EFI). This dereferences efi_system_table_t *sys_table_arg as well as efi_runtime_services_t *runtime: efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable; The problem is that efi_system_table_t and efi_runtime_services_t uses 64-bit wide elements when compiled on 64-bit (unsigned long or void *). They need to be cast to efi_system_table_32_t and efi_runtime_services_32_t at runtime if EFI is 32-bit. The efi_call_early() and efi_call_proto() macros do this automatically. I suggest that you add efi_call_runtime() for symmetry. Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt index 95a4d34af3fd..b8527c6b7646 100644 --- a/Documentation/x86/zero-page.txt +++ b/Documentation/x86/zero-page.txt @@ -31,6 +31,8 @@ Offset Proto Name Meaning 1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below) 1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in edd_mbr_sig_buffer (below) +1EB/001 ALL kbd_status Numlock is enabled +1EC/001 ALL secure_boot Secure boot is enabled in the firmware 1EF/001 ALL sentinel Used to detect broken bootloaders 290/040 ALL edd_mbr_sig_buffer EDD MBR signatures 2D0/A00 ALL e820_map E820 memory map table diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index cc69e37548db..17b376596c96 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -12,6 +12,7 @@ #include <asm/efi.h> #include <asm/setup.h> #include <asm/desc.h> +#include <asm/bootparam_utils.h> #include "../string.h" #include "eboot.h" @@ -537,6 +538,36 @@ static void setup_efi_pci(struct boot_params *params) efi_call_early(free_pool, pci_handle); } +static int get_secure_boot(void) +{ + u8 sb, setup; + unsigned long datasize = sizeof(sb); + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; + efi_status_t status; + + status = efi_early->call((unsigned long)sys_table->runtime->get_variable, + L"SecureBoot", &var_guid, NULL, &datasize, &sb); + + if (status != EFI_SUCCESS) + return 0; + + if (sb == 0) + return 0; + + + status = efi_early->call((unsigned long)sys_table->runtime->get_variable, + L"SetupMode", &var_guid, NULL, &datasize, + &setup); + + if (status != EFI_SUCCESS) + return 0; + + if (setup == 1) + return 0; + + return 1; +} + static efi_status_t setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) { @@ -1094,6 +1125,10 @@ struct boot_params *efi_main(struct efi_config *c, else setup_boot_services32(efi_early); + sanitize_boot_params(boot_params); + + boot_params->secure_boot = get_secure_boot(); + setup_graphics(boot_params); setup_efi_pci(boot_params); diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c18ce67495fa..2b3e5427097b 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -134,7 +134,8 @@ struct boot_params { __u8 eddbuf_entries; /* 0x1e9 */ __u8 edd_mbr_sig_buf_entries; /* 0x1ea */ __u8 kbd_status; /* 0x1eb */ - __u8 _pad5[3]; /* 0x1ec */ + __u8 secure_boot; /* 0x1ec */ + __u8 _pad5[2]; /* 0x1ed */ /* * The sentinel is set to a nonzero value (0xff) in header.S. *