Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1523861 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-16 22:50 +0100
    Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-17 15:40 +0100
      Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 12:50 +0100
        Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-21 21:00 +0100
      Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 12:50 +0100
        Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 13:00 +0100
          Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 13:50 +0100
            Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 14:20 +0100
              Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-21 16:20 +0100
                Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 16:30 +0100
      [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit Lukas Wunner <lukas@wunner.de> - 2016-11-22 14:10 +0100
      [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode Lukas Wunner <lukas@wunner.de> - 2016-11-22 14:10 +0100
      [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
      [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Lukas Wunner <lukas@wunner.de> - 2016-11-22 11:20 +0100
          Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 15:20 +0100
            Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 16:00 +0100
              Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 17:00 +0100
                Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 17:30 +0100
                  Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 17:50 +0100
                    Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 18:00 +0100
      [PATCH 3/6] efi: Add SHIM and image security database GUID  definitions David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
      [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 11:50 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-22 11:50 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 15:50 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 16:00 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 21:30 +0100
              Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-23 01:10 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 16:10 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 21:40 +0100

Page 1 of 2  [1] 2  Next page →

#1523861 — [PATCH 02/16] efi: Get the secure boot status

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)
+{
+	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.
 	 *

[toc] | [next] | [standalone]

#1524457

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

[toc] | [prev] | [next] | [standalone]

#1526592

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

[toc] | [prev] | [next] | [standalone]

#1527014

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

[toc] | [prev] | [next] | [standalone]

#1526598

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

[toc] | [prev] | [next] | [standalone]

#1526601

(+ 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.

[toc] | [prev] | [next] | [standalone]

#1526646

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

[toc] | [prev] | [next] | [standalone]

#1526668

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.

[toc] | [prev] | [next] | [standalone]

#1526770

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

[toc] | [prev] | [next] | [standalone]

#1526788

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

[toc] | [prev] | [next] | [standalone]

#1527147 — [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit

From: Josh Boyer <jwboyer@fedoraproject.org>

UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
for use with efi_enabled.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/kernel/setup.c |    5 +++++
 include/linux/efi.h     |    1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9c337b0e8ba7..a197221a451b 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1160,6 +1160,11 @@ void __init setup_arch(char **cmdline_p)
 
 	io_delay_init();
 
+	if (boot_params.secure_boot) {
+		set_bit(EFI_SECURE_BOOT, &efi.flags);
+		pr_info("Secure boot enabled\n");
+	}
+
 	/*
 	 * Parse the ACPI tables for possible boot-time SMP configuration.
 	 */
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 615d8704f048..3864de3b40ad 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1066,6 +1066,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_ARCH_1		7	/* First arch-specific bit */
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
+#define EFI_SECURE_BOOT		10	/* Are we in Secure Boot mode? */
 
 #ifdef CONFIG_EFI
 /*

[toc] | [prev] | [next] | [standalone]

#1527497 — Re: [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit

On Tue, Nov 22, 2016 at 12:32:15AM +0000, David Howells wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
> 
> UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
> for use with efi_enabled.

Please add an explanation what you plan to do with this bit going forward.

Thanks,

Lukas

> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  arch/x86/kernel/setup.c |    5 +++++
>  include/linux/efi.h     |    1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 9c337b0e8ba7..a197221a451b 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1160,6 +1160,11 @@ void __init setup_arch(char **cmdline_p)
>  
>  	io_delay_init();
>  
> +	if (boot_params.secure_boot) {
> +		set_bit(EFI_SECURE_BOOT, &efi.flags);
> +		pr_info("Secure boot enabled\n");
> +	}
> +
>  	/*
>  	 * Parse the ACPI tables for possible boot-time SMP configuration.
>  	 */
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 615d8704f048..3864de3b40ad 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1066,6 +1066,7 @@ extern int __init efi_setup_pcdp_console(char *);
>  #define EFI_ARCH_1		7	/* First arch-specific bit */
>  #define EFI_DBG			8	/* Print additional debug info at runtime */
>  #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
> +#define EFI_SECURE_BOOT		10	/* Are we in Secure Boot mode? */
>  
>  #ifdef CONFIG_EFI
>  /*
> 

[toc] | [prev] | [next] | [standalone]

#1527150 — [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode

From: Josh Boyer <jwboyer@fedoraproject.org>

A user can manually tell the shim boot loader to disable validation of
images it loads.  When a user does this, it creates a UEFI variable called
MokSBState that does not have the runtime attribute set.  Given that the
user explicitly disabled validation, we can honor that and not enable
secure boot mode if that variable is set.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/firmware/efi/libstub/secureboot.c |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index e44d8c9ee150..d928398a3a52 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -23,10 +23,14 @@ int efi_get_secureboot(void)
 		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
 	static const efi_char16_t const sm_var_name[] = {
 		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
+	static efi_char16_t const MokSBState_var_name[] = {
+		'M', 'o', 'k', 'S', 'B', 'S', 't', 'a', 't', 'e', 0 };
 
 	static const efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
+	static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
 
-	u8 val;
+	u32 attr;
+	u8 val, moksbstate;
 	unsigned long size = sizeof(val);
 	efi_status_t status;
 
@@ -50,6 +54,22 @@ int efi_get_secureboot(void)
 	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.
+	 */
+	size = sizeof(moksbstate);
+	status = f_getvar((efi_char16_t *)MokSBState_var_name, &shim_guid,
+			  &attr, &size, &moksbstate);
+
+	/* If it fails, we don't care why.  Default to secure */
+	if (status != EFI_SUCCESS)
+		return 1;
+
+	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) &&
+	    moksbstate == 1)
+		return 0;
+
 	return 1;
 
 out_efi_err:

[toc] | [prev] | [next] | [standalone]

#1527495 — Re: [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode

On Tue, Nov 22, 2016 at 12:32:08AM +0000, David Howells wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
> 
> A user can manually tell the shim boot loader to disable validation of
> images it loads.  When a user does this, it creates a UEFI variable called
> MokSBState that does not have the runtime attribute set.  Given that the
> user explicitly disabled validation, we can honor that and not enable
> secure boot mode if that variable is set.
> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/firmware/efi/libstub/secureboot.c |   22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index e44d8c9ee150..d928398a3a52 100644
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -23,10 +23,14 @@ int efi_get_secureboot(void)
>  		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
>  	static const efi_char16_t const sm_var_name[] = {
>  		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
> +	static efi_char16_t const MokSBState_var_name[] = {
> +		'M', 'o', 'k', 'S', 'B', 'S', 't', 'a', 't', 'e', 0 };
>  
>  	static const efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> +	static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
>  
> -	u8 val;
> +	u32 attr;
> +	u8 val, moksbstate;
>  	unsigned long size = sizeof(val);
>  	efi_status_t status;
>  
> @@ -50,6 +54,22 @@ int efi_get_secureboot(void)
>  	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.
> +	 */
> +	size = sizeof(moksbstate);
> +	status = f_getvar((efi_char16_t *)MokSBState_var_name, &shim_guid,
> +			  &attr, &size, &moksbstate);

Please use efi_call_runtime() instead of f_getvar().

> +
> +	/* If it fails, we don't care why.  Default to secure */
> +	if (status != EFI_SUCCESS)
> +		return 1;
> +
> +	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) &&
> +	    moksbstate == 1)

This would fit on a single line.

Thanks,

Lukas

> +		return 0;
> +
>  	return 1;
>  
>  out_efi_err:
> 

[toc] | [prev] | [next] | [standalone]

#1527151 — [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services

Provide the ability to perform mixed-mode runtime service calls for arm in
the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides
the ability to invoke arbitrary boot services.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/arm/include/asm/efi.h   |    1 +
 arch/arm64/include/asm/efi.h |    1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 0b06f5341b45..e4e6a9d6a825 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -55,6 +55,7 @@ void efi_virtmap_unload(void);
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
 #define efi_is_64bit()			(false)
 
 #define efi_call_proto(protocol, f, instance, ...)			\
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 771b3f0bc757..d74ae223d89f 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -49,6 +49,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
 #define efi_is_64bit()			(true)
 
 #define efi_call_proto(protocol, f, instance, ...)			\

[toc] | [prev] | [next] | [standalone]

#1527154 — [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

Provide the ability to perform mixed-mode runtime service calls for x86 in
the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides
the ability to invoke arbitrary boot services.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/boot/compressed/eboot.c   |    1 +
 arch/x86/boot/compressed/head_32.S |    6 +++---
 arch/x86/boot/compressed/head_64.S |    8 ++++----
 arch/x86/include/asm/efi.h         |    5 +++++
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ff01c8fc76f7..c8c32ebcdfdb 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c)		\
 									\
 	table = (typeof(table))sys_table;				\
 									\
+	c->runtime_services = table->runtime;				\
 	c->boot_services = table->boottime;				\
 	c->text_output = table->con_out;				\
 }
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index fd0b6a272dd5..d85b9625e836 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 32(%eax)
+	add	%esi, 40(%eax)
 	pushl	%eax
 
 	call	make_boot_params
@@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 32(%eax)
+	add	%esi, 40(%eax)
 	pushl	%eax
 2:
 	call	efi_main
@@ -264,7 +264,7 @@ relocated:
 #ifdef CONFIG_EFI_STUB
 	.data
 efi32_config:
-	.fill 4,8,0
+	.fill 5,8,0
 	.long efi_call_phys
 	.long 0
 	.byte 0
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index efdfba21a5b2..beab8322f72a 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
 	/*
 	 * Relocate efi_config->call().
 	 */
-	addq	%rbp, efi64_config+32(%rip)
+	addq	%rbp, efi64_config+40(%rip)
 
 	movq	%rax, %rdi
 	call	make_boot_params
@@ -285,7 +285,7 @@ handover_entry:
 	 * Relocate efi_config->call().
 	 */
 	movq	efi_config(%rip), %rax
-	addq	%rbp, 32(%rax)
+	addq	%rbp, 40(%rax)
 2:
 	movq	efi_config(%rip), %rdi
 	call	efi_main
@@ -457,14 +457,14 @@ efi_config:
 #ifdef CONFIG_EFI_MIXED
 	.global efi32_config
 efi32_config:
-	.fill	4,8,0
+	.fill	5,8,0
 	.quad	efi64_thunk
 	.byte	0
 #endif
 
 	.global efi64_config
 efi64_config:
-	.fill	4,8,0
+	.fill	5,8,0
 	.quad	efi_call
 	.byte	1
 #endif /* CONFIG_EFI_STUB */
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index e99675b9c861..2f77bcefe6b4 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -191,6 +191,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map(
 struct efi_config {
 	u64 image_handle;
 	u64 table;
+	u64 runtime_services;
 	u64 boot_services;
 	u64 text_output;
 	efi_status_t (*call)(unsigned long, ...);
@@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void)
 #define __efi_call_early(f, ...)					\
 	__efi_early()->call((unsigned long)f, __VA_ARGS__);
 
+#define efi_call_runtime(f, ...)					\
+	__efi_early()->call(efi_table_attr(efi_runtime_services, f,	\
+		__efi_early()->runtime_services), __VA_ARGS__)
+
 extern bool efi_reboot_required(void);
 
 #else

[toc] | [prev] | [next] | [standalone]

#1527368 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

On Tue, Nov 22, 2016 at 12:31:39AM +0000, David Howells wrote:
> Provide the ability to perform mixed-mode runtime service calls for x86 in
> the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides

Small nit, checkpatch usually complains that this should be written as
12-character SHA-1 followed by the commit subject, i.e.

0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

Other than that LGTM.  Same for patch 2 of this series.

Thanks,

Lukas

> the ability to invoke arbitrary boot services.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  arch/x86/boot/compressed/eboot.c   |    1 +
>  arch/x86/boot/compressed/head_32.S |    6 +++---
>  arch/x86/boot/compressed/head_64.S |    8 ++++----
>  arch/x86/include/asm/efi.h         |    5 +++++
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ff01c8fc76f7..c8c32ebcdfdb 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c)		\
>  									\
>  	table = (typeof(table))sys_table;				\
>  									\
> +	c->runtime_services = table->runtime;				\
>  	c->boot_services = table->boottime;				\
>  	c->text_output = table->con_out;				\
>  }
> diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> index fd0b6a272dd5..d85b9625e836 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
>  
>  	/* Relocate efi_config->call() */
>  	leal	efi32_config(%esi), %eax
> -	add	%esi, 32(%eax)
> +	add	%esi, 40(%eax)
>  	pushl	%eax
>  
>  	call	make_boot_params
> @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
>  
>  	/* Relocate efi_config->call() */
>  	leal	efi32_config(%esi), %eax
> -	add	%esi, 32(%eax)
> +	add	%esi, 40(%eax)
>  	pushl	%eax
>  2:
>  	call	efi_main
> @@ -264,7 +264,7 @@ relocated:
>  #ifdef CONFIG_EFI_STUB
>  	.data
>  efi32_config:
> -	.fill 4,8,0
> +	.fill 5,8,0
>  	.long efi_call_phys
>  	.long 0
>  	.byte 0
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index efdfba21a5b2..beab8322f72a 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
>  	/*
>  	 * Relocate efi_config->call().
>  	 */
> -	addq	%rbp, efi64_config+32(%rip)
> +	addq	%rbp, efi64_config+40(%rip)
>  
>  	movq	%rax, %rdi
>  	call	make_boot_params
> @@ -285,7 +285,7 @@ handover_entry:
>  	 * Relocate efi_config->call().
>  	 */
>  	movq	efi_config(%rip), %rax
> -	addq	%rbp, 32(%rax)
> +	addq	%rbp, 40(%rax)
>  2:
>  	movq	efi_config(%rip), %rdi
>  	call	efi_main
> @@ -457,14 +457,14 @@ efi_config:
>  #ifdef CONFIG_EFI_MIXED
>  	.global efi32_config
>  efi32_config:
> -	.fill	4,8,0
> +	.fill	5,8,0
>  	.quad	efi64_thunk
>  	.byte	0
>  #endif
>  
>  	.global efi64_config
>  efi64_config:
> -	.fill	4,8,0
> +	.fill	5,8,0
>  	.quad	efi_call
>  	.byte	1
>  #endif /* CONFIG_EFI_STUB */
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index e99675b9c861..2f77bcefe6b4 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -191,6 +191,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map(
>  struct efi_config {
>  	u64 image_handle;
>  	u64 table;
> +	u64 runtime_services;
>  	u64 boot_services;
>  	u64 text_output;
>  	efi_status_t (*call)(unsigned long, ...);
> @@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void)
>  #define __efi_call_early(f, ...)					\
>  	__efi_early()->call((unsigned long)f, __VA_ARGS__);
>  
> +#define efi_call_runtime(f, ...)					\
> +	__efi_early()->call(efi_table_attr(efi_runtime_services, f,	\
> +		__efi_early()->runtime_services), __VA_ARGS__)
> +
>  extern bool efi_reboot_required(void);
>  
>  #else
> 

[toc] | [prev] | [next] | [standalone]

#1527538 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

Lukas Wunner <lukas@wunner.de> wrote:

> Small nit, checkpatch usually complains that this should be written as
> 12-character SHA-1 followed by the commit subject, i.e.
> 
> 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

In this case, checkpatch is wrong.

David

[toc] | [prev] | [next] | [standalone]

#1527577 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

On Tue, 2016-11-22 at 14:17 +0000, David Howells wrote:
> Lukas Wunner <lukas@wunner.de> wrote:
> 
> > Small nit, checkpatch usually complains that this should be written as
> > 12-character SHA-1 followed by the commit subject, i.e.
> > 
> > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> 
> In this case, checkpatch is wrong.

Why do you think so?

[toc] | [prev] | [next] | [standalone]

#1527629 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

Joe Perches <joe@perches.com> wrote:

> > > Small nit, checkpatch usually complains that this should be written as
> > > 12-character SHA-1 followed by the commit subject, i.e.
> > > 
> > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > 
> > In this case, checkpatch is wrong.
>
> Why do you think so?

Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
that case, it's just about acceptable.

Apart from that, I think we should put in the full SHA-1 commit.  The
probability of a collision in a 12-digit hex number for the >5,000,000 commits
just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
all the commits in not-yet-upstreamed trees - which might be another million
commits, say - then we're over 6%..

Oh, yes, and speaking of checkpatch, can you make it so that if it sees:

commit 12345...
Author: foo <foo@bar>
Date: blah

   Subject line

   Description lines
   ...
   ...
   ...
   ...

   Signed-off-by-and-suchline-lines

diff ...

with the all description indented by 4 spaces, then assume that it's the
output of git show and not give the warnings about signed-off-by and other
things being indented?

David

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web