efivars: Work around serious firmware bugs

- Allow disabling use as a pstore backend
- Add module parameter to disable use as a pstore backend
  * [x86] Set EFI_VARS_PSTORE_DEFAULT_DISABLE=y
- explicitly calculate length of VariableName
- Handle duplicate names from get_next_variable()

Also apply another fix entangled with them:
- efi_pstore: Introducing workqueue updating sysfs

svn path=/dists/sid/linux/; revision=19937
This commit is contained in:
Ben Hutchings 2013-03-22 20:15:09 +00:00
parent 9a503ad42c
commit 2ea813fe8d
10 changed files with 689 additions and 1 deletions

9
debian/changelog vendored
View File

@ -96,6 +96,13 @@ linux (3.2.41-1) unstable; urgency=low
* efi: Ensure efivars is loaded on EFI systems (Closes: #703363)
- [x86] Use a platform device to trigger loading of efivars
- [ia64] Change EFI_VARS from module to built-in
* efivars: Work around serious firmware bugs
- Allow disabling use as a pstore backend
- Add module parameter to disable use as a pstore backend
* [x86] Set EFI_VARS_PSTORE_DEFAULT_DISABLE=y
- explicitly calculate length of VariableName
- Handle duplicate names from get_next_variable()
* efi_pstore: Introducing workqueue updating sysfs
* kmsg_dump: Only dump kernel log in error cases (Closes: #703386)
- kexec: remove KMSG_DUMP_KEXEC
- kmsg_dump: don't run on non-error paths by default
@ -118,7 +125,7 @@ linux (3.2.41-1) unstable; urgency=low
module, as we now have a real fix for #701784
* [rt] Update to 3.2.40-rt60
-- Ben Hutchings <ben@decadent.org.uk> Fri, 22 Mar 2013 18:24:24 +0000
-- Ben Hutchings <ben@decadent.org.uk> Fri, 22 Mar 2013 19:38:42 +0000
linux (3.2.39-2) unstable; urgency=high

View File

@ -132,6 +132,7 @@ CONFIG_IPMI_POWEROFF=m
## file: drivers/firmware/Kconfig
##
CONFIG_EFI_VARS=y
CONFIG_EFI_VARS_PSTORE=y
CONFIG_DMIID=y
##

View File

@ -384,6 +384,9 @@ CONFIG_EDAC_AMD8111=m
CONFIG_EDD=m
# CONFIG_EDD_OFF is not set
CONFIG_EFI_VARS=m
CONFIG_EFI_VARS_PSTORE=y
#. Runtime-disabled by default
CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE=y
CONFIG_DELL_RBU=m
CONFIG_DCDBAS=m
CONFIG_DMIID=y

View File

@ -0,0 +1,176 @@
From: Seiji Aguchi <seiji.aguchi@hds.com>
Date: Tue, 12 Feb 2013 13:04:41 -0800
Subject: efi_pstore: Introducing workqueue updating sysfs
commit a93bc0c6e07ed9bac44700280e65e2945d864fd4 upstream.
[Problem]
efi_pstore creates sysfs entries, which enable users to access to NVRAM,
in a write callback. If a kernel panic happens in an interrupt context,
it may fail because it could sleep due to dynamic memory allocations during
creating sysfs entries.
[Patch Description]
This patch removes sysfs operations from a write callback by introducing
a workqueue updating sysfs entries which is scheduled after the write
callback is called.
Also, the workqueue is kicked in a just oops case.
A system will go down in other cases such as panic, clean shutdown and emergency
restart. And we don't need to create sysfs entries because there is no chance for
users to access to them.
efi_pstore will be robust against a kernel panic in an interrupt context with this patch.
Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
Acked-by: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
[bwh: Backported to 3.2:
- Adjust contest
- Don't check reason in efi_pstore_write(), as it is not given as a
parameter
- Move up declaration of __efivars]
---
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -128,6 +128,8 @@ struct efivar_attribute {
ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
};
+static struct efivars __efivars;
+
#define PSTORE_EFI_ATTRIBUTES \
(EFI_VARIABLE_NON_VOLATILE | \
EFI_VARIABLE_BOOTSERVICE_ACCESS | \
@@ -152,6 +154,13 @@ efivar_create_sysfs_entry(struct efivars
efi_char16_t *variable_name,
efi_guid_t *vendor_guid);
+/*
+ * Prototype for workqueue functions updating sysfs entry
+ */
+
+static void efivar_update_sysfs_entries(struct work_struct *);
+static DECLARE_WORK(efivar_work, efivar_update_sysfs_entries);
+
/* Return the number of unicode characters in data */
static unsigned long
utf16_strnlen(efi_char16_t *s, size_t maxlength)
@@ -834,11 +843,7 @@ static int efi_pstore_write(enum pstore_
if (found)
efivar_unregister(found);
- if (size)
- ret = efivar_create_sysfs_entry(efivars,
- utf16_strsize(efi_name,
- DUMP_NAME_LEN * 2),
- efi_name, &vendor);
+ schedule_work(&efivar_work);
*id = part;
return ret;
@@ -1017,6 +1022,75 @@ static ssize_t efivar_delete(struct file
return count;
}
+static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
+{
+ struct efivar_entry *entry, *n;
+ struct efivars *efivars = &__efivars;
+ unsigned long strsize1, strsize2;
+ bool found = false;
+
+ strsize1 = utf16_strsize(variable_name, 1024);
+ list_for_each_entry_safe(entry, n, &efivars->list, list) {
+ strsize2 = utf16_strsize(entry->var.VariableName, 1024);
+ if (strsize1 == strsize2 &&
+ !memcmp(variable_name, &(entry->var.VariableName),
+ strsize2) &&
+ !efi_guidcmp(entry->var.VendorGuid,
+ *vendor)) {
+ found = true;
+ break;
+ }
+ }
+ return found;
+}
+
+static void efivar_update_sysfs_entries(struct work_struct *work)
+{
+ struct efivars *efivars = &__efivars;
+ efi_guid_t vendor;
+ efi_char16_t *variable_name;
+ unsigned long variable_name_size = 1024;
+ efi_status_t status = EFI_NOT_FOUND;
+ bool found;
+
+ /* Add new sysfs entries */
+ while (1) {
+ variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+ if (!variable_name) {
+ pr_err("efivars: Memory allocation failed.\n");
+ return;
+ }
+
+ spin_lock_irq(&efivars->lock);
+ found = false;
+ while (1) {
+ variable_name_size = 1024;
+ status = efivars->ops->get_next_variable(
+ &variable_name_size,
+ variable_name,
+ &vendor);
+ if (status != EFI_SUCCESS) {
+ break;
+ } else {
+ if (!variable_is_present(variable_name,
+ &vendor)) {
+ found = true;
+ break;
+ }
+ }
+ }
+ spin_unlock_irq(&efivars->lock);
+
+ if (!found) {
+ kfree(variable_name);
+ break;
+ } else
+ efivar_create_sysfs_entry(efivars,
+ variable_name_size,
+ variable_name, &vendor);
+ }
+}
+
/*
* Let's not leave out systab information that snuck into
* the efivars driver
@@ -1273,7 +1347,6 @@ out:
}
EXPORT_SYMBOL_GPL(register_efivars);
-static struct efivars __efivars;
static struct efivar_operations ops;
/*
@@ -1331,6 +1404,8 @@ err_put:
static void __exit
efivars_exit(void)
{
+ cancel_work_sync(&efivar_work);
+
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
unregister_efivars(&__efivars);
kobject_put(efi_kobj);
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -620,7 +620,8 @@ struct efivars {
* 1) ->list - adds, removals, reads, writes
* 2) ops.[gs]et_variable() calls.
* It must not be held when creating sysfs entries or calling kmalloc.
- * ops.get_next_variable() is only called from register_efivars(),
+ * ops.get_next_variable() is only called from register_efivars()
+ * or efivar_update_sysfs_entries(),
* which is protected by the BKL, so that path is safe.
*/
spinlock_t lock;

View File

@ -0,0 +1,72 @@
From: Seth Forshee <seth.forshee@canonical.com>
Date: Mon, 11 Mar 2013 16:17:50 -0500
Subject: efivars: Add module parameter to disable use as a pstore backend
commit ec0971ba5372a4dfa753f232449d23a8fd98490e upstream.
We know that with some firmware implementations writing too much data to
UEFI variables can lead to bricking machines. Recent changes attempt to
address this issue, but for some it may still be prudent to avoid
writing large amounts of data until the solution has been proven on a
wide variety of hardware.
Crash dumps or other data from pstore can potentially be a large data
source. Add a pstore_module parameter to efivars to allow disabling its
use as a backend for pstore. Also add a config option,
CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE, to allow setting the default
value of this paramter to true (i.e. disabled by default).
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[bwh: Backported to 3.2: adjust context]
---
drivers/firmware/Kconfig | 9 +++++++++
drivers/firmware/efivars.c | 8 +++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -62,6 +62,15 @@ config EFI_VARS_PSTORE
will allow writing console messages, crash dumps, or anything
else supported by pstore to EFI variables.
+config EFI_VARS_PSTORE_DEFAULT_DISABLE
+ bool "Disable using efivars as a pstore backend by default"
+ depends on EFI_VARS_PSTORE
+ default n
+ help
+ Saying Y here will disable the use of efivars as a storage
+ backend for pstore by default. This setting can be overridden
+ using the efivars module's pstore_disable parameter.
+
config EFI_PCDP
bool "Console device selection via EFI PCDP or HCDP table"
depends on ACPI && EFI && IA64
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -93,6 +93,11 @@ MODULE_ALIAS("platform:efivars");
#define DUMP_NAME_LEN 52
+static bool efivars_pstore_disable =
+ IS_ENABLED(EFI_VARS_PSTORE_DEFAULT_DISABLE);
+
+module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
+
/*
* The maximum size of VariableName + Data = 1024
* Therefore, it's reasonable to save that much
@@ -1258,7 +1263,8 @@ int register_efivars(struct efivars *efi
if (error)
unregister_efivars(efivars);
- efivar_pstore_register(efivars);
+ if (!efivars_pstore_disable)
+ efivar_pstore_register(efivars);
out:
kfree(variable_name);

View File

@ -0,0 +1,142 @@
From: Seth Forshee <seth.forshee@canonical.com>
Date: Thu, 7 Mar 2013 11:40:17 -0600
Subject: efivars: Allow disabling use as a pstore backend
commit ed9dc8ce7a1c8115dba9483a9b51df8b63a2e0ef upstream.
Add a new option, CONFIG_EFI_VARS_PSTORE, which can be set to N to
avoid using efivars as a backend to pstore, as some users may want to
compile out the code completely.
Set the default to Y to maintain backwards compatability, since this
feature has always been enabled until now.
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[bwh: Backported to 3.2: adjust context]
---
drivers/firmware/Kconfig | 9 +++++++
drivers/firmware/efivars.c | 64 ++++++++++++++------------------------------
2 files changed, 29 insertions(+), 44 deletions(-)
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -53,6 +53,15 @@ config EFI_VARS
Subsequent efibootmgr releases may be found at:
<http://linux.dell.com/efibootmgr>
+config EFI_VARS_PSTORE
+ bool "Register efivars backend for pstore"
+ depends on EFI_VARS && PSTORE
+ default y
+ help
+ Say Y here to enable use efivars as a backend to pstore. This
+ will allow writing console messages, crash dumps, or anything
+ else supported by pstore to EFI variables.
+
config EFI_PCDP
bool "Console device selection via EFI PCDP or HCDP table"
depends on ACPI && EFI && IA64
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -660,8 +660,6 @@ static struct kobj_type efivar_ktype = {
.default_attrs = def_attrs,
};
-static struct pstore_info efi_pstore_info;
-
static inline void
efivar_unregister(struct efivar_entry *var)
{
@@ -698,7 +696,7 @@ static int efi_status_to_err(efi_status_
return err;
}
-#ifdef CONFIG_PSTORE
+#ifdef CONFIG_EFI_VARS_PSTORE
static int efi_pstore_open(struct pstore_info *psi)
{
@@ -848,36 +846,6 @@ static int efi_pstore_erase(enum pstore_
return 0;
}
-#else
-static int efi_pstore_open(struct pstore_info *psi)
-{
- return 0;
-}
-
-static int efi_pstore_close(struct pstore_info *psi)
-{
- return 0;
-}
-
-static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
- struct timespec *timespec,
- char **buf, struct pstore_info *psi)
-{
- return -1;
-}
-
-static int efi_pstore_write(enum pstore_type_id type, u64 *id,
- unsigned int part, size_t size, struct pstore_info *psi)
-{
- return 0;
-}
-
-static int efi_pstore_erase(enum pstore_type_id type, u64 id,
- struct pstore_info *psi)
-{
- return 0;
-}
-#endif
static struct pstore_info efi_pstore_info = {
.owner = THIS_MODULE,
@@ -889,6 +857,24 @@ static struct pstore_info efi_pstore_inf
.erase = efi_pstore_erase,
};
+static void efivar_pstore_register(struct efivars *efivars)
+{
+ efivars->efi_pstore_info = efi_pstore_info;
+ efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+ if (efivars->efi_pstore_info.buf) {
+ efivars->efi_pstore_info.bufsize = 1024;
+ efivars->efi_pstore_info.data = efivars;
+ spin_lock_init(&efivars->efi_pstore_info.buf_lock);
+ pstore_register(&efivars->efi_pstore_info);
+ }
+}
+#else
+static void efivar_pstore_register(struct efivars *efivars)
+{
+ return;
+}
+#endif
+
static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr,
char *buf, loff_t pos, size_t count)
@@ -1272,15 +1258,7 @@ int register_efivars(struct efivars *efi
if (error)
unregister_efivars(efivars);
- efivars->efi_pstore_info = efi_pstore_info;
-
- efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
- if (efivars->efi_pstore_info.buf) {
- efivars->efi_pstore_info.bufsize = 1024;
- efivars->efi_pstore_info.data = efivars;
- spin_lock_init(&efivars->efi_pstore_info.buf_lock);
- pstore_register(&efivars->efi_pstore_info);
- }
+ efivar_pstore_register(efivars);
out:
kfree(variable_name);

View File

@ -0,0 +1,24 @@
From: Ben Hutchings <ben@decadent.org.uk>
Date: Fri, 22 Mar 2013 19:43:53 +0000
Subject: efivars: Fix check for CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
The 'CONFIG_' prefix is not implicit in IS_ENABLED().
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: <stable@vger.kernel.org>
---
drivers/firmware/efivars.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -94,7 +94,7 @@ MODULE_ALIAS("platform:efivars");
#define DUMP_NAME_LEN 52
static bool efivars_pstore_disable =
- IS_ENABLED(EFI_VARS_PSTORE_DEFAULT_DISABLE);
+ IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE);
module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);

View File

@ -0,0 +1,156 @@
From: Matt Fleming <matt.fleming@intel.com>
Date: Thu, 7 Mar 2013 11:59:14 +0000
Subject: efivars: Handle duplicate names from get_next_variable()
commit e971318bbed610e28bb3fde9d548e6aaf0a6b02e upstream.
Some firmware exhibits a bug where the same VariableName and
VendorGuid values are returned on multiple invocations of
GetNextVariableName(). See,
https://bugzilla.kernel.org/show_bug.cgi?id=47631
As a consequence of such a bug, Andre reports hitting the following
WARN_ON() in the sysfs code after updating the BIOS on his, "Gigabyte
Technology Co., Ltd. To be filled by O.E.M./Z77X-UD3H, BIOS F19e
11/21/2012)" machine,
[ 0.581554] EFI Variables Facility v0.08 2004-May-17
[ 0.584914] ------------[ cut here ]------------
[ 0.585639] WARNING: at /home/andre/linux/fs/sysfs/dir.c:536 sysfs_add_one+0xd4/0x100()
[ 0.586381] Hardware name: To be filled by O.E.M.
[ 0.587123] sysfs: cannot create duplicate filename '/firmware/efi/vars/SbAslBufferPtrVar-01f33c25-764d-43ea-aeea-6b5a41f3f3e8'
[ 0.588694] Modules linked in:
[ 0.589484] Pid: 1, comm: swapper/0 Not tainted 3.8.0+ #7
[ 0.590280] Call Trace:
[ 0.591066] [<ffffffff81208954>] ? sysfs_add_one+0xd4/0x100
[ 0.591861] [<ffffffff810587bf>] warn_slowpath_common+0x7f/0xc0
[ 0.592650] [<ffffffff810588bc>] warn_slowpath_fmt+0x4c/0x50
[ 0.593429] [<ffffffff8134dd85>] ? strlcat+0x65/0x80
[ 0.594203] [<ffffffff81208954>] sysfs_add_one+0xd4/0x100
[ 0.594979] [<ffffffff81208b78>] create_dir+0x78/0xd0
[ 0.595753] [<ffffffff81208ec6>] sysfs_create_dir+0x86/0xe0
[ 0.596532] [<ffffffff81347e4c>] kobject_add_internal+0x9c/0x220
[ 0.597310] [<ffffffff81348307>] kobject_init_and_add+0x67/0x90
[ 0.598083] [<ffffffff81584a71>] ? efivar_create_sysfs_entry+0x61/0x1c0
[ 0.598859] [<ffffffff81584b2b>] efivar_create_sysfs_entry+0x11b/0x1c0
[ 0.599631] [<ffffffff8158517e>] register_efivars+0xde/0x420
[ 0.600395] [<ffffffff81d430a7>] ? edd_init+0x2f5/0x2f5
[ 0.601150] [<ffffffff81d4315f>] efivars_init+0xb8/0x104
[ 0.601903] [<ffffffff8100215a>] do_one_initcall+0x12a/0x180
[ 0.602659] [<ffffffff81d05d80>] kernel_init_freeable+0x13e/0x1c6
[ 0.603418] [<ffffffff81d05586>] ? loglevel+0x31/0x31
[ 0.604183] [<ffffffff816a6530>] ? rest_init+0x80/0x80
[ 0.604936] [<ffffffff816a653e>] kernel_init+0xe/0xf0
[ 0.605681] [<ffffffff816ce7ec>] ret_from_fork+0x7c/0xb0
[ 0.606414] [<ffffffff816a6530>] ? rest_init+0x80/0x80
[ 0.607143] ---[ end trace 1609741ab737eb29 ]---
There's not much we can do to work around and keep traversing the
variable list once we hit this firmware bug. Our only solution is to
terminate the loop because, as Lingzhu reports, some machines get
stuck when they encounter duplicate names,
> I had an IBM System x3100 M4 and x3850 X5 on which kernel would
> get stuck in infinite loop creating duplicate sysfs files because,
> for some reason, there are several duplicate boot entries in nvram
> getting GetNextVariableName into a circle of iteration (with
> period > 2).
Also disable the workqueue, as efivar_update_sysfs_entries() uses
GetNextVariableName() to figure out which variables have been created
since the last iteration. That algorithm isn't going to work if
GetNextVariableName() returns duplicates. Note that we don't disable
EFI variable creation completely on the affected machines, it's just
that any pstore dump-* files won't appear in sysfs until the next
boot.
Reported-by: Andre Heider <a.heider@gmail.com>
Reported-by: Lingzhu Xiang <lxiang@redhat.com>
Tested-by: Lingzhu Xiang <lxiang@redhat.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[bwh: Backported to 3.2: reason is not checked in efi_pstore_write()]
---
drivers/firmware/efivars.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -160,6 +160,7 @@ efivar_create_sysfs_entry(struct efivars
static void efivar_update_sysfs_entries(struct work_struct *);
static DECLARE_WORK(efivar_work, efivar_update_sysfs_entries);
+static bool efivar_wq_enabled = true;
/* Return the number of unicode characters in data */
static unsigned long
@@ -843,7 +844,8 @@ static int efi_pstore_write(enum pstore_
if (found)
efivar_unregister(found);
- schedule_work(&efivar_work);
+ if (efivar_wq_enabled)
+ schedule_work(&efivar_work);
*id = part;
return ret;
@@ -1306,6 +1308,35 @@ void unregister_efivars(struct efivars *
}
EXPORT_SYMBOL_GPL(unregister_efivars);
+/*
+ * Print a warning when duplicate EFI variables are encountered and
+ * disable the sysfs workqueue since the firmware is buggy.
+ */
+static void dup_variable_bug(efi_char16_t *s16, efi_guid_t *vendor_guid,
+ unsigned long len16)
+{
+ size_t i, len8 = len16 / sizeof(efi_char16_t);
+ char *s8;
+
+ /*
+ * Disable the workqueue since the algorithm it uses for
+ * detecting new variables won't work with this buggy
+ * implementation of GetNextVariableName().
+ */
+ efivar_wq_enabled = false;
+
+ s8 = kzalloc(len8, GFP_KERNEL);
+ if (!s8)
+ return;
+
+ for (i = 0; i < len8; i++)
+ s8[i] = s16[i];
+
+ printk(KERN_WARNING "efivars: duplicate variable: %s-%pUl\n",
+ s8, vendor_guid);
+ kfree(s8);
+}
+
int register_efivars(struct efivars *efivars,
const struct efivar_operations *ops,
struct kobject *parent_kobj)
@@ -1348,6 +1379,22 @@ int register_efivars(struct efivars *efi
case EFI_SUCCESS:
variable_name_size = var_name_strnsize(variable_name,
variable_name_size);
+
+ /*
+ * Some firmware implementations return the
+ * same variable name on multiple calls to
+ * get_next_variable(). Terminate the loop
+ * immediately as there is no guarantee that
+ * we'll ever see a different variable name,
+ * and may end up looping here forever.
+ */
+ if (variable_is_present(variable_name, &vendor_guid)) {
+ dup_variable_bug(variable_name, &vendor_guid,
+ variable_name_size);
+ status = EFI_NOT_FOUND;
+ break;
+ }
+
efivar_create_sysfs_entry(efivars,
variable_name_size,
variable_name,

View File

@ -0,0 +1,101 @@
From: Matt Fleming <matt.fleming@intel.com>
Date: Fri, 1 Mar 2013 14:49:12 +0000
Subject: efivars: explicitly calculate length of VariableName
commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
It's not wise to assume VariableNameSize represents the length of
VariableName, as not all firmware updates VariableNameSize in the same
way (some don't update it at all if EFI_SUCCESS is returned). There
are even implementations out there that update VariableNameSize with
values that are both larger than the string returned in VariableName
and smaller than the buffer passed to GetNextVariableName(), which
resulted in the following bug report from Michael Schroeder,
> On HP z220 system (firmware version 1.54), some EFI variables are
> incorrectly named :
>
> ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
The issue here is that because we blindly use VariableNameSize without
verifying its value, we can potentially read garbage values from the
buffer containing VariableName if VariableNameSize is larger than the
length of VariableName.
Since VariableName is a string, we can calculate its size by searching
for the terminating NULL character.
Reported-by: Frederic Crozat <fcrozat@suse.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Michael Schroeder <mls@suse.com>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Lingzhu Xiang <lxiang@redhat.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
drivers/firmware/efivars.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1044,6 +1044,31 @@ static bool variable_is_present(efi_char
return found;
}
+/*
+ * Returns the size of variable_name, in bytes, including the
+ * terminating NULL character, or variable_name_size if no NULL
+ * character is found among the first variable_name_size bytes.
+ */
+static unsigned long var_name_strnsize(efi_char16_t *variable_name,
+ unsigned long variable_name_size)
+{
+ unsigned long len;
+ efi_char16_t c;
+
+ /*
+ * The variable name is, by definition, a NULL-terminated
+ * string, so make absolutely sure that variable_name_size is
+ * the value we expect it to be. If not, return the real size.
+ */
+ for (len = 2; len <= variable_name_size; len += sizeof(c)) {
+ c = variable_name[(len / sizeof(c)) - 1];
+ if (!c)
+ break;
+ }
+
+ return min(len, variable_name_size);
+}
+
static void efivar_update_sysfs_entries(struct work_struct *work)
{
struct efivars *efivars = &__efivars;
@@ -1084,10 +1109,13 @@ static void efivar_update_sysfs_entries(
if (!found) {
kfree(variable_name);
break;
- } else
+ } else {
+ variable_name_size = var_name_strnsize(variable_name,
+ variable_name_size);
efivar_create_sysfs_entry(efivars,
variable_name_size,
variable_name, &vendor);
+ }
}
}
@@ -1318,6 +1346,8 @@ int register_efivars(struct efivars *efi
&vendor_guid);
switch (status) {
case EFI_SUCCESS:
+ variable_name_size = var_name_strnsize(variable_name,
+ variable_name_size);
efivar_create_sysfs_entry(efivars,
variable_name_size,
variable_name,

View File

@ -627,3 +627,9 @@ bugfix/all/vhost-net-fix-heads-usage-of-ubuf_info.patch
bugfix/all/udf-avoid-info-leak-on-export.patch
bugfix/all/isofs-avoid-info-leak-on-export.patch
debian/dm-avoid-ABI-change-in-3.2.41.patch
bugfix/all/efivars-Allow-disabling-use-as-a-pstore-backend.patch
bugfix/all/efivars-Add-module-parameter-to-disable-use-as-a-pst.patch
bugfix/all/efivars-Fix-check-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch
bugfix/all/efi_pstore-Introducing-workqueue-updating-sysfs.patch
bugfix/all/efivars-explicitly-calculate-length-of-VariableName.patch
bugfix/all/efivars-Handle-duplicate-names-from-get_next_variabl.patch