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

Groups > linux.kernel > #1390021 > unrolled thread

[tip:efi/core] efibc: Add EFI Bootloader Control module

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

  [tip:efi/core] efibc: Add EFI Bootloader Control module "tip-bot for Compostella, Jeremy" <tipbot@zytor.com> - 2016-04-28 12:50 +0200
    Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Ingo Molnar <mingo@kernel.org> - 2016-04-29 12:00 +0200
      Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-29 12:40 +0200
        Re: [tip:efi/core] efibc: Add EFI Bootloader Control module jeremy.compostella@intel.com (Compostella, Jeremy) - 2016-04-29 13:40 +0200
          Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-29 14:20 +0200
            Re: [tip:efi/core] efibc: Add EFI Bootloader Control module jeremy.compostella@intel.com (Compostella, Jeremy) - 2016-04-29 16:00 +0200
              Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Ingo Molnar <mingo@kernel.org> - 2016-04-29 21:50 +0200
                Re: [tip:efi/core] efibc: Add EFI Bootloader Control module jeremy.compostella@intel.com (Compostella, Jeremy) - 2016-04-30 10:40 +0200
                  Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-30 22:10 +0200
              Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-30 22:10 +0200
                Re: [tip:efi/core] efibc: Add EFI Bootloader Control module jeremy.compostella@intel.com (Compostella, Jeremy) - 2016-05-02 10:00 +0200
                  Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-03 16:50 +0200
      Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-05-01 10:10 +0200
        Re: [tip:efi/core] efibc: Add EFI Bootloader Control module Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-01 15:20 +0200

#1390021 — [tip:efi/core] efibc: Add EFI Bootloader Control module

Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
Author:     Compostella, Jeremy <jeremy.compostella@intel.com>
AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 28 Apr 2016 11:34:02 +0200

efibc: Add EFI Bootloader Control module

This module installs a reboot callback, such that if reboot() is invoked
with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
EFI variable, to be read by the bootloader.

If the string matches one of the boot labels defined in its configuration,
the bootloader will boot once to that label.  The "LoaderEntryRebootReason"
EFI variable is set with the reboot reason: "reboot", "shutdown".

The bootloader reads this reboot reason and takes particular action
according to its policy.

There are reboot implementations that do "reboot <reason>", such as
Android's reboot command and Upstart's reboot replacement, which pass
the reason as an argument to the reboot syscall.  There is no
platform-agnostic way how those could be modified to pass the reason
to the bootloader, regardless of platform or bootloader.

Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stefan Stanacar <stefan.stanacar@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/1461614832-17633-26-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/Kconfig  |  15 +++++++
 drivers/firmware/efi/Makefile |   1 +
 drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h           |   4 ++
 4 files changed, 121 insertions(+)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..0b0b635 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,21 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_BOOTLOADER_CONTROL
+	tristate "EFI Bootloader Control"
+	depends on EFI_VARS
+	default n
+	---help---
+	  This module installs a reboot hook, such that if reboot() is
+	  invoked with a string argument NNN, "NNN" is copied to the
+	  "LoaderEntryOneShot" EFI variable, to be read by the
+	  bootloader. If the string matches one of the boot labels
+	  defined in its configuration, the bootloader will boot once
+	  to that label. The "LoaderEntryRebootReason" EFI variable is
+	  set with the reboot reason: "reboot" or "shutdown". The
+	  bootloader reads this reboot reason and takes particular
+	  action according to its policy.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index d5be623..b080808 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
+obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
new file mode 100644
index 0000000..2e0c7cc
--- /dev/null
+++ b/drivers/firmware/efi/efibc.c
@@ -0,0 +1,101 @@
+/*
+ * efibc: control EFI bootloaders which obey LoaderEntryOneShot var
+ * Copyright (c) 2013-2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#define pr_fmt(fmt) "efibc: " fmt
+
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/reboot.h>
+
+static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
+{
+	size_t i;
+
+	for (i = 0; i < strlen(str); i++)
+		str16[i] = str[i];
+
+	str16[i] = '\0';
+}
+
+static void efibc_set_variable(const char *name, const char *value)
+{
+	int ret;
+	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
+	struct efivar_entry entry;
+	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
+
+	if (size > sizeof(entry.var.Data))
+		pr_err("value is too large");
+
+	efibc_str_to_str16(name, entry.var.VariableName);
+	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
+	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
+
+	ret = efivar_entry_set(&entry,
+			       EFI_VARIABLE_NON_VOLATILE
+			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
+			       | EFI_VARIABLE_RUNTIME_ACCESS,
+			       size, entry.var.Data, NULL);
+	if (ret)
+		pr_err("failed to set %s EFI variable: 0x%x\n",
+		       name, ret);
+}
+
+static int efibc_reboot_notifier_call(struct notifier_block *notifier,
+				      unsigned long event, void *data)
+{
+	const char *reason = "shutdown";
+
+	if (event == SYS_RESTART)
+		reason = "reboot";
+
+	efibc_set_variable("LoaderEntryRebootReason", reason);
+
+	if (!data)
+		return NOTIFY_DONE;
+
+	efibc_set_variable("LoaderEntryOneShot", (char *)data);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block efibc_reboot_notifier = {
+	.notifier_call = efibc_reboot_notifier_call,
+};
+
+static int __init efibc_init(void)
+{
+	int ret;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
+
+	ret = register_reboot_notifier(&efibc_reboot_notifier);
+	if (ret)
+		pr_err("unable to register reboot notifier\n");
+
+	return ret;
+}
+module_init(efibc_init);
+
+static void __exit efibc_exit(void)
+{
+	unregister_reboot_notifier(&efibc_reboot_notifier);
+}
+module_exit(efibc_exit);
+
+MODULE_AUTHOR("Jeremy Compostella <jeremy.compostella@intel.com>");
+MODULE_AUTHOR("Matt Gumbel <matthew.k.gumbel@intel.com");
+MODULE_DESCRIPTION("EFI Bootloader Control");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e534588..4db7052 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -642,6 +642,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0xe03fc20a, 0x85dc, 0x406e, \
 		 0xb9, 0xe, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
 
+#define LINUX_EFI_LOADER_ENTRY_GUID \
+	EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, \
+		 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;

[toc] | [next] | [standalone]

#1390953

* tip-bot for Compostella, Jeremy <tipbot@zytor.com> wrote:

> Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
> Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
> Author:     Compostella, Jeremy <jeremy.compostella@intel.com>
> AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Thu, 28 Apr 2016 11:34:02 +0200
> 
> efibc: Add EFI Bootloader Control module
> 
> This module installs a reboot callback, such that if reboot() is invoked
> with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
> EFI variable, to be read by the bootloader.

>  drivers/firmware/efi/Kconfig  |  15 +++++++
>  drivers/firmware/efi/Makefile |   1 +
>  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/efi.h           |   4 ++
>  4 files changed, 121 insertions(+)

So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this 
new warning:

  drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256 bytes is larger than 1024 bytes [-Wframe-larger-than=]

2K of stack use for a function is quite excessive, can we improve the stack 
footprint of this code?

Thanks,

	Ingo

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

#1390978

On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote:
> 
> * tip-bot for Compostella, Jeremy <tipbot@zytor.com> wrote:
> 
> > Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
> > Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
> > Author:     Compostella, Jeremy <jeremy.compostella@intel.com>
> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Thu, 28 Apr 2016 11:34:02 +0200
> > 
> > efibc: Add EFI Bootloader Control module
> > 
> > This module installs a reboot callback, such that if reboot() is invoked
> > with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
> > EFI variable, to be read by the bootloader.
> 
> >  drivers/firmware/efi/Kconfig  |  15 +++++++
> >  drivers/firmware/efi/Makefile |   1 +
> >  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/efi.h           |   4 ++
> >  4 files changed, 121 insertions(+)
> 
> So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this 
> new warning:
> 
>   drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> 2K of stack use for a function is quite excessive, can we improve the stack 
> footprint of this code?

I'm waiting to hear from Jeremy on whether we can simply move the
struct efivar_entry (which is the cause of the stack bloat) off the
stack and into the .bss, because it only gets used from the reboot
notifier call chain.

But upon reading kernel_restart() I'm no longer sure it's guaranteed
to be called only once, or even non-concurrently. It seems that if the
user executes the reboot command and either the sysrq reboot code is
invoked, or an error is encountered dm-verify-target driver or any
other kernel_restart() caller is invoked we could race.

Perhaps we should guard efi_reboot_notifier_call() with an atomic_t
and exit if we've already invoked it?

Alternatively, we could just kmalloc() the object ;)

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

#1391028

Matt Fleming <matt@codeblueprint.co.uk> writes:

> On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote:
>> 
>> * tip-bot for Compostella, Jeremy <tipbot@zytor.com> wrote:
>> 
>> > Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Author:     Compostella, Jeremy <jeremy.compostella@intel.com>
>> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
>> > Committer:  Ingo Molnar <mingo@kernel.org>
>> > CommitDate: Thu, 28 Apr 2016 11:34:02 +0200
>> > 
>> > efibc: Add EFI Bootloader Control module
>> > 
>> > This module installs a reboot callback, such that if reboot() is invoked
>> > with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
>> > EFI variable, to be read by the bootloader.
>> 
>> >  drivers/firmware/efi/Kconfig  |  15 +++++++
>> >  drivers/firmware/efi/Makefile |   1 +
>> >  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/efi.h           |   4 ++
>> >  4 files changed, 121 insertions(+)
>> 
>> So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this 
>> new warning:
>> 
>>   drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256
>> bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> 
>> 2K of stack use for a function is quite excessive, can we improve the stack 
>> footprint of this code?
>
> I'm waiting to hear from Jeremy on whether we can simply move the
> struct efivar_entry (which is the cause of the stack bloat) off the
> stack and into the .bss, because it only gets used from the reboot
> notifier call chain.
>
> But upon reading kernel_restart() I'm no longer sure it's guaranteed
> to be called only once, or even non-concurrently. It seems that if the
> user executes the reboot command and either the sysrq reboot code is
> invoked, or an error is encountered dm-verify-target driver or any
> other kernel_restart() caller is invoked we could race.
>
> Perhaps we should guard efi_reboot_notifier_call() with an atomic_t
> and exit if we've already invoked it?
>
> Alternatively, we could just kmalloc() the object ;)
I would go for this last proposal because it fixes the issue, it works
with the potential race condition you mentioned and it is a simple
solution.  Yet, it means that if we really run into a race condition,
the LoaderEntryOneShot value might be indeterminate.

Though, this is a very unlikely situation and if it happens I cannot
think of any good reason to prefer to store the first restart target
or the second.

Do you want me to make this change ?

Jérémy
-- 
One Emacs to rule them all

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

#1391048

On Fri, 29 Apr, at 01:36:01PM, Jeremy Compostella wrote:
> I would go for this last proposal because it fixes the issue, it works
> with the potential race condition you mentioned and it is a simple
> solution.  Yet, it means that if we really run into a race condition,
> the LoaderEntryOneShot value might be indeterminate.
> 
> Though, this is a very unlikely situation and if it happens I cannot
> think of any good reason to prefer to store the first restart target
> or the second.
> 
> Do you want me to make this change ?

Yes please, that'd be greatly appreciated. Be sure to do it on top of
either the EFI 'next' branch or tip:efi/core.

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

#1391177

[Multipart message — attachments visible in raw view] — view raw

Matt Fleming <matt@codeblueprint.co.uk> writes:

> On Fri, 29 Apr, at 01:36:01PM, Jeremy Compostella wrote:
>> I would go for this last proposal because it fixes the issue, it works
>> with the potential race condition you mentioned and it is a simple
>> solution.  Yet, it means that if we really run into a race condition,
>> the LoaderEntryOneShot value might be indeterminate.
>> 
>> Though, this is a very unlikely situation and if it happens I cannot
>> think of any good reason to prefer to store the first restart target
>> or the second.
>> 
>> Do you want me to make this change ?
>
> Yes please, that'd be greatly appreciated. Be sure to do it on top of
> either the EFI 'next' branch or tip:efi/core.
Please, find the patch in attachment.  I wrote it on top of
tip:efi/core.

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

#1391369

* Compostella, Jeremy <jeremy.compostella@intel.com> wrote:

> -static void efibc_set_variable(const char *name, const char *value)
> +static int efibc_set_variable(const char *name, const char *value)
>  {
>  	int ret;
>  	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
> -	struct efivar_entry entry;
> +	struct efivar_entry *entry;
>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>  
> -	if (size > sizeof(entry.var.Data))
> +	if (size > sizeof(entry->var.Data)) {
>  		pr_err("value is too large");
> +		return -1;
> +	}
>  
> -	efibc_str_to_str16(name, entry.var.VariableName);
> -	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
> -	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		pr_err("failed to allocate efivar entry");
> +		return -1;
> +	}
>  
> -	ret = efivar_entry_set(&entry,
> +	efibc_str_to_str16(name, entry->var.VariableName);
> +	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> +	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
> +
> +	ret = efivar_entry_set(entry,
>  			       EFI_VARIABLE_NON_VOLATILE
>  			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
>  			       | EFI_VARIABLE_RUNTIME_ACCESS,
> -			       size, entry.var.Data, NULL);
> +			       size, entry->var.Data, NULL);
>  	if (ret)
>  		pr_err("failed to set %s EFI variable: 0x%x\n",
>  		       name, ret);
> +
> +	kfree(entry);
> +	return ret;
>  }
>  
>  static int efibc_reboot_notifier_call(struct notifier_block *notifier,
>  				      unsigned long event, void *data)
>  {
>  	const char *reason = "shutdown";
> +	int ret;
>  
>  	if (event == SYS_RESTART)
>  		reason = "reboot";
>  
> -	efibc_set_variable("LoaderEntryRebootReason", reason);
> -
> -	if (!data)
> -		return NOTIFY_DONE;
> +	ret = efibc_set_variable("LoaderEntryRebootReason", reason);
> +	if (ret || !data)
> + 		return NOTIFY_DONE;
>  
>  	efibc_set_variable("LoaderEntryOneShot", (char *)data);

Hm, can reboot notifiers do non-atomic allocations?

Why is efivar_entry so huge?

Thanks,

	Ingo

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

#1391581

Ingo Molnar <mingo@kernel.org> writes:

> * Compostella, Jeremy <jeremy.compostella@intel.com> wrote:
>
>> -static void efibc_set_variable(const char *name, const char *value)
>> +static int efibc_set_variable(const char *name, const char *value)
>>  {
>>  	int ret;
>>  	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
>> -	struct efivar_entry entry;
>> +	struct efivar_entry *entry;
>>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>>  
>> -	if (size > sizeof(entry.var.Data))
>> +	if (size > sizeof(entry->var.Data)) {
>>  		pr_err("value is too large");
>> +		return -1;
>> +	}
>>  
>> -	efibc_str_to_str16(name, entry.var.VariableName);
>> -	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
>> -	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
>> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry) {
>> +		pr_err("failed to allocate efivar entry");
>> +		return -1;
>> +	}
>>  
>> -	ret = efivar_entry_set(&entry,
>> +	efibc_str_to_str16(name, entry->var.VariableName);
>> +	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
>> +	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
>> +
>> +	ret = efivar_entry_set(entry,
>>  			       EFI_VARIABLE_NON_VOLATILE
>>  			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
>>  			       | EFI_VARIABLE_RUNTIME_ACCESS,
>> -			       size, entry.var.Data, NULL);
>> +			       size, entry->var.Data, NULL);
>>  	if (ret)
>>  		pr_err("failed to set %s EFI variable: 0x%x\n",
>>  		       name, ret);
>> +
>> +	kfree(entry);
>> +	return ret;
>>  }
>>  
>>  static int efibc_reboot_notifier_call(struct notifier_block *notifier,
>>  				      unsigned long event, void *data)
>>  {
>>  	const char *reason = "shutdown";
>> +	int ret;
>>  
>>  	if (event == SYS_RESTART)
>>  		reason = "reboot";
>>  
>> -	efibc_set_variable("LoaderEntryRebootReason", reason);
>> -
>> -	if (!data)
>> -		return NOTIFY_DONE;
>> +	ret = efibc_set_variable("LoaderEntryRebootReason", reason);
>> +	if (ret || !data)
>> + 		return NOTIFY_DONE;
>>  
>>  	efibc_set_variable("LoaderEntryOneShot", (char *)data);
>
> Hm, can reboot notifiers do non-atomic allocations?
The reboot notifier chain is a blocking notifier chain. AFAIK, it
allows non-atomic allocation, right ?

> Why is efivar_entry so huge?
efivar_entry structure include two "big" arrays of 1024 bytes each for
the EFI variable name and data.

Thanks,

Jérémy
-- 
One Emacs to rule them all

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

#1391671

On Sat, 30 Apr, at 10:33:32AM, Jeremy Compostella wrote:
> Ingo Molnar <mingo@kernel.org> writes:
> >
> > Hm, can reboot notifiers do non-atomic allocations?
> The reboot notifier chain is a blocking notifier chain. AFAIK, it
> allows non-atomic allocation, right ?
 
I would assume so, yes.

> > Why is efivar_entry so huge?
> efivar_entry structure include two "big" arrays of 1024 bytes each for
> the EFI variable name and data.

Yeah. This is partially historical in that the EFI variable code has
always had these arrays.

But it's also that if you're dealing with EFI variables you always
want someplace to cache the data and name, so no one has ever proposed
to split out the data/name from struct efivar_entry.

We've never put them on the stack before either ;)

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

#1391669

On Fri, 29 Apr, at 03:53:39PM, Jeremy Compostella wrote:
> From ef3a2941769e59b11d1ec36117209dc4c90c7cf9 Mon Sep 17 00:00:00 2001
> From: Jeremy Compostella <jeremy.compostella@intel.com>
> Date: Fri, 29 Apr 2016 15:29:59 +0200
> Subject: [PATCH] efibc: fix excessive stack footprint warning
> 
> Use dynamic memory allocation instead of stack memory for the entry
> object.
> 
> This patch also fixes a potential buffer override.
> 
> Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
> ---
>  drivers/firmware/efi/efibc.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
> index 2e0c7cc..dd6d1a6 100644
> --- a/drivers/firmware/efi/efibc.c
> +++ b/drivers/firmware/efi/efibc.c
> @@ -17,6 +17,7 @@
>  #include <linux/efi.h>
>  #include <linux/module.h>
>  #include <linux/reboot.h>
> +#include <linux/slab.h>
>  
>  static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>  {
> @@ -28,42 +29,53 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>  	str16[i] = '\0';
>  }
>  
> -static void efibc_set_variable(const char *name, const char *value)
> +static int efibc_set_variable(const char *name, const char *value)
>  {
>  	int ret;
>  	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
> -	struct efivar_entry entry;
> +	struct efivar_entry *entry;
>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>  
> -	if (size > sizeof(entry.var.Data))
> +	if (size > sizeof(entry->var.Data)) {
>  		pr_err("value is too large");
> +		return -1;
> +	}

This isn't right. The usual return value for this scenario would be
either -ENOMEM or -EINVAL. Personally I'd lean towards -EINVAL.

>  
> -	efibc_str_to_str16(name, entry.var.VariableName);
> -	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
> -	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		pr_err("failed to allocate efivar entry");
> +		return -1;
> +	}

This should be -ENOMEM.

>  
> -	ret = efivar_entry_set(&entry,
> +	efibc_str_to_str16(name, entry->var.VariableName);
> +	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> +	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
> +
> +	ret = efivar_entry_set(entry,
>  			       EFI_VARIABLE_NON_VOLATILE
>  			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
>  			       | EFI_VARIABLE_RUNTIME_ACCESS,
> -			       size, entry.var.Data, NULL);
> +			       size, entry->var.Data, NULL);
>  	if (ret)
>  		pr_err("failed to set %s EFI variable: 0x%x\n",
>  		       name, ret);
> +
> +	kfree(entry);
> +	return ret;
>  }
>  
>  static int efibc_reboot_notifier_call(struct notifier_block *notifier,
>  				      unsigned long event, void *data)
>  {
>  	const char *reason = "shutdown";
> +	int ret;
>  
>  	if (event == SYS_RESTART)
>  		reason = "reboot";
>  
> -	efibc_set_variable("LoaderEntryRebootReason", reason);
> -
> -	if (!data)
> -		return NOTIFY_DONE;
> +	ret = efibc_set_variable("LoaderEntryRebootReason", reason);
> +	if (ret || !data)
> + 		return NOTIFY_DONE;
>  
>  	efibc_set_variable("LoaderEntryOneShot", (char *)data);

You need to check this return value too now.

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

#1392043

[Multipart message — attachments visible in raw view] — view raw

Matt Fleming <matt@codeblueprint.co.uk> writes:

> On Fri, 29 Apr, at 03:53:39PM, Jeremy Compostella wrote:
>> From ef3a2941769e59b11d1ec36117209dc4c90c7cf9 Mon Sep 17 00:00:00 2001
>> From: Jeremy Compostella <jeremy.compostella@intel.com>
>> Date: Fri, 29 Apr 2016 15:29:59 +0200
>> Subject: [PATCH] efibc: fix excessive stack footprint warning
>> 
>> Use dynamic memory allocation instead of stack memory for the entry
>> object.
>> 
>> This patch also fixes a potential buffer override.
>> 
>> Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
>> ---
>>  drivers/firmware/efi/efibc.c | 36 ++++++++++++++++++++++++------------
>>  1 file changed, 24 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
>> index 2e0c7cc..dd6d1a6 100644
>> --- a/drivers/firmware/efi/efibc.c
>> +++ b/drivers/firmware/efi/efibc.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/efi.h>
>>  #include <linux/module.h>
>>  #include <linux/reboot.h>
>> +#include <linux/slab.h>
>>  
>>  static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>>  {
>> @@ -28,42 +29,53 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>>  	str16[i] = '\0';
>>  }
>>  
>> -static void efibc_set_variable(const char *name, const char *value)
>> +static int efibc_set_variable(const char *name, const char *value)
>>  {
>>  	int ret;
>>  	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
>> -	struct efivar_entry entry;
>> +	struct efivar_entry *entry;
>>  	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>>  
>> -	if (size > sizeof(entry.var.Data))
>> +	if (size > sizeof(entry->var.Data)) {
>>  		pr_err("value is too large");
>> +		return -1;
>> +	}
>
> This isn't right. The usual return value for this scenario would be
> either -ENOMEM or -EINVAL. Personally I'd lean towards -EINVAL.
Right. Done.

>>  
>> -	efibc_str_to_str16(name, entry.var.VariableName);
>> -	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
>> -	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
>> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry) {
>> +		pr_err("failed to allocate efivar entry");
>> +		return -1;
>> +	}
>
> This should be -ENOMEM.
Done.

>>  
>> -	ret = efivar_entry_set(&entry,
>> +	efibc_str_to_str16(name, entry->var.VariableName);
>> +	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
>> +	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
>> +
>> +	ret = efivar_entry_set(entry,
>>  			       EFI_VARIABLE_NON_VOLATILE
>>  			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
>>  			       | EFI_VARIABLE_RUNTIME_ACCESS,
>> -			       size, entry.var.Data, NULL);
>> +			       size, entry->var.Data, NULL);
>>  	if (ret)
>>  		pr_err("failed to set %s EFI variable: 0x%x\n",
>>  		       name, ret);
>> +
>> +	kfree(entry);
>> +	return ret;
>>  }
>>  
>>  static int efibc_reboot_notifier_call(struct notifier_block *notifier,
>>  				      unsigned long event, void *data)
>>  {
>>  	const char *reason = "shutdown";
>> +	int ret;
>>  
>>  	if (event == SYS_RESTART)
>>  		reason = "reboot";
>>  
>> -	efibc_set_variable("LoaderEntryRebootReason", reason);
>> -
>> -	if (!data)
>> -		return NOTIFY_DONE;
>> +	ret = efibc_set_variable("LoaderEntryRebootReason", reason);
>> +	if (ret || !data)
>> + 		return NOTIFY_DONE;
>>  
>>  	efibc_set_variable("LoaderEntryOneShot", (char *)data);
>
> You need to check this return value too now.
We are leaving this function returning NOTIFY_DONE whatever the return
value of that last efibc_set_variable() call.

Please find the updated patch in attachment.

Also, thank you for answering Ard's questions.  I promised I won't
look at my emails on Labor day ;)

Jérémy

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

#1393469

On Mon, 02 May, at 09:56:09AM, Jeremy Compostella wrote:
> 
> Please find the updated patch in attachment.

Thanks Jeremy. Applied.

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

#1391772

On 29 April 2016 at 11:53, Ingo Molnar <mingo@kernel.org> wrote:
>
> * tip-bot for Compostella, Jeremy <tipbot@zytor.com> wrote:
>
>> Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> Author:     Compostella, Jeremy <jeremy.compostella@intel.com>
>> AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> CommitDate: Thu, 28 Apr 2016 11:34:02 +0200
>>
>> efibc: Add EFI Bootloader Control module
>>
>> This module installs a reboot callback, such that if reboot() is invoked
>> with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
>> EFI variable, to be read by the bootloader.
>

Apologies for only mentioning this now, but I wonder why we need this
in the kernel in the first place? The UEFI spec defines 'BootNext' as
the way to set the boot entry for the next boot only, and this could
also be set from userland. (i.e., the agent that knows how to invoke
reboot with this particular string argument could simply set the EFI
variable from userspace)

As far as reboot/shutdown is concerned, there is a ResetSystem()
runtime service which reboots via the UEFI firmware, which seems like
a better place to record the reboot/shutdown value.

So what bootloader is this exactly that requires this?

Thanks,
Ard.


>>  drivers/firmware/efi/Kconfig  |  15 +++++++
>>  drivers/firmware/efi/Makefile |   1 +
>>  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/efi.h           |   4 ++
>>  4 files changed, 121 insertions(+)
>
> So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this
> new warning:
>
>   drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> 2K of stack use for a function is quite excessive, can we improve the stack
> footprint of this code?
>
> Thanks,
>
>         Ingo

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

#1391815

On Sun, 01 May, at 10:03:55AM, Ard Biesheuvel wrote:
> 
> Apologies for only mentioning this now, but I wonder why we need this
> in the kernel in the first place? The UEFI spec defines 'BootNext' as
> the way to set the boot entry for the next boot only, and this could
> also be set from userland. (i.e., the agent that knows how to invoke
> reboot with this particular string argument could simply set the EFI
> variable from userspace)
 
Right, I raised this general point when Stefan sent the patch
originally. Specifically, I asked why this can't be solved entirely in
userspace and Stefan replied,

  : It is possible, but that means modifying those userspace apps :)
  : There are reboot implementations that do "reboot <reason>", such as
  : Android's reboot command [1] and Upstart's reboot replacement [2], which
  : pass the reason as an argument to the reboot syscall.
  : Probably your first question will be - "Why don't you modify those
  : apps?" Well, I don't see platform-agnostic way how those could be
  : modified to pass the reason to the bootloader, regardless of platform or
  : bootloader.

The difference with BootNext is that setting BootNext requires smarts
in the OS to pick the next boot entry. This driver allows the boot
loader to make that call.

> As far as reboot/shutdown is concerned, there is a ResetSystem()
> runtime service which reboots via the UEFI firmware, which seems like
> a better place to record the reboot/shutdown value.
 
But remember, on x86 we rarely use ResetSystem() because of the
firmware bugs we've hit in the past. Users may want this driver even
though ResetSystem() isn't used to reboot the machine.

> So what bootloader is this exactly that requires this?

I think gummiboot and the android boot loader can make use of it, but
none require it.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web