[02/16] efi: Get the secure boot status
diff mbox

Message ID 147933285147.19316.11046583275861569558.stgit@warthog.procyon.org.uk
State New
Headers show

Commit Message

David Howells Nov. 16, 2016, 9:47 p.m. UTC
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(-)


--
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

Comments

Lukas Wunner Nov. 17, 2016, 12:37 p.m. UTC | #1
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
David Howells Nov. 21, 2016, 11:42 a.m. UTC | #2
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
David Howells Nov. 21, 2016, 11:46 a.m. UTC | #3
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
Ard Biesheuvel Nov. 21, 2016, 11:52 a.m. UTC | #4
(+ 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
David Howells Nov. 21, 2016, 12:41 p.m. UTC | #5
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
Ard Biesheuvel Nov. 21, 2016, 1:14 p.m. UTC | #6
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
Lukas Wunner Nov. 21, 2016, 3:17 p.m. UTC | #7
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
Ard Biesheuvel Nov. 21, 2016, 3:25 p.m. UTC | #8
> 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
Lukas Wunner Nov. 21, 2016, 7:58 p.m. UTC | #9
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

Patch
diff mbox

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.
 	 *