LinuxLists.cc - [PATCH] Modify UEFI anti-bricking code

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

After quick testing it looks like this fixes the boot problem.
Boots with grub2 (EFI stubs), grub (no EFI stubs) and elilo.

Thanks!

On Sat, Jun 01, 2013 at 04:06:20PM -0400, Matthew Garrett wrote:
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
>
> Reverting these gets us back to the situation where we'd refuse to create
> variables on some systems because they classify deleted variables as "used"
> until the firmware triggers a garbage collection run, which they won't do
> until they reach a lower threshold. This results in it being impossible to
> install a bootloader, which is unhelpful.
>
> Feedback from Samsung indicates that the firmware doesn't need more than
> 5KB of storage space for its own purposes, so that seems like a reasonable
> threshold. However, there's still no guarantee that a platform will attempt
> garbage collection merely because it drops below this threshold. It seems
> that this is often only triggered if an attempt to write generates a
> genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
> create a variable larger than the remaining space. This should fail, but if
> it somehow succeeds we can then immediately delete it.
>
> I've tested this on the UEFI machines I have available, but I don't have
> a Samsung and so can't verify that it avoids the bricking problem.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 47 ----------
> arch/x86/include/asm/efi.h | 7 --
> arch/x86/include/uapi/asm/bootparam.h | 1 -
> arch/x86/platform/efi/efi.c | 169 +++++++++-------------------------
> 4 files changed, 45 insertions(+), 179 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 35ee62f..c205035 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -251,51 +251,6 @@ 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->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
> - 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((void *)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;
> @@ -1202,8 +1157,6 @@ 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 2fb5d58..60c89f3 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -102,13 +102,6 @@ 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 0874424..c15ddaf 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -6,7 +6,6 @@
> #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 82089d8..63e167a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -42,7 +42,6 @@
> #include <linux/io.h>
> #include <linux/reboot.h>
> #include <linux/bcd.h>
> -#include <linux/ucs2_string.h>
>
> #include <asm/setup.h>
> #include <asm/efi.h>
> @@ -54,13 +53,6 @@
>
> #define EFI_DEBUG 1
>
> -/*
> - * There's some additional metadata associated with each
> - * variable. Intel's reference implementation is 60 bytes - bump that
> - * to account for potential alignment constraints
> - */
> -#define VAR_METADATA_SIZE 64
> -
> struct efi __read_mostly efi = {
> .mps = EFI_INVALID_TABLE_ADDR,
> .acpi = EFI_INVALID_TABLE_ADDR,
> @@ -79,13 +71,6 @@ struct efi_memory_map memmap;
> static struct efi efi_phys __initdata;
> static efi_system_table_t efi_systab __initdata;
>
> -static u64 efi_var_store_size;
> -static u64 efi_var_remaining_size;
> -static u64 efi_var_max_var_size;
> -static u64 boot_used_size;
> -static u64 boot_var_size;
> -static u64 active_size;
> -
> unsigned long x86_efi_facility;
>
> /*
> @@ -188,53 +173,8 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
> efi_char16_t *name,
> efi_guid_t *vendor)
> {
> - efi_status_t status;
> - static bool finished = false;
> - static u64 var_size;
> -
> - status = efi_call_virt3(get_next_variable,
> - name_size, name, vendor);
> -
> - if (status == EFI_NOT_FOUND) {
> - finished = true;
> - if (var_size < boot_used_size) {
> - boot_var_size = boot_used_size - var_size;
> - active_size += boot_var_size;
> - } else {
> - printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n");
> - }
> - }
> -
> - if (boot_used_size && !finished) {
> - unsigned long size = 0;
> - u32 attr;
> - efi_status_t s;
> - void *tmp;
> -
> - s = virt_efi_get_variable(name, vendor, &attr, &size, NULL);
> -
> - if (s != EFI_BUFFER_TOO_SMALL || !size)
> - return status;
> -
> - tmp = kmalloc(size, GFP_ATOMIC);
> -
> - if (!tmp)
> - return status;
> -
> - s = virt_efi_get_variable(name, vendor, &attr, &size, tmp);
> -
> - if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) {
> - var_size += size;
> - var_size += ucs2_strsize(name, 1024);
> - active_size += size;
> - active_size += VAR_METADATA_SIZE;
> - active_size += ucs2_strsize(name, 1024);
> - }
> -
> - kfree(tmp);
> - }
> -
> - return status;
> + return efi_call_virt3(get_next_variable,
> + name_size, name, vendor);
> }
>
> static efi_status_t virt_efi_set_variable(efi_char16_t *name,
> @@ -243,34 +183,9 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
> unsigned long data_size,
> void *data)
> {
> - efi_status_t status;
> - u32 orig_attr = 0;
> - unsigned long orig_size = 0;
> -
> - status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size,
> - NULL);
> -
> - if (status != EFI_BUFFER_TOO_SMALL)
> - orig_size = 0;
> -
> - status = efi_call_virt5(set_variable,
> - name, vendor, attr,
> - data_size, data);
> -
> - if (status == EFI_SUCCESS) {
> - if (orig_size) {
> - active_size -= orig_size;
> - active_size -= ucs2_strsize(name, 1024);
> - active_size -= VAR_METADATA_SIZE;
> - }
> - if (data_size) {
> - active_size += data_size;
> - active_size += ucs2_strsize(name, 1024);
> - active_size += VAR_METADATA_SIZE;
> - }
> - }
> -
> - return status;
> + return efi_call_virt5(set_variable,
> + name, vendor, attr,
> + data_size, data);
> }
>
> static efi_status_t virt_efi_query_variable_info(u32 attr,
> @@ -786,9 +701,6 @@ 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 ||
> @@ -806,22 +718,6 @@ 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));
> - }
> -
> - boot_used_size = efi_var_store_size - efi_var_remaining_size;
> -
> set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
>
> /*
> @@ -1141,28 +1037,53 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!max_size && remaining_size > size)
> - printk_once(KERN_ERR FW_BUG "Broken EFI implementation"
> - " is returning MaxVariableSize=0\n");
> /*
> * Some firmware implementations refuse to boot if there's insufficient
> * space in the variable store. We account for that by refusing the
> * write if permitting it would reduce the available space to under
> - * 50%. However, some firmware won't reclaim variable space until
> - * after the used (not merely the actively used) space drops below
> - * a threshold. We can approximate that case with the value calculated
> - * above. If both the firmware and our calculations indicate that the
> - * available space would drop below 50%, refuse the write.
> + * 5KB. This figure was provided by Samsung, so should be safe.
> */
> + if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
> + /*
> + * Triggering garbage collection may require that the firmware
> + * generate a real EFI_OUT_OF_RESOURCES error. We can force
> + * that by attempting to use more space than is available.
> + */
> + unsigned long dummy_size = remaining_size + 1024;
> + void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
> + efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> + efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
> + 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
> + 0xa9);
> +
> + status = efi.set_variable(efi_name, &guid, attributes,
> + dummy_size, dummy);
> +
> + if (status == EFI_SUCCESS) {
> + /*
> + * This should have failed, so if it didn't make sure
> + * that we delete it...
> + */
> + efi.set_variable(efi_name, &guid, attributes, 0,
> + dummy);
> + }
>
> - if (!storage_size || size > remaining_size ||
> - (max_size && size > max_size))
> - return EFI_OUT_OF_RESOURCES;
> + /*
> + * The runtime code may now have triggered a garbage collection
> + * run, so check the variable info again
> + */
> + status = efi.query_variable_info(attributes, &storage_size,
> + &remaining_size, &max_size);
>
> - if (!efi_no_storage_paranoia &&
> - ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) &&
> - (remaining_size - size < storage_size / 2)))
> - return EFI_OUT_OF_RESOURCES;
> + if (status != EFI_SUCCESS)
> + return status;
> +
> + /*
> + * There still isn't enough room, so return an error
> + */
> + if (remaining_size - size < 5120)
> + return EFI_OUT_OF_RESOURCES;
> + }
>
> return EFI_SUCCESS;
> }
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc [email protected]

2013-06-03 12:18:03

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On 01/06/13 21:06, Matthew Garrett wrote:
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
>
> Reverting these gets us back to the situation where we'd refuse to create
> variables on some systems because they classify deleted variables as "used"
> until the firmware triggers a garbage collection run, which they won't do
> until they reach a lower threshold. This results in it being impossible to
> install a bootloader, which is unhelpful.
>
> Feedback from Samsung indicates that the firmware doesn't need more than
> 5KB of storage space for its own purposes, so that seems like a reasonable
> threshold. However, there's still no guarantee that a platform will attempt
> garbage collection merely because it drops below this threshold. It seems
> that this is often only triggered if an attempt to write generates a
> genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
> create a variable larger than the remaining space. This should fail, but if
> it somehow succeeds we can then immediately delete it.
>
> I've tested this on the UEFI machines I have available, but I don't have
> a Samsung and so can't verify that it avoids the bricking problem.

This patch works on my ASUS machine, which suffers from the "you need to
write more than 50% to initiate garbage collection" issue. So that's
good news. I'm trying to find someone with one of the vulnerable Samsung
laptops, but this patch does look like a step in the right direction,
seeing as it solves Russ' boot issue.

[...]

> @@ -1141,28 +1037,53 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!max_size && remaining_size > size)
> - printk_once(KERN_ERR FW_BUG "Broken EFI implementation"
> - " is returning MaxVariableSize=0\n");

Do we really want to drop this hunk? The point of this code was to
inform firmware vendors that their implementation is returning funky
results, and that they should look into why it's doing that.

> /*
> * Some firmware implementations refuse to boot if there's insufficient
> * space in the variable store. We account for that by refusing the
> * write if permitting it would reduce the available space to under
> - * 50%. However, some firmware won't reclaim variable space until
> - * after the used (not merely the actively used) space drops below
> - * a threshold. We can approximate that case with the value calculated
> - * above. If both the firmware and our calculations indicate that the
> - * available space would drop below 50%, refuse the write.
> + * 5KB. This figure was provided by Samsung, so should be safe.
> */
> + if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
> + /*
> + * Triggering garbage collection may require that the firmware
> + * generate a real EFI_OUT_OF_RESOURCES error. We can force
> + * that by attempting to use more space than is available.
> + */
> + unsigned long dummy_size = remaining_size + 1024;
> + void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
> + efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> + efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
> + 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
> + 0xa9);

What's the origin of this guid? And should we be adding it to
include/linux/efi.h?

2013-06-03 14:59:09

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Mon, 2013-06-03 at 13:17 +0100, Matt Fleming wrote:

> Do we really want to drop this hunk? The point of this code was to
> inform firmware vendors that their implementation is returning funky
> results, and that they should look into why it's doing that.

We're not doing anything with that information now, and I don't think we
can do anything meaningful with it - some implementations report the
maximum size as the maximum supported by the implementation, and some
report it as the maximum that can be written given the amount of space
currently available. So yeah, it's against the spec to report 0 here,
but any other value is equally useless to us, so it seems like something
we'd be complaining about for no reason.

> What's the origin of this guid? And should we be adding it to
> include/linux/efi.h?

It's a scratch guid that's randomly generated to avoid colliding with
any other guid - we want to avoid this ever being a real variable.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-03 16:15:16

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於六，2013-06-01 於 16:06 -0400，Matthew Garrett 提到：
> + unsigned long dummy_size = remaining_size + 1024;
> + void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
> + efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> + efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
> + 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
> + 0xa9);
> +
> + status = efi.set_variable(efi_name, &guid, attributes,
> + dummy_size, dummy);
> +
> + if (status == EFI_SUCCESS) {
> + /*
> + * This should have failed, so if it didn't make sure
> + * that we delete it...
> + */
> + efi.set_variable(efi_name, &guid, attributes, 0,
> + dummy);
> + }

Oliver raised a question for if power fails between that succesful
attempt and the deletion?

Due to the create/delete operating are not atomic, I think that will be
better we try to delete DUMMY object in efi_enter_virtual_mode() or when
efivars initial.

The following diff tested on OVMF for delete DUMMY object when system
boot, for reference.

Thanks a lot!
Joey Lee

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 63e167a..72770b0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -861,6 +861,10 @@ void __init efi_enter_virtual_mode(void)
u64 end, systab, start_pfn, end_pfn;
void *p, *va, *new_memmap = NULL;
int count = 0;
+ efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
+ efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
+ 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
+ 0xa9);

efi.systab = NULL;

@@ -981,6 +985,9 @@ void __init efi_enter_virtual_mode(void)
runtime_code_page_mkexec();

kfree(new_memmap);
+
+ /* clean DUMMY object */
+ efi.set_variable(efi_name, &guid, 0, 0, NULL);
}

/*

2013-06-03 16:31:50

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Tue, 2013-06-04 at 00:13 +0800, joeyli wrote:

> Oliver raised a question for if power fails between that succesful
> attempt and the deletion?

It's a pretty tiny window, but sure. Making sure we delete it seems
sensible. In that case we should make the GUID a #define rather than
write it out twice.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-04 03:36:26

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於一，2013-06-03 於 16:31 +0000，Matthew Garrett 提到：
> On Tue, 2013-06-04 at 00:13 +0800, joeyli wrote:
>
> > Oliver raised a question for if power fails between that succesful
> > attempt and the deletion?
>
> It's a pretty tiny window, but sure. Making sure we delete it seems
> sensible. In that case we should make the GUID a #define rather than
> write it out twice.
>

Base on your patch, the following diff moved DUMMY GUID to #define, and
add a static efi name string:

Thanks a lot!
Joey Lee

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 63e167a..cc3cfe8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,11 @@

#define EFI_DEBUG 1

+#define EFI_DUMMY_GUID \
+ EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
+
+static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
+
struct efi __read_mostly efi = {
.mps = EFI_INVALID_TABLE_ADDR,
.acpi = EFI_INVALID_TABLE_ADDR,
@@ -981,6 +986,9 @@ void __init efi_enter_virtual_mode(void)
runtime_code_page_mkexec();

kfree(new_memmap);
+
+ /* clean DUMMY object */
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
}

/*
@@ -1051,21 +1059,17 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
*/
unsigned long dummy_size = remaining_size + 1024;
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
- efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
- efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
- 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
- 0xa9);

- status = efi.set_variable(efi_name, &guid, attributes,
- dummy_size, dummy);
+ status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ attributes, dummy_size, dummy);

if (status == EFI_SUCCESS) {
/*
* This should have failed, so if it didn't make sure
* that we delete it...
*/
- efi.set_variable(efi_name, &guid, attributes, 0,
- dummy);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ attributes, 0, dummy);
}

/*

2013-06-04 05:30:35

by Lingzhu Xiang

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On 06/02/2013 04:06 AM, Matthew Garrett wrote:
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
>
> Reverting these gets us back to the situation where we'd refuse to create
> variables on some systems because they classify deleted variables as "used"
> until the firmware triggers a garbage collection run, which they won't do
> until they reach a lower threshold. This results in it being impossible to
> install a bootloader, which is unhelpful.
>
> Feedback from Samsung indicates that the firmware doesn't need more than
> 5KB of storage space for its own purposes, so that seems like a reasonable
> threshold. However, there's still no guarantee that a platform will attempt
> garbage collection merely because it drops below this threshold. It seems
> that this is often only triggered if an attempt to write generates a
> genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
> create a variable larger than the remaining space. This should fail, but if
> it somehow succeeds we can then immediately delete it.
>
> I've tested this on the UEFI machines I have available, but I don't have
> a Samsung and so can't verify that it avoids the bricking problem.

This patch works well on my Dell Windows 8 logo machine.

This Dell machine will use up ~2K nvram space on each reboot and do
garbage collection at boot time when full. With the new 5K threshold, it
is only a few reboots away from boot time garbage collection anyway.

Run time garbage collection also works. But the write takes 5 seconds.

Lingzhu Xiang

2013-06-05 14:49:50

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On 4 June 2013 04:35, joeyli <[email protected]> wrote:
> ?? ?@?A2013-06-03 ?? 16:31 +0000?AMatthew Garrett ?????G
>> On Tue, 2013-06-04 at 00:13 +0800, joeyli wrote:
>>
>> > Oliver raised a question for if power fails between that succesful
>> > attempt and the deletion?
>>
>> It's a pretty tiny window, but sure. Making sure we delete it seems
>> sensible. In that case we should make the GUID a #define rather than
>> write it out twice.
>>
>
> Base on your patch, the following diff moved DUMMY GUID to #define, and
> add a static efi name string:

Folks, what do you want me to do with this? Merge it with Matthew's patch?

The feedback from the original patch has been pretty good so I'm happy
to pick it up and get it sent to Linus.

2013-06-05 14:53:52

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:

> Folks, what do you want me to do with this? Merge it with Matthew's patch?

Do that and add Joey's signed-off-by?

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-05 15:59:19

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Wed, 05 Jun, at 02:53:27PM, Matthew Garrett wrote:
> On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:
>
> > Folks, what do you want me to do with this? Merge it with Matthew's patch?
>
> Do that and add Joey's signed-off-by?

Right, this is what I've got queued up.

---

>From 380dcc12ba82f4e10feb6a72207b2e4771d16d8d Mon Sep 17 00:00:00 2001
From: Matthew Garrett <[email protected]>
Date: Sat, 1 Jun 2013 16:06:20 -0400
Subject: [PATCH] Modify UEFI anti-bricking code

This patch reworks the UEFI anti-bricking code, including an effective
reversion of cc5a080c and 31ff2f20. It turns out that calling
QueryVariableInfo() from boot services results in some firmware
implementations jumping to physical addresses even after entering virtual
mode, so until we have 1:1 mappings for UEFI runtime space this isn't
going to work so well.

Reverting these gets us back to the situation where we'd refuse to create
variables on some systems because they classify deleted variables as "used"
until the firmware triggers a garbage collection run, which they won't do
until they reach a lower threshold. This results in it being impossible to
install a bootloader, which is unhelpful.

Feedback from Samsung indicates that the firmware doesn't need more than
5KB of storage space for its own purposes, so that seems like a reasonable
threshold. However, there's still no guarantee that a platform will attempt
garbage collection merely because it drops below this threshold. It seems
that this is often only triggered if an attempt to write generates a
genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
create a variable larger than the remaining space. This should fail, but if
it somehow succeeds we can then immediately delete it.

I've tested this on the UEFI machines I have available, but I don't have
a Samsung and so can't verify that it avoids the bricking problem.

Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Lee, Chun-Y <[email protected]> [ dummy variable cleanup ]
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 47 ----------
arch/x86/include/asm/efi.h | 7 --
arch/x86/include/uapi/asm/bootparam.h | 1 -
arch/x86/platform/efi/efi.c | 171 ++++++++++------------------------
4 files changed, 48 insertions(+), 178 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 35ee62f..c205035 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,51 +251,6 @@ 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->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
- 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((void *)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;
@@ -1202,8 +1157,6 @@ 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 2fb5d58..60c89f3 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -102,13 +102,6 @@ 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 0874424..c15ddaf 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,7 +6,6 @@
#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 82089d8..cc3cfe8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -42,7 +42,6 @@
#include <linux/io.h>
#include <linux/reboot.h>
#include <linux/bcd.h>
-#include <linux/ucs2_string.h>

#include <asm/setup.h>
#include <asm/efi.h>
@@ -54,12 +53,10 @@

#define EFI_DEBUG 1

-/*
- * There's some additional metadata associated with each
- * variable. Intel's reference implementation is 60 bytes - bump that
- * to account for potential alignment constraints
- */
-#define VAR_METADATA_SIZE 64
+#define EFI_DUMMY_GUID \
+ EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
+
+static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };

struct efi __read_mostly efi = {
.mps = EFI_INVALID_TABLE_ADDR,
@@ -79,13 +76,6 @@ struct efi_memory_map memmap;
static struct efi efi_phys __initdata;
static efi_system_table_t efi_systab __initdata;

-static u64 efi_var_store_size;
-static u64 efi_var_remaining_size;
-static u64 efi_var_max_var_size;
-static u64 boot_used_size;
-static u64 boot_var_size;
-static u64 active_size;
-
unsigned long x86_efi_facility;

/*
@@ -188,53 +178,8 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
efi_char16_t *name,
efi_guid_t *vendor)
{
- efi_status_t status;
- static bool finished = false;
- static u64 var_size;
-
- status = efi_call_virt3(get_next_variable,
- name_size, name, vendor);
-
- if (status == EFI_NOT_FOUND) {
- finished = true;
- if (var_size < boot_used_size) {
- boot_var_size = boot_used_size - var_size;
- active_size += boot_var_size;
- } else {
- printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n");
- }
- }
-
- if (boot_used_size && !finished) {
- unsigned long size = 0;
- u32 attr;
- efi_status_t s;
- void *tmp;
-
- s = virt_efi_get_variable(name, vendor, &attr, &size, NULL);
-
- if (s != EFI_BUFFER_TOO_SMALL || !size)
- return status;
-
- tmp = kmalloc(size, GFP_ATOMIC);
-
- if (!tmp)
- return status;
-
- s = virt_efi_get_variable(name, vendor, &attr, &size, tmp);
-
- if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) {
- var_size += size;
- var_size += ucs2_strsize(name, 1024);
- active_size += size;
- active_size += VAR_METADATA_SIZE;
- active_size += ucs2_strsize(name, 1024);
- }
-
- kfree(tmp);
- }
-
- return status;
+ return efi_call_virt3(get_next_variable,
+ name_size, name, vendor);
}

static efi_status_t virt_efi_set_variable(efi_char16_t *name,
@@ -243,34 +188,9 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
unsigned long data_size,
void *data)
{
- efi_status_t status;
- u32 orig_attr = 0;
- unsigned long orig_size = 0;
-
- status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size,
- NULL);
-
- if (status != EFI_BUFFER_TOO_SMALL)
- orig_size = 0;
-
- status = efi_call_virt5(set_variable,
- name, vendor, attr,
- data_size, data);
-
- if (status == EFI_SUCCESS) {
- if (orig_size) {
- active_size -= orig_size;
- active_size -= ucs2_strsize(name, 1024);
- active_size -= VAR_METADATA_SIZE;
- }
- if (data_size) {
- active_size += data_size;
- active_size += ucs2_strsize(name, 1024);
- active_size += VAR_METADATA_SIZE;
- }
- }
-
- return status;
+ return efi_call_virt5(set_variable,
+ name, vendor, attr,
+ data_size, data);
}

static efi_status_t virt_efi_query_variable_info(u32 attr,
@@ -786,9 +706,6 @@ 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 ||
@@ -806,22 +723,6 @@ 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));
- }
-
- boot_used_size = efi_var_store_size - efi_var_remaining_size;
-
set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);

/*
@@ -1085,6 +986,9 @@ void __init efi_enter_virtual_mode(void)
runtime_code_page_mkexec();

kfree(new_memmap);
+
+ /* clean DUMMY object */
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
}

/*
@@ -1141,28 +1045,49 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
if (status != EFI_SUCCESS)
return status;

- if (!max_size && remaining_size > size)
- printk_once(KERN_ERR FW_BUG "Broken EFI implementation"
- " is returning MaxVariableSize=0\n");
/*
* Some firmware implementations refuse to boot if there's insufficient
* space in the variable store. We account for that by refusing the
* write if permitting it would reduce the available space to under
- * 50%. However, some firmware won't reclaim variable space until
- * after the used (not merely the actively used) space drops below
- * a threshold. We can approximate that case with the value calculated
- * above. If both the firmware and our calculations indicate that the
- * available space would drop below 50%, refuse the write.
+ * 5KB. This figure was provided by Samsung, so should be safe.
*/
+ if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ /*
+ * Triggering garbage collection may require that the firmware
+ * generate a real EFI_OUT_OF_RESOURCES error. We can force
+ * that by attempting to use more space than is available.
+ */
+ unsigned long dummy_size = remaining_size + 1024;
+ void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
+
+ status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ attributes, dummy_size, dummy);
+
+ if (status == EFI_SUCCESS) {
+ /*
+ * This should have failed, so if it didn't make sure
+ * that we delete it...
+ */
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ attributes, 0, dummy);
+ }

- if (!storage_size || size > remaining_size ||
- (max_size && size > max_size))
- return EFI_OUT_OF_RESOURCES;
+ /*
+ * The runtime code may now have triggered a garbage collection
+ * run, so check the variable info again
+ */
+ status = efi.query_variable_info(attributes, &storage_size,
+ &remaining_size, &max_size);

- if (!efi_no_storage_paranoia &&
- ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) &&
- (remaining_size - size < storage_size / 2)))
- return EFI_OUT_OF_RESOURCES;
+ if (status != EFI_SUCCESS)
+ return status;
+
+ /*
+ * There still isn't enough room, so return an error
+ */
+ if (remaining_size - size < 5120)
+ return EFI_OUT_OF_RESOURCES;
+ }

return EFI_SUCCESS;
}
--
1.8.1.4

--
Matt Fleming, Intel Open Source Technology Center

2013-06-05 16:08:57

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Wed, 2013-06-05 at 16:59 +0100, Matt Fleming wrote:

> + /* clean DUMMY object */
> + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);

Hm. Actually, is that going to work? From the spec:

If a preexisting variable is rewritten with different attributes,
SetVariable()shall not modify the variable and shall return
EFI_INVALID_PARAMETER.

So I think we probably need to fix the attributes to NV|RT|BS for both
this call and the one in query_variable_store. We should probably also
only do the workaround if the NV bit is set in the original query.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-05 20:00:00

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Wed, 05 Jun, at 04:08:39PM, Matthew Garrett wrote:
> On Wed, 2013-06-05 at 16:59 +0100, Matt Fleming wrote:
>
> > + /* clean DUMMY object */
> > + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
>
> Hm. Actually, is that going to work? From the spec:
>
> If a preexisting variable is rewritten with different attributes,
> SetVariable()shall not modify the variable and shall return
> EFI_INVALID_PARAMETER.
>
> So I think we probably need to fix the attributes to NV|RT|BS for both
> this call and the one in query_variable_store. We should probably also
> only do the workaround if the NV bit is set in the original query.

Yeah, that looks wrong.

Joey, care to resend an updated patch?

--
Matt Fleming, Intel Open Source Technology Center

2013-06-05 20:19:22

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On 06/05/2013 08:59 AM, Matt Fleming wrote:
> + * There still isn't enough room, so return an error
> + */
> + if (remaining_size - size < 5120)
> + return EFI_OUT_OF_RESOURCES;
> + }

Please don't open-code the constant 5120 in this case. We may easily
run into a system which needs, say, 7K, and it is error-prone to just
replace numbers.

-hpa

2013-06-06 02:25:46

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於三，2013-06-05 於 16:08 +0000，Matthew Garrett 提到：
> On Wed, 2013-06-05 at 16:59 +0100, Matt Fleming wrote:
>
> > + /* clean DUMMY object */
> > + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
>
> Hm. Actually, is that going to work? From the spec:
>

The patch I tested on OVMF, it can delete DUMMY object when system boot.

> If a preexisting variable is rewritten with different attributes,
> SetVariable()shall not modify the variable and shall return
> EFI_INVALID_PARAMETER.
>
> So I think we probably need to fix the attributes to NV|RT|BS for both
> this call and the one in query_variable_store. We should probably also
> only do the workaround if the NV bit is set in the original query.
>
> --
> Matthew Garrett | [email protected]

Yes, I think that more safe for fix the attributes.

Thanks a lot!
Joey Lee

2013-06-06 05:06:36

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於三，2013-06-05 於 16:59 +0100，Matt Fleming 提到：
> On Wed, 05 Jun, at 02:53:27PM, Matthew Garrett wrote:
> > On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:
> >
> > > Folks, what do you want me to do with this? Merge it with Matthew's patch?
> >
> > Do that and add Joey's signed-off-by?
>
> Right, this is what I've got queued up.
>
> ---
>
> >From 380dcc12ba82f4e10feb6a72207b2e4771d16d8d Mon Sep 17 00:00:00 2001
> From: Matthew Garrett <[email protected]>
> Date: Sat, 1 Jun 2013 16:06:20 -0400
> Subject: [PATCH] Modify UEFI anti-bricking code
>
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
[...]

The follow diff change is base on 380dcc12 patch queued in efi git tree,
it included Matthew and hpa's suggestions. I fix the attributes of DUMMY
object to NV/BS/RT and introduced a #define of the minimum reserve flash
space.

This change works to me on OVMF.

Thanks a lot!
Joey Lee

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..2617675 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@

#define EFI_DEBUG 1

+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)

@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);

/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
}

/*
@@ -1051,7 +1057,12 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_OUT_OF_RESOURCES;
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);

status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);

if (status == EFI_SUCCESS) {
/*
@@ -1085,7 +1099,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}

2013-06-06 05:42:31

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於四，2013-06-06 於 13:05 +0800，joeyli 提到：
> 於三，2013-06-05 於 16:59 +0100，Matt Fleming 提到：
> > On Wed, 05 Jun, at 02:53:27PM, Matthew Garrett wrote:
> > > On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:
> > >
> > > > Folks, what do you want me to do with this? Merge it with Matthew's patch?
> > >
> > > Do that and add Joey's signed-off-by?
> >
> > Right, this is what I've got queued up.
> >
> > ---
> >
> > >From 380dcc12ba82f4e10feb6a72207b2e4771d16d8d Mon Sep 17 00:00:00 2001
> > From: Matthew Garrett <[email protected]>
> > Date: Sat, 1 Jun 2013 16:06:20 -0400
> > Subject: [PATCH] Modify UEFI anti-bricking code
> >
> > This patch reworks the UEFI anti-bricking code, including an effective
> > reversion of cc5a080c and 31ff2f20. It turns out that calling
> > QueryVariableInfo() from boot services results in some firmware
> > implementations jumping to physical addresses even after entering virtual
> > mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> > going to work so well.
> [...]
>
> The follow diff change is base on 380dcc12 patch queued in efi git tree,
> it included Matthew and hpa's suggestions. I fix the attributes of DUMMY
> object to NV/BS/RT and introduced a #define of the minimum reserve flash
> space.
>
> This change works to me on OVMF.
>
>
>
> Thanks a lot!
> Joey Lee
>

Sorry for attached a wrong diff result, it lost a NV/BS/RT attributes
changed in efi_query_variable_store(). The right diff change is
following.

Thanks a lot!
Joey Lee

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..ec8ac97 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@

#define EFI_DEBUG 1

+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)

@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);

/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
}

/*
@@ -1051,7 +1057,12 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_OUT_OF_RESOURCES;
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);

status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);

if (status == EFI_SUCCESS) {
/*
@@ -1069,7 +1083,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
* that we delete it...
*/
efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, 0, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, dummy);
}

/*
@@ -1085,7 +1102,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}

2013-06-06 05:42:28

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Thu, 2013-06-06 at 13:05 +0800, joeyli wrote:

> + if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
> + return EFI_OUT_OF_RESOURCES;

I'd move this up to the top of the function, and just return 0 - there's
no risk of the firmware causing problems if it's a volatile variable, so
we should probably just pass it down to the firmware and return an error
from there.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-06 07:41:52

by joeyli

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

於四，2013-06-06 於 05:42 +0000，Matthew Garrett 提到：
> On Thu, 2013-06-06 at 13:05 +0800, joeyli wrote:
>
> > + if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
> > + return EFI_OUT_OF_RESOURCES;
>
> I'd move this up to the top of the function, and just return 0 - there's
> no risk of the firmware causing problems if it's a volatile variable, so
> we should probably just pass it down to the firmware and return an error
> from there.
>

OK, I moved volatile checking to the top of the function.
New version, version 3 diff result like the following.

Thanks a lot for reviewing
Joey Lee

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..5ae2eb0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@

#define EFI_DEBUG 1

+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)

@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);

/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
}

/*
@@ -1040,6 +1046,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
efi_status_t status;
u64 storage_size, remaining_size, max_size;

+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return 0;
+
status = efi.query_variable_info(attributes, &storage_size,
&remaining_size, &max_size);
if (status != EFI_SUCCESS)
@@ -1051,7 +1060,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);

status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);

if (status == EFI_SUCCESS) {
/*
@@ -1069,7 +1083,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
* that we delete it...
*/
efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, 0, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, dummy);
}

/*
@@ -1085,7 +1102,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}

2013-06-06 09:25:59

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Thu, 06 Jun, at 03:40:26PM, joeyli wrote:
> OK, I moved volatile checking to the top of the function.
> New version, version 3 diff result like the following.

Thanks. This is what I've now got queued up.

---

>From 118428bf3b207d9b390a27f32dfef6dc2979078d Mon Sep 17 00:00:00 2001
From: Matthew Garrett <[email protected]>
Date: Sat, 1 Jun 2013 16:06:20 -0400
Subject: [PATCH] Modify UEFI anti-bricking code

This patch reworks the UEFI anti-bricking code, including an effective
reversion of cc5a080c and 31ff2f20. It turns out that calling
QueryVariableInfo() from boot services results in some firmware
implementations jumping to physical addresses even after entering virtual
mode, so until we have 1:1 mappings for UEFI runtime space this isn't
going to work so well.

Reverting these gets us back to the situation where we'd refuse to create
variables on some systems because they classify deleted variables as "used"
until the firmware triggers a garbage collection run, which they won't do
until they reach a lower threshold. This results in it being impossible to
install a bootloader, which is unhelpful.

Feedback from Samsung indicates that the firmware doesn't need more than
5KB of storage space for its own purposes, so that seems like a reasonable
threshold. However, there's still no guarantee that a platform will attempt
garbage collection merely because it drops below this threshold. It seems
that this is often only triggered if an attempt to write generates a
genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
create a variable larger than the remaining space. This should fail, but if
it somehow succeeds we can then immediately delete it.

I've tested this on the UEFI machines I have available, but I don't have
a Samsung and so can't verify that it avoids the bricking problem.

Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Lee, Chun-Y <[email protected]> [ dummy variable cleanup ]
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 47 ---------
arch/x86/include/asm/efi.h | 7 --
arch/x86/include/uapi/asm/bootparam.h | 1 -
arch/x86/platform/efi/efi.c | 188 ++++++++++++----------------------
4 files changed, 65 insertions(+), 178 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 35ee62f..c205035 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,51 +251,6 @@ 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->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
- 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((void *)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;
@@ -1202,8 +1157,6 @@ 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 2fb5d58..60c89f3 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -102,13 +102,6 @@ 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 0874424..c15ddaf 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,7 +6,6 @@
#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 82089d8..5ae2eb0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -42,7 +42,6 @@
#include <linux/io.h>
#include <linux/reboot.h>
#include <linux/bcd.h>
-#include <linux/ucs2_string.h>

#include <asm/setup.h>
#include <asm/efi.h>
@@ -54,12 +53,12 @@

#define EFI_DEBUG 1

-/*
- * There's some additional metadata associated with each
- * variable. Intel's reference implementation is 60 bytes - bump that
- * to account for potential alignment constraints
- */
-#define VAR_METADATA_SIZE 64
+#define EFI_MIN_RESERVE 5120
+
+#define EFI_DUMMY_GUID \
+ EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
+
+static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };

struct efi __read_mostly efi = {
.mps = EFI_INVALID_TABLE_ADDR,
@@ -79,13 +78,6 @@ struct efi_memory_map memmap;
static struct efi efi_phys __initdata;
static efi_system_table_t efi_systab __initdata;

-static u64 efi_var_store_size;
-static u64 efi_var_remaining_size;
-static u64 efi_var_max_var_size;
-static u64 boot_used_size;
-static u64 boot_var_size;
-static u64 active_size;
-
unsigned long x86_efi_facility;

/*
@@ -188,53 +180,8 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
efi_char16_t *name,
efi_guid_t *vendor)
{
- efi_status_t status;
- static bool finished = false;
- static u64 var_size;
-
- status = efi_call_virt3(get_next_variable,
- name_size, name, vendor);
-
- if (status == EFI_NOT_FOUND) {
- finished = true;
- if (var_size < boot_used_size) {
- boot_var_size = boot_used_size - var_size;
- active_size += boot_var_size;
- } else {
- printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n");
- }
- }
-
- if (boot_used_size && !finished) {
- unsigned long size = 0;
- u32 attr;
- efi_status_t s;
- void *tmp;
-
- s = virt_efi_get_variable(name, vendor, &attr, &size, NULL);
-
- if (s != EFI_BUFFER_TOO_SMALL || !size)
- return status;
-
- tmp = kmalloc(size, GFP_ATOMIC);
-
- if (!tmp)
- return status;
-
- s = virt_efi_get_variable(name, vendor, &attr, &size, tmp);
-
- if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) {
- var_size += size;
- var_size += ucs2_strsize(name, 1024);
- active_size += size;
- active_size += VAR_METADATA_SIZE;
- active_size += ucs2_strsize(name, 1024);
- }
-
- kfree(tmp);
- }
-
- return status;
+ return efi_call_virt3(get_next_variable,
+ name_size, name, vendor);
}

static efi_status_t virt_efi_set_variable(efi_char16_t *name,
@@ -243,34 +190,9 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
unsigned long data_size,
void *data)
{
- efi_status_t status;
- u32 orig_attr = 0;
- unsigned long orig_size = 0;
-
- status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size,
- NULL);
-
- if (status != EFI_BUFFER_TOO_SMALL)
- orig_size = 0;
-
- status = efi_call_virt5(set_variable,
- name, vendor, attr,
- data_size, data);
-
- if (status == EFI_SUCCESS) {
- if (orig_size) {
- active_size -= orig_size;
- active_size -= ucs2_strsize(name, 1024);
- active_size -= VAR_METADATA_SIZE;
- }
- if (data_size) {
- active_size += data_size;
- active_size += ucs2_strsize(name, 1024);
- active_size += VAR_METADATA_SIZE;
- }
- }
-
- return status;
+ return efi_call_virt5(set_variable,
+ name, vendor, attr,
+ data_size, data);
}

static efi_status_t virt_efi_query_variable_info(u32 attr,
@@ -786,9 +708,6 @@ 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 ||
@@ -806,22 +725,6 @@ 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));
- }
-
- boot_used_size = efi_var_store_size - efi_var_remaining_size;
-
set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);

/*
@@ -1085,6 +988,13 @@ void __init efi_enter_virtual_mode(void)
runtime_code_page_mkexec();

kfree(new_memmap);
+
+ /* clean DUMMY object */
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
}

/*
@@ -1136,33 +1046,65 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
efi_status_t status;
u64 storage_size, remaining_size, max_size;

+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return 0;
+
status = efi.query_variable_info(attributes, &storage_size,
&remaining_size, &max_size);
if (status != EFI_SUCCESS)
return status;

- if (!max_size && remaining_size > size)
- printk_once(KERN_ERR FW_BUG "Broken EFI implementation"
- " is returning MaxVariableSize=0\n");
/*
* Some firmware implementations refuse to boot if there's insufficient
* space in the variable store. We account for that by refusing the
* write if permitting it would reduce the available space to under
- * 50%. However, some firmware won't reclaim variable space until
- * after the used (not merely the actively used) space drops below
- * a threshold. We can approximate that case with the value calculated
- * above. If both the firmware and our calculations indicate that the
- * available space would drop below 50%, refuse the write.
+ * 5KB. This figure was provided by Samsung, so should be safe.
*/
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
+ /*
+ * Triggering garbage collection may require that the firmware
+ * generate a real EFI_OUT_OF_RESOURCES error. We can force
+ * that by attempting to use more space than is available.
+ */
+ unsigned long dummy_size = remaining_size + 1024;
+ void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
+
+ status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);
+
+ if (status == EFI_SUCCESS) {
+ /*
+ * This should have failed, so if it didn't make sure
+ * that we delete it...
+ */
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, dummy);
+ }

- if (!storage_size || size > remaining_size ||
- (max_size && size > max_size))
- return EFI_OUT_OF_RESOURCES;
+ /*
+ * The runtime code may now have triggered a garbage collection
+ * run, so check the variable info again
+ */
+ status = efi.query_variable_info(attributes, &storage_size,
+ &remaining_size, &max_size);

- if (!efi_no_storage_paranoia &&
- ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) &&
- (remaining_size - size < storage_size / 2)))
- return EFI_OUT_OF_RESOURCES;
+ if (status != EFI_SUCCESS)
+ return status;
+
+ /*
+ * There still isn't enough room, so return an error
+ */
+ if (remaining_size - size < EFI_MIN_RESERVE)
+ return EFI_OUT_OF_RESOURCES;
+ }

return EFI_SUCCESS;
}
--
1.8.1.4

--
Matt Fleming, Intel Open Source Technology Center

2013-06-06 14:48:51

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Thu, Jun 06, 2013 at 10:25:42AM +0100, Matt Fleming wrote:
> On Thu, 06 Jun, at 03:40:26PM, joeyli wrote:
> > OK, I moved volatile checking to the top of the function.
> > New version, version 3 diff result like the following.
>
> Thanks. This is what I've now got queued up.
>
> ---
>
> >From 118428bf3b207d9b390a27f32dfef6dc2979078d Mon Sep 17 00:00:00 2001
> From: Matthew Garrett <[email protected]>
> Date: Sat, 1 Jun 2013 16:06:20 -0400
> Subject: [PATCH] Modify UEFI anti-bricking code
>
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
>
> Reverting these gets us back to the situation where we'd refuse to create
> variables on some systems because they classify deleted variables as "used"
> until the firmware triggers a garbage collection run, which they won't do
> until they reach a lower threshold. This results in it being impossible to
> install a bootloader, which is unhelpful.
>
> Feedback from Samsung indicates that the firmware doesn't need more than
> 5KB of storage space for its own purposes, so that seems like a reasonable
> threshold. However, there's still no guarantee that a platform will attempt
> garbage collection merely because it drops below this threshold. It seems
> that this is often only triggered if an attempt to write generates a
> genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to
> create a variable larger than the remaining space. This should fail, but if
> it somehow succeeds we can then immediately delete it.
>
> I've tested this on the UEFI machines I have available, but I don't have
> a Samsung and so can't verify that it avoids the bricking problem.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> Signed-off-by: Lee, Chun-Y <[email protected]> [ dummy variable cleanup ]
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 47 ---------
> arch/x86/include/asm/efi.h | 7 --
> arch/x86/include/uapi/asm/bootparam.h | 1 -
> arch/x86/platform/efi/efi.c | 188 ++++++++++++----------------------
> 4 files changed, 65 insertions(+), 178 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 35ee62f..c205035 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -251,51 +251,6 @@ 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->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
> - 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((void *)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;
> @@ -1202,8 +1157,6 @@ 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 2fb5d58..60c89f3 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -102,13 +102,6 @@ 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 0874424..c15ddaf 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -6,7 +6,6 @@
> #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 82089d8..5ae2eb0 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -42,7 +42,6 @@
> #include <linux/io.h>
> #include <linux/reboot.h>
> #include <linux/bcd.h>
> -#include <linux/ucs2_string.h>
>
> #include <asm/setup.h>
> #include <asm/efi.h>
> @@ -54,12 +53,12 @@
>
> #define EFI_DEBUG 1
>
> -/*
> - * There's some additional metadata associated with each
> - * variable. Intel's reference implementation is 60 bytes - bump that
> - * to account for potential alignment constraints
> - */
> -#define VAR_METADATA_SIZE 64
> +#define EFI_MIN_RESERVE 5120
> +
> +#define EFI_DUMMY_GUID \
> + EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
> +
> +static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
>
> struct efi __read_mostly efi = {
> .mps = EFI_INVALID_TABLE_ADDR,
> @@ -79,13 +78,6 @@ struct efi_memory_map memmap;
> static struct efi efi_phys __initdata;
> static efi_system_table_t efi_systab __initdata;
>
> -static u64 efi_var_store_size;
> -static u64 efi_var_remaining_size;
> -static u64 efi_var_max_var_size;
> -static u64 boot_used_size;
> -static u64 boot_var_size;
> -static u64 active_size;
> -
> unsigned long x86_efi_facility;
>
> /*
> @@ -188,53 +180,8 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
> efi_char16_t *name,
> efi_guid_t *vendor)
> {
> - efi_status_t status;
> - static bool finished = false;
> - static u64 var_size;
> -
> - status = efi_call_virt3(get_next_variable,
> - name_size, name, vendor);
> -
> - if (status == EFI_NOT_FOUND) {
> - finished = true;
> - if (var_size < boot_used_size) {
> - boot_var_size = boot_used_size - var_size;
> - active_size += boot_var_size;
> - } else {
> - printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n");
> - }
> - }
> -
> - if (boot_used_size && !finished) {
> - unsigned long size = 0;
> - u32 attr;
> - efi_status_t s;
> - void *tmp;
> -
> - s = virt_efi_get_variable(name, vendor, &attr, &size, NULL);
> -
> - if (s != EFI_BUFFER_TOO_SMALL || !size)
> - return status;
> -
> - tmp = kmalloc(size, GFP_ATOMIC);
> -
> - if (!tmp)
> - return status;
> -
> - s = virt_efi_get_variable(name, vendor, &attr, &size, tmp);
> -
> - if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) {
> - var_size += size;
> - var_size += ucs2_strsize(name, 1024);
> - active_size += size;
> - active_size += VAR_METADATA_SIZE;
> - active_size += ucs2_strsize(name, 1024);
> - }
> -
> - kfree(tmp);
> - }
> -
> - return status;
> + return efi_call_virt3(get_next_variable,
> + name_size, name, vendor);
> }
>
> static efi_status_t virt_efi_set_variable(efi_char16_t *name,
> @@ -243,34 +190,9 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
> unsigned long data_size,
> void *data)
> {
> - efi_status_t status;
> - u32 orig_attr = 0;
> - unsigned long orig_size = 0;
> -
> - status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size,
> - NULL);
> -
> - if (status != EFI_BUFFER_TOO_SMALL)
> - orig_size = 0;
> -
> - status = efi_call_virt5(set_variable,
> - name, vendor, attr,
> - data_size, data);
> -
> - if (status == EFI_SUCCESS) {
> - if (orig_size) {
> - active_size -= orig_size;
> - active_size -= ucs2_strsize(name, 1024);
> - active_size -= VAR_METADATA_SIZE;
> - }
> - if (data_size) {
> - active_size += data_size;
> - active_size += ucs2_strsize(name, 1024);
> - active_size += VAR_METADATA_SIZE;
> - }
> - }
> -
> - return status;
> + return efi_call_virt5(set_variable,
> + name, vendor, attr,
> + data_size, data);
> }
>
> static efi_status_t virt_efi_query_variable_info(u32 attr,
> @@ -786,9 +708,6 @@ 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 ||
> @@ -806,22 +725,6 @@ 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));
> - }
> -
> - boot_used_size = efi_var_store_size - efi_var_remaining_size;
> -
> set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
>
> /*
> @@ -1085,6 +988,13 @@ void __init efi_enter_virtual_mode(void)
> runtime_code_page_mkexec();
>
> kfree(new_memmap);
> +
> + /* clean DUMMY object */
> + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS,
> + 0, NULL);
> }
>
> /*
> @@ -1136,33 +1046,65 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
> efi_status_t status;
> u64 storage_size, remaining_size, max_size;
>
> + if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
> + return 0;
> +
> status = efi.query_variable_info(attributes, &storage_size,
> &remaining_size, &max_size);
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!max_size && remaining_size > size)
> - printk_once(KERN_ERR FW_BUG "Broken EFI implementation"
> - " is returning MaxVariableSize=0\n");
> /*
> * Some firmware implementations refuse to boot if there's insufficient
> * space in the variable store. We account for that by refusing the
> * write if permitting it would reduce the available space to under
> - * 50%. However, some firmware won't reclaim variable space until
> - * after the used (not merely the actively used) space drops below
> - * a threshold. We can approximate that case with the value calculated
> - * above. If both the firmware and our calculations indicate that the
> - * available space would drop below 50%, refuse the write.
> + * 5KB. This figure was provided by Samsung, so should be safe.
> */
> + if ((remaining_size - size < EFI_MIN_RESERVE) &&
> + !efi_no_storage_paranoia) {
> +
> + /*
> + * Triggering garbage collection may require that the firmware
> + * generate a real EFI_OUT_OF_RESOURCES error. We can force
> + * that by attempting to use more space than is available.
> + */
> + unsigned long dummy_size = remaining_size + 1024;

This looks like it will try to allocate more than the remaining size.
Is that intended?

> + void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
> +
> + status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS,
> + dummy_size, dummy);
> +
> + if (status == EFI_SUCCESS) {
> + /*
> + * This should have failed, so if it didn't make sure
> + * that we delete it...
> + */
> + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS,
> + 0, dummy);
> + }
>
> - if (!storage_size || size > remaining_size ||
> - (max_size && size > max_size))
> - return EFI_OUT_OF_RESOURCES;
> + /*
> + * The runtime code may now have triggered a garbage collection
> + * run, so check the variable info again
> + */
> + status = efi.query_variable_info(attributes, &storage_size,
> + &remaining_size, &max_size);
>
> - if (!efi_no_storage_paranoia &&
> - ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) &&
> - (remaining_size - size < storage_size / 2)))
> - return EFI_OUT_OF_RESOURCES;
> + if (status != EFI_SUCCESS)
> + return status;
> +
> + /*
> + * There still isn't enough room, so return an error
> + */
> + if (remaining_size - size < EFI_MIN_RESERVE)
> + return EFI_OUT_OF_RESOURCES;
> + }
>
> return EFI_SUCCESS;
> }
> --
> 1.8.1.4
>
> --
> Matt Fleming, Intel Open Source Technology Center

--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc [email protected]

2013-06-06 15:00:45

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Thu, 06 Jun, at 09:48:46AM, Russ Anderson wrote:
> This looks like it will try to allocate more than the remaining size.
> Is that intended?

Yes, the intention is to trigger garbage collection.

--
Matt Fleming, Intel Open Source Technology Center

2013-06-06 15:28:46

[permalink] [raw]

Subject: Re: [PATCH] Modify UEFI anti-bricking code

On Thu, Jun 06, 2013 at 04:00:39PM +0100, Matt Fleming wrote:
> On Thu, 06 Jun, at 09:48:46AM, Russ Anderson wrote:
> > This looks like it will try to allocate more than the remaining size.
> > Is that intended?
>
> Yes, the intention is to trigger garbage collection.

OK, if that's what it takes. It just struck me as something
that would fail initial parameter checking in the bios,
before getting far enough to trigger garbage collection.

Since I really want the rest of this patch to go in, I'm certainly
not going to object. :-)

Thanks,
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc [email protected]

2013-06-10 18:54:42