EFI variables can be flagged as being accessible only within boot services.
This makes it awkward for us to figure out how much space they use at
runtime. In theory we could figure this out by simply comparing the results
from QueryVariableInfo() to the space used by all of our variables, but
that fails if the platform doesn't garbage collect on every boot. Thankfully,
calling QueryVariableInfo() while still inside boot services gives a more
reliable answer. This patch passes that information from the EFI boot stub
up to the efivars code, letting us calculate a reasonably accurate value.
Signed-off-by: Matthew Garrett <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++
arch/x86/include/asm/efi.h | 10 ++++++++
arch/x86/include/uapi/asm/bootparam.h | 1 +
arch/x86/platform/efi/efi.c | 21 ++++++++++++++++
drivers/firmware/efivars.c | 29 +++++++++++++++++++++
5 files changed, 108 insertions(+)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c205035..8615f75 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
*size = len;
}
+static efi_status_t setup_efi_vars(struct boot_params *params)
+{
+ struct setup_data *data;
+ struct efi_var_bootdata *efidata;
+ u64 store_size, remaining_size, var_size;
+ efi_status_t status;
+
+ if (!sys_table->runtime->query_variable_info)
+ return EFI_UNSUPPORTED;
+
+ data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+
+ while (data && data->next)
+ data = (struct setup_data *)(unsigned long)data->next;
+
+ status = efi_call_phys4(sys_table->runtime->query_variable_info,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
+ &remaining_size, &var_size);
+
+ if (status != EFI_SUCCESS)
+ return status;
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, sizeof(*efidata), &efidata);
+
+ if (status != EFI_SUCCESS)
+ return status;
+
+ efidata->data.type = SETUP_EFI_VARS;
+ efidata->data.len = sizeof(struct efi_var_bootdata) -
+ sizeof(struct setup_data);
+ efidata->data.next = 0;
+ efidata->store_size = store_size;
+ efidata->remaining_size = remaining_size;
+ efidata->max_var_size = var_size;
+
+ if (data)
+ data->next = (unsigned long)efidata;
+ else
+ params->hdr.setup_data = (unsigned long)efidata;
+
+}
+
static efi_status_t setup_efi_pci(struct boot_params *params)
{
efi_pci_io_protocol *pci;
@@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
setup_graphics(boot_params);
+ setup_efi_vars(boot_params);
+
setup_efi_pci(boot_params);
status = efi_call_phys3(sys_table->boottime->allocate_pool,
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 60c89f3..6c3a154 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -93,6 +93,9 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
#endif /* CONFIG_X86_32 */
+extern u64 efi_var_store_size;
+extern u64 efi_var_remaining_size;
+extern u64 efi_var_max_var_size;
extern int add_efi_memmap;
extern unsigned long x86_efi_facility;
extern void efi_set_executable(efi_memory_desc_t *md, bool executable);
@@ -102,6 +105,13 @@ extern void efi_call_phys_epilog(void);
extern void efi_unmap_memmap(void);
extern void efi_memory_uc(u64 addr, unsigned long size);
+struct efi_var_bootdata {
+ struct setup_data data;
+ u64 store_size;
+ u64 remaining_size;
+ u64 max_var_size;
+};
+
#ifdef CONFIG_EFI
static inline bool efi_is_native(void)
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..0874424 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
#define SETUP_E820_EXT 1
#define SETUP_DTB 2
#define SETUP_PCI 3
+#define SETUP_EFI_VARS 4
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 5f2ecaf..cd2f020 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata;
unsigned long x86_efi_facility;
+u64 efi_var_store_size;
+u64 efi_var_remaining_size;
+u64 efi_var_max_var_size;
+
/*
* Returns 1 if 'facility' is enabled, 0 otherwise.
*/
@@ -682,6 +686,9 @@ void __init efi_init(void)
char vendor[100] = "unknown";
int i = 0;
void *tmp;
+ struct setup_data *data;
+ struct efi_var_bootdata *efi_var_data;
+ u64 pa_data;
#ifdef CONFIG_X86_32
if (boot_params.efi_info.efi_systab_hi ||
@@ -699,6 +706,20 @@ void __init efi_init(void)
if (efi_systab_init(efi_phys.systab))
return;
+ pa_data = boot_params.hdr.setup_data;
+ while (pa_data) {
+ data = early_ioremap(pa_data, sizeof(*efi_var_data));
+ if (data->type == SETUP_EFI_VARS) {
+ efi_var_data = (struct efi_var_bootdata *)data;
+
+ efi_var_store_size = efi_var_data->store_size;
+ efi_var_remaining_size = efi_var_data->remaining_size;
+ efi_var_max_var_size = efi_var_data->max_var_size;
+ }
+ pa_data = data->next;
+ early_iounmap(data, sizeof(*efi_var_data));
+ }
+
set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
/*
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7acafb8..60e7d8f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -85,6 +85,7 @@
#include <linux/ramfs.h>
#include <linux/pagemap.h>
+#include <asm/efi.h>
#include <asm/uaccess.h>
#define EFIVARS_VERSION "0.08"
@@ -139,6 +140,8 @@ struct efivar_attribute {
static struct efivars __efivars;
static struct efivar_operations ops;
+static u64 boot_var_size;
+static u64 boot_used_size;
#define PSTORE_EFI_ATTRIBUTES \
(EFI_VARIABLE_NON_VOLATILE | \
@@ -2014,6 +2017,9 @@ int register_efivars(struct efivars *efivars,
efi_char16_t *variable_name;
unsigned long variable_name_size = 1024;
int error = 0;
+ struct efivar_entry *entry;
+ struct efi_variable *var;
+ u64 active_size = 0;
variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
@@ -2086,6 +2092,25 @@ int register_efivars(struct efivars *efivars,
}
} while (status != EFI_NOT_FOUND);
+ if (boot_used_size) {
+ list_for_each_entry(entry, &efivars->list, list) {
+ var = &entry->var;
+ status = get_var_data(efivars, var);
+
+ if (status ||
+ !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
+ continue;
+
+ active_size += var->DataSize;
+ active_size += utf16_strsize(var->VariableName, 1024);
+ }
+
+ if (active_size < boot_used_size)
+ boot_var_size = boot_used_size - active_size;
+ else
+ printk(KERN_WARNING FW_BUG "efivars: Inconsistent initial sizes\n");
+ }
+
error = create_efivars_bin_attributes(efivars);
if (error)
unregister_efivars(efivars);
@@ -2133,6 +2158,10 @@ efivars_init(void)
ops.get_next_variable = efi.get_next_variable;
ops.query_variable_info = efi.query_variable_info;
+#ifdef CONFIG_X86
+ boot_used_size = efi_var_store_size - efi_var_remaining_size;
+#endif
+
error = register_efivars(&__efivars, &ops, efi_kobj);
if (error)
goto err_put;
--
1.8.1.2
EFI implementations distinguish between space that is actively used by a
variable and space that merely hasn't been garbage collected yet. Space
that hasn't yet been garbage collected isn't available for use and so isn't
counted in the remaining_space field returned by QueryVariableInfo().
Combined with commit 68d9298 this can cause problems. Some implementations
don't garbage collect until the remaining space is smaller than the maximum
variable size, and as a result check_var_size() will always fail once more
than 50% of the variable store has been used even if most of that space is
marked as available for garbage collection. The user is unable to create
new variables, and deleting variables doesn't increase the remaining space.
The problem that 68d9298 was attempting to avoid was one where certain
platforms fail if the actively used space is greater than 50% of the
available storage space. We should be able to calculate that by simply
summing the size of each available variable and subtracting that from
the total storage space. With luck this will fix the problem described in
https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
damage to occur to the machines 68d9298 was attempting to fix.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 60e7d8f..a6d8a58 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -439,9 +439,19 @@ static efi_status_t
check_var_size_locked(struct efivars *efivars, u32 attributes,
unsigned long size)
{
- u64 storage_size, remaining_size, max_size;
+ u64 storage_size, remaining_size, max_size, active_available;
+ struct efivar_entry *entry;
+ struct efi_variable *var;
efi_status_t status;
const struct efivar_operations *fops = efivars->ops;
+ unsigned long active_size = 0;
+
+ /*
+ * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit
+ * RAM, not flash, so ignore them.
+ */
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_SUCCESS;
if (!efivars->ops->query_variable_info)
return EFI_UNSUPPORTED;
@@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
if (status != EFI_SUCCESS)
return status;
- if (!storage_size || size > remaining_size || size > max_size ||
- (remaining_size - size) < (storage_size / 2))
+ list_for_each_entry(entry, &efivars->list, list) {
+ var = &entry->var;
+ status = get_var_data_locked(efivars, var);
+
+ if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
+ continue;
+
+ active_size += var->DataSize;
+ active_size += utf16_strsize(var->VariableName, 1024);
+ /*
+ * There's some additional metadata associated with each
+ * variable. Intel's reference implementation is 60 bytes -
+ * bump that to 128 to account for vendor additions and
+ * potential alignment constraints
+ */
+ active_size += 128;
+ }
+
+ /*
+ * Add on the size of any boot services-only variables
+ */
+ active_size += boot_var_size;
+
+ active_available = storage_size - active_size;
+
+ if (!storage_size || size > remaining_size ||
+ (active_available - size) < (storage_size / 2))
return EFI_OUT_OF_RESOURCES;
return status;
--
1.8.1.2
On Mon, 2013-04-01 at 11:13 -0400, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.
Good thinking.
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
[...]
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata;
>
> unsigned long x86_efi_facility;
>
> +u64 efi_var_store_size;
> +u64 efi_var_remaining_size;
> +u64 efi_var_max_var_size;
[...]
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -2133,6 +2158,10 @@ efivars_init(void)
> ops.get_next_variable = efi.get_next_variable;
> ops.query_variable_info = efi.query_variable_info;
>
> +#ifdef CONFIG_X86
> + boot_used_size = efi_var_store_size - efi_var_remaining_size;
> +#endif
efivars can be built as a module, but these aren't exported.
Ben.
> error = register_efivars(&__efivars, &ops, efi_kobj);
> if (error)
> goto err_put;
--
Ben Hutchings
DNRC Motto: I can please only one person per day.
Today is not your day. Tomorrow isn't looking good either.
On Mon, 2013-04-01 at 11:14 -0400, Matthew Garrett wrote:
> EFI implementations distinguish between space that is actively used by a
> variable and space that merely hasn't been garbage collected yet. Space
> that hasn't yet been garbage collected isn't available for use and so isn't
> counted in the remaining_space field returned by QueryVariableInfo().
>
> Combined with commit 68d9298 this can cause problems. Some implementations
> don't garbage collect until the remaining space is smaller than the maximum
> variable size, and as a result check_var_size() will always fail once more
> than 50% of the variable store has been used even if most of that space is
> marked as available for garbage collection. The user is unable to create
> new variables, and deleting variables doesn't increase the remaining space.
>
> The problem that 68d9298 was attempting to avoid was one where certain
> platforms fail if the actively used space is greater than 50% of the
> available storage space. We should be able to calculate that by simply
> summing the size of each available variable and subtracting that from
> the total storage space. With luck this will fix the problem described in
> https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
> damage to occur to the machines 68d9298 was attempting to fix.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 60e7d8f..a6d8a58 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!storage_size || size > remaining_size || size > max_size ||
> - (remaining_size - size) < (storage_size / 2))
> + list_for_each_entry(entry, &efivars->list, list) {
> + var = &entry->var;
> + status = get_var_data_locked(efivars, var);
> +
> + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> + continue;
> +
> + active_size += var->DataSize;
> + active_size += utf16_strsize(var->VariableName, 1024);
> + /*
> + * There's some additional metadata associated with each
> + * variable. Intel's reference implementation is 60 bytes -
> + * bump that to 128 to account for vendor additions and
> + * potential alignment constraints
> + */
> + active_size += 128;
> + }
> +
> + /*
> + * Add on the size of any boot services-only variables
> + */
> + active_size += boot_var_size;
> +
> + active_available = storage_size - active_size;
> +
> + if (!storage_size || size > remaining_size ||
> + (active_available - size) < (storage_size / 2))
> return EFI_OUT_OF_RESOURCES;
Both substractions here could quite possibly underflow due to
over-estimation of active_size.
We can rearrange the condition to be:
active_size + size > storage_size / 2
and delete the active_available variable. But I think metadata for the
new variable should be accounted as well:
active_size + size + 128 > storage_size / 2
Since we'll now be using that magic number twice, it needs a name.
Ben.
> return status;
--
Ben Hutchings
DNRC Motto: I can please only one person per day.
Today is not your day. Tomorrow isn't looking good either.
On 01/04/13 16:13, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++
> arch/x86/include/asm/efi.h | 10 ++++++++
> arch/x86/include/uapi/asm/bootparam.h | 1 +
> arch/x86/platform/efi/efi.c | 21 ++++++++++++++++
> drivers/firmware/efivars.c | 29 +++++++++++++++++++++
> 5 files changed, 108 insertions(+)
We're fixing a regression in efivars.c, but only for those users that
boot via the EFI boot stub? That seems likely to upset some people.
Introducing new features via the EFI boot stub is fine, and working
around firmware bugs so that we can use some feature is also cool, but
we can't start fixing regressions from other subsystems in the EFI boot
stub.
--
Matt Fleming, Intel Open Source Technology Center
On 01/04/13 16:14, Matthew Garrett wrote:
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!storage_size || size > remaining_size || size > max_size ||
> - (remaining_size - size) < (storage_size / 2))
> + list_for_each_entry(entry, &efivars->list, list) {
> + var = &entry->var;
> + status = get_var_data_locked(efivars, var);
> +
> + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> + continue;
> +
> + active_size += var->DataSize;
> + active_size += utf16_strsize(var->VariableName, 1024);
> + /*
> + * There's some additional metadata associated with each
> + * variable. Intel's reference implementation is 60 bytes -
> + * bump that to 128 to account for vendor additions and
> + * potential alignment constraints
> + */
> + active_size += 128;
> + }
This is the kind of thing I was referring to when I said,
Hmm... I'm not convinced this will provide a long-term solution. Is there
anything that mandates that the size of all variables has to correlate
sensibly with the results from QueryVariableInfo()? Even if there is in
theory, I doubt it'll be true everywhere in practice, and trying to
workaround implementation bugs in our workarounds for other bugs is the
path to madness.
We can't continue this approach where we attempt to guess how the firmware
implements variable storage, because as we've seen, there are various
schemes out there.
This looks like something that will differ between implementations, and the
fact that it's appearing in our code is a sure sign that this isn't the way to
go.
--
Matt Fleming, Intel Open Source Technology Center
On Wed, 2013-04-03 at 14:09 +0100, Matt Fleming wrote:
> We're fixing a regression in efivars.c, but only for those users that
> boot via the EFI boot stub? That seems likely to upset some people.
Not really - it just makes the estimates more accurate. Applying (2)
without (1) should still fix the functional regression.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:
> This looks like something that will differ between implementations, and the
> fact that it's appearing in our code is a sure sign that this isn't the way to
> go.
Our choices right now are:
1) Break machines that don't garbage collect on every reboot
2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
3) Maintain a whitelist or blacklist that will inevitably be inadequate,
either breaking otherwise working machines or risking bricking of broken
ones
4) Attempt to implement something that'll work in all cases
Dealing with firmware is hard. This fixes (1) without leaving us with
(2), which seems like a net win.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 03/04/13 14:48, Matthew Garrett wrote:
> On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:
>
>> This looks like something that will differ between implementations, and the
>> fact that it's appearing in our code is a sure sign that this isn't the way to
>> go.
>
> Our choices right now are:
>
> 1) Break machines that don't garbage collect on every reboot
> 2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
> 3) Maintain a whitelist or blacklist that will inevitably be inadequate,
> either breaking otherwise working machines or risking bricking of broken
> ones
> 4) Attempt to implement something that'll work in all cases
The solution you're proposing has the same downsides as 3) - we risk
having to tweak things either way. The difference is that in the case of
3) the tweaking is adding entries to the whitelist, whereas tweaking
your solution has more chance of introducing further unwanted
regressions because you'd be tweaking an algorithm, an algorithm that
relies on the internal implementation of the variable storage code.
> Dealing with firmware is hard. This fixes (1) without leaving us with
> (2), which seems like a net win.
I'm not convinced that implementing 3) would inevitably lead to 2),
provided that we apply a bit of common sense when adding entries. I'm
not advocating some kind of flag day where we add umpteen machines to
the whitelist.
For reference, I just pushed two patches to the 'whitelist' branch at,
git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
which should hopefully illustrate the kind of thing that I'm talking about.
--
Matt Fleming, Intel Open Source Technology Center
On Wed, 2013-04-03 at 18:12 +0100, Matt Fleming wrote:
> The solution you're proposing has the same downsides as 3) - we risk
> having to tweak things either way. The difference is that in the case of
> 3) the tweaking is adding entries to the whitelist, whereas tweaking
> your solution has more chance of introducing further unwanted
> regressions because you'd be tweaking an algorithm, an algorithm that
> relies on the internal implementation of the variable storage code.
We *risk* having to tweak things, and we fail on the side of safety.
> > Dealing with firmware is hard. This fixes (1) without leaving us with
> > (2), which seems like a net win.
>
> I'm not convinced that implementing 3) would inevitably lead to 2),
> provided that we apply a bit of common sense when adding entries. I'm
> not advocating some kind of flag day where we add umpteen machines to
> the whitelist.
>
> For reference, I just pushed two patches to the 'whitelist' branch at,
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
>
> which should hopefully illustrate the kind of thing that I'm talking about.
I don't think that works. People are complaining that we broke some
Thinkpads as well, but we also have reports that Thinkpads can be
bricked if we use too much space.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?