From 2ea813fe8d37a27de9d55bbce52c600b2540f2d3 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Fri, 22 Mar 2013 20:15:09 +0000 Subject: [PATCH] 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 --- debian/changelog | 9 +- debian/config/ia64/config | 1 + debian/config/kernelarch-x86/config | 3 + ...Introducing-workqueue-updating-sysfs.patch | 176 ++++++++++++++++++ ...le-parameter-to-disable-use-as-a-pst.patch | 72 +++++++ ...ow-disabling-use-as-a-pstore-backend.patch | 142 ++++++++++++++ ...k-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch | 24 +++ ...uplicate-names-from-get_next_variabl.patch | 156 ++++++++++++++++ ...tly-calculate-length-of-VariableName.patch | 101 ++++++++++ debian/patches/series | 6 + 10 files changed, 689 insertions(+), 1 deletion(-) create mode 100644 debian/patches/bugfix/all/efi_pstore-Introducing-workqueue-updating-sysfs.patch create mode 100644 debian/patches/bugfix/all/efivars-Add-module-parameter-to-disable-use-as-a-pst.patch create mode 100644 debian/patches/bugfix/all/efivars-Allow-disabling-use-as-a-pstore-backend.patch create mode 100644 debian/patches/bugfix/all/efivars-Fix-check-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch create mode 100644 debian/patches/bugfix/all/efivars-Handle-duplicate-names-from-get_next_variabl.patch create mode 100644 debian/patches/bugfix/all/efivars-explicitly-calculate-length-of-VariableName.patch diff --git a/debian/changelog b/debian/changelog index 36487df4a..0476071c3 100644 --- a/debian/changelog +++ b/debian/changelog @@ -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 Fri, 22 Mar 2013 18:24:24 +0000 + -- Ben Hutchings Fri, 22 Mar 2013 19:38:42 +0000 linux (3.2.39-2) unstable; urgency=high diff --git a/debian/config/ia64/config b/debian/config/ia64/config index ae58fb760..6d8841eb7 100644 --- a/debian/config/ia64/config +++ b/debian/config/ia64/config @@ -132,6 +132,7 @@ CONFIG_IPMI_POWEROFF=m ## file: drivers/firmware/Kconfig ## CONFIG_EFI_VARS=y +CONFIG_EFI_VARS_PSTORE=y CONFIG_DMIID=y ## diff --git a/debian/config/kernelarch-x86/config b/debian/config/kernelarch-x86/config index 0aadf769b..e9dd1bdf6 100644 --- a/debian/config/kernelarch-x86/config +++ b/debian/config/kernelarch-x86/config @@ -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 diff --git a/debian/patches/bugfix/all/efi_pstore-Introducing-workqueue-updating-sysfs.patch b/debian/patches/bugfix/all/efi_pstore-Introducing-workqueue-updating-sysfs.patch new file mode 100644 index 000000000..c4caad991 --- /dev/null +++ b/debian/patches/bugfix/all/efi_pstore-Introducing-workqueue-updating-sysfs.patch @@ -0,0 +1,176 @@ +From: Seiji Aguchi +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 +Acked-by: Matt Fleming +Signed-off-by: Tony Luck +[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; diff --git a/debian/patches/bugfix/all/efivars-Add-module-parameter-to-disable-use-as-a-pst.patch b/debian/patches/bugfix/all/efivars-Add-module-parameter-to-disable-use-as-a-pst.patch new file mode 100644 index 000000000..1a94adfde --- /dev/null +++ b/debian/patches/bugfix/all/efivars-Add-module-parameter-to-disable-use-as-a-pst.patch @@ -0,0 +1,72 @@ +From: Seth Forshee +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 +Cc: Josh Boyer +Cc: Matthew Garrett +Cc: Seiji Aguchi +Cc: Tony Luck +Signed-off-by: Matt Fleming +[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); diff --git a/debian/patches/bugfix/all/efivars-Allow-disabling-use-as-a-pstore-backend.patch b/debian/patches/bugfix/all/efivars-Allow-disabling-use-as-a-pstore-backend.patch new file mode 100644 index 000000000..6b3295cd4 --- /dev/null +++ b/debian/patches/bugfix/all/efivars-Allow-disabling-use-as-a-pstore-backend.patch @@ -0,0 +1,142 @@ +From: Seth Forshee +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 +Cc: Josh Boyer +Cc: Matthew Garrett +Cc: Seiji Aguchi +Cc: Tony Luck +Signed-off-by: Matt Fleming +[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: + + ++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); diff --git a/debian/patches/bugfix/all/efivars-Fix-check-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch b/debian/patches/bugfix/all/efivars-Fix-check-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch new file mode 100644 index 000000000..abcd188b5 --- /dev/null +++ b/debian/patches/bugfix/all/efivars-Fix-check-for-CONFIG_EFI_VARS_PSTORE_DEFAULT.patch @@ -0,0 +1,24 @@ +From: Ben Hutchings +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 +Cc: Seth Forshee +Cc: +--- + 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); + diff --git a/debian/patches/bugfix/all/efivars-Handle-duplicate-names-from-get_next_variabl.patch b/debian/patches/bugfix/all/efivars-Handle-duplicate-names-from-get_next_variabl.patch new file mode 100644 index 000000000..f5339a589 --- /dev/null +++ b/debian/patches/bugfix/all/efivars-Handle-duplicate-names-from-get_next_variabl.patch @@ -0,0 +1,156 @@ +From: Matt Fleming +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] [] ? sysfs_add_one+0xd4/0x100 +[ 0.591861] [] warn_slowpath_common+0x7f/0xc0 +[ 0.592650] [] warn_slowpath_fmt+0x4c/0x50 +[ 0.593429] [] ? strlcat+0x65/0x80 +[ 0.594203] [] sysfs_add_one+0xd4/0x100 +[ 0.594979] [] create_dir+0x78/0xd0 +[ 0.595753] [] sysfs_create_dir+0x86/0xe0 +[ 0.596532] [] kobject_add_internal+0x9c/0x220 +[ 0.597310] [] kobject_init_and_add+0x67/0x90 +[ 0.598083] [] ? efivar_create_sysfs_entry+0x61/0x1c0 +[ 0.598859] [] efivar_create_sysfs_entry+0x11b/0x1c0 +[ 0.599631] [] register_efivars+0xde/0x420 +[ 0.600395] [] ? edd_init+0x2f5/0x2f5 +[ 0.601150] [] efivars_init+0xb8/0x104 +[ 0.601903] [] do_one_initcall+0x12a/0x180 +[ 0.602659] [] kernel_init_freeable+0x13e/0x1c6 +[ 0.603418] [] ? loglevel+0x31/0x31 +[ 0.604183] [] ? rest_init+0x80/0x80 +[ 0.604936] [] kernel_init+0xe/0xf0 +[ 0.605681] [] ret_from_fork+0x7c/0xb0 +[ 0.606414] [] ? 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 +Reported-by: Lingzhu Xiang +Tested-by: Lingzhu Xiang +Cc: Seiji Aguchi +Signed-off-by: Matt Fleming +[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, diff --git a/debian/patches/bugfix/all/efivars-explicitly-calculate-length-of-VariableName.patch b/debian/patches/bugfix/all/efivars-explicitly-calculate-length-of-VariableName.patch new file mode 100644 index 000000000..ddfa2ec95 --- /dev/null +++ b/debian/patches/bugfix/all/efivars-explicitly-calculate-length-of-VariableName.patch @@ -0,0 +1,101 @@ +From: Matt Fleming +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 +Cc: Matthew Garrett +Cc: Josh Boyer +Cc: Michael Schroeder +Cc: Lee, Chun-Yi +Cc: Lingzhu Xiang +Cc: Seiji Aguchi +Signed-off-by: Matt Fleming +--- + 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, diff --git a/debian/patches/series b/debian/patches/series index cee49f2df..5696436c5 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -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