[PATCH] x86, efi: add no_bricked_efi whitelist

[PATCH] x86, efi: add no_bricked_efi whitelist

[Yasuaki Ishimatsu]

By following works, my system very often fails set_variable() to set new
variable to efi variable storage and shows "efivars: set_variable() failed:
status=-28" message.

- commit 68d929862e29a8b52a7f2f2f86a0600423b093cd
    efi: be more paranoid about available space when creating variables
- commit 31ff2f20d9003e74991d135f56e503fe776c127c
    efi: Distinguish between "remaining space" and actually used space
- commit 8c58bf3eec3b8fc8162fe557e9361891c20758f2
    x86,efi: Implement efi_no_storage_paranoia parameter
- commit f8b8404337de4e2466e2e1139ea68b1f8295974f
    Modify UEFI anti-bricking code

When booting my system, remaining space of efi variable storage is about
5KB. So there is no room that sets a new variable to the storage. On my
system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
on pre OS environment with UEFI.

This patch adds whitelist. If a server is in the whitelist,
efi_no_storage_paranoia is set to true. And the system can use all efi
variable storage.

Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
CC: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
CC: Lee, Chun-Y <jlee-IBi9RG/b67k@public.gmane.org>
CC: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c7e22ab..9fadf5d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -116,6 +116,15 @@ static int __init setup_storage_paranoia(char *arg)
 }
 early_param("efi_no_storage_paranoia", setup_storage_paranoia);

+struct no_bricked_efi {
+	char vendor[100];
+	u32 revision;
+};
+
+static struct no_bricked_efi efi_whitelist[] __initdata = {
+	{"FUJITSU LIMITED", 0},
+	{""}
+};

 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
@@ -704,6 +713,24 @@ static int __init efi_memmap_init(void)
 	return 0;
 }

+static bool __init is_no_bricked_efi(char *vendor, u32 revision)
+{
+	int i = 0;
+
+	while (efi_whitelist[i].vendor[0] != '\0') {
+		if (strncmp(efi_whitelist[i].vendor, vendor,
+			    sizeof(efi_whitelist[i].vendor))) {
+			i++;
+			continue;
+		}
+
+		if (revision >= efi_whitelist[i].revision)
+			return true;
+	}
+
+	return false;
+}
+
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
@@ -778,6 +805,8 @@ void __init efi_init(void)
 #if EFI_DEBUG
 	print_efi_memmap();
 #endif
+	if (is_no_bricked_efi(vendor, efi.systab->hdr.revision))
+		efi_no_storage_paranoia = true;
 }

 void __init efi_late_init(void)

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Richard Weinberger]

Am Mittwoch, 20. November 2013, 17:34:18 schrieb Yasuaki Ishimatsu:
> By following works, my system very often fails set_variable() to set new
> variable to efi variable storage and shows "efivars: set_variable() failed:
> status=-28" message.
> 
> - commit 68d929862e29a8b52a7f2f2f86a0600423b093cd
>     efi: be more paranoid about available space when creating variables
> - commit 31ff2f20d9003e74991d135f56e503fe776c127c
>     efi: Distinguish between "remaining space" and actually used space
> - commit 8c58bf3eec3b8fc8162fe557e9361891c20758f2
>     x86,efi: Implement efi_no_storage_paranoia parameter
> - commit f8b8404337de4e2466e2e1139ea68b1f8295974f
>     Modify UEFI anti-bricking code
> 
> When booting my system, remaining space of efi variable storage is about
> 5KB. So there is no room that sets a new variable to the storage. On my
> system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
> environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
> threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
> on pre OS environment with UEFI.
> 
> This patch adds whitelist. If a server is in the whitelist,
> efi_no_storage_paranoia is set to true. And the system can use all efi
> variable storage.
> 
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> CC: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
> CC: Lee, Chun-Y <jlee-IBi9RG/b67k@public.gmane.org>
> CC: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index c7e22ab..9fadf5d 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -116,6 +116,15 @@ static int __init setup_storage_paranoia(char *arg)
>  }
>  early_param("efi_no_storage_paranoia", setup_storage_paranoia);
> 
> +struct no_bricked_efi {
> +	char vendor[100];
> +	u32 revision;
> +};
> +
> +static struct no_bricked_efi efi_whitelist[] __initdata = {
> +	{"FUJITSU LIMITED", 0},

So, no UEFI from Fujitsu on planet earth suffers from such issues?
How can you guarantee that?

Thanks,
//richard

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Matthew Garrett]

On Wed, 2013-11-20 at 17:34 +0900, Yasuaki Ishimatsu wrote:

> When booting my system, remaining space of efi variable storage is about
> 5KB. So there is no room that sets a new variable to the storage. On my
> system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
> environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
> threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
> on pre OS environment with UEFI.

By "remaining space is about 5KB", do you mean that the rest of the
space is filled with variables or do you mean that there is more free
space but it hasn't been garbage collected?

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Yasuaki Ishimatsu]

(2013/11/20 17:54), Richard Weinberger wrote:
> Am Mittwoch, 20. November 2013, 17:34:18 schrieb Yasuaki Ishimatsu:
>> By following works, my system very often fails set_variable() to set new
>> variable to efi variable storage and shows "efivars: set_variable() failed:
>> status=-28" message.
>>
>> - commit 68d929862e29a8b52a7f2f2f86a0600423b093cd
>>      efi: be more paranoid about available space when creating variables
>> - commit 31ff2f20d9003e74991d135f56e503fe776c127c
>>      efi: Distinguish between "remaining space" and actually used space
>> - commit 8c58bf3eec3b8fc8162fe557e9361891c20758f2
>>      x86,efi: Implement efi_no_storage_paranoia parameter
>> - commit f8b8404337de4e2466e2e1139ea68b1f8295974f
>>      Modify UEFI anti-bricking code
>>
>> When booting my system, remaining space of efi variable storage is about
>> 5KB. So there is no room that sets a new variable to the storage. On my
>> system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
>> environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
>> threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
>> on pre OS environment with UEFI.
>>
>> This patch adds whitelist. If a server is in the whitelist,
>> efi_no_storage_paranoia is set to true. And the system can use all efi
>> variable storage.
>>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>> CC: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
>> CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
>> CC: Lee, Chun-Y <jlee-IBi9RG/b67k@public.gmane.org>
>> CC: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> CC: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>   arch/x86/platform/efi/efi.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index c7e22ab..9fadf5d 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -116,6 +116,15 @@ static int __init setup_storage_paranoia(char *arg)
>>   }
>>   early_param("efi_no_storage_paranoia", setup_storage_paranoia);
>>
>> +struct no_bricked_efi {
>> +	char vendor[100];
>> +	u32 revision;
>> +};
>> +
>> +static struct no_bricked_efi efi_whitelist[] __initdata = {
>> +	{"FUJITSU LIMITED", 0},
>
> So, no UEFI from Fujitsu on planet earth suffers from such issues?
> How can you guarantee that?

Thank you for comments.
I cannot guarantee that. How about using Product Name gotten by
dmi_get_system_info(DMI_PRODUCT_NAME) instead of Vendor Name.?

Thanks,
Yasuaki Ishimatsu

>
> Thanks,
> //richard
>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Yasuaki Ishimatsu]

(2013/11/21 1:02), Matthew Garrett wrote:
> On Wed, 2013-11-20 at 17:34 +0900, Yasuaki Ishimatsu wrote:
>
>> When booting my system, remaining space of efi variable storage is about
>> 5KB. So there is no room that sets a new variable to the storage. On my
>> system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
>> environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
>> threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
>> on pre OS environment with UEFI.
>
> By "remaining space is about 5KB", do you mean that the rest of the
> space is filled with variables or do you mean that there is more free
> space but it hasn't been garbage collected?
>

Remaining space is free space that can be used by efi variable. But by 5KB
threshold, we cannot use the space while running OS.

Thanks,
Yasuaki Ishimatsu

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Matthew Garrett]

On Thu, 2013-11-21 at 18:35 +0900, Yasuaki Ishimatsu wrote:

> Remaining space is free space that can be used by efi variable. But by 5KB
> threshold, we cannot use the space while running OS.

Is the situation something like(assuming a 128KB flash part):

1) 64KB of variables, 59KB of deleted variables, 5KB of free space,

or

2) 123KB of variables, 5KB of free space

?
-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Yasuaki Ishimatsu]

(2013/11/22 5:12), Matthew Garrett wrote:
> On Thu, 2013-11-21 at 18:35 +0900, Yasuaki Ishimatsu wrote:
>
>> Remaining space is free space that can be used by efi variable. But by 5KB
>> threshold, we cannot use the space while running OS.
>
> Is the situation something like(assuming a 128KB flash part):
>
> 1) 64KB of variables, 59KB of deleted variables, 5KB of free space,

My situation looks like this. The free space spreads out if gc runs.

Thanks,
Yasuaki Ishimatsu


>
> or
>
> 2) 123KB of variables, 5KB of free space
>
> ?
>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Matthew Garrett]

On Fri, 2013-11-22 at 09:00 +0900, Yasuaki Ishimatsu wrote:
> (2013/11/22 5:12), Matthew Garrett wrote:
> > On Thu, 2013-11-21 at 18:35 +0900, Yasuaki Ishimatsu wrote:
> >
> >> Remaining space is free space that can be used by efi variable. But by 5KB
> >> threshold, we cannot use the space while running OS.
> >
> > Is the situation something like(assuming a 128KB flash part):
> >
> > 1) 64KB of variables, 59KB of deleted variables, 5KB of free space,
> 
> My situation looks like this. The free space spreads out if gc runs.

Ok. In that case Joey's suggestion that we try forcing a GC run in the
boot stub if free space < (threshold * 2) might be the best plan.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Yasuaki Ishimatsu]

(2013/11/22 9:03), Matthew Garrett wrote:
> On Fri, 2013-11-22 at 09:00 +0900, Yasuaki Ishimatsu wrote:
>> (2013/11/22 5:12), Matthew Garrett wrote:
>>> On Thu, 2013-11-21 at 18:35 +0900, Yasuaki Ishimatsu wrote:
>>>
>>>> Remaining space is free space that can be used by efi variable. But by 5KB
>>>> threshold, we cannot use the space while running OS.
>>>
>>> Is the situation something like(assuming a 128KB flash part):
>>>
>>> 1) 64KB of variables, 59KB of deleted variables, 5KB of free space,
>>
>> My situation looks like this. The free space spreads out if gc runs.
>
> Ok. In that case Joey's suggestion that we try forcing a GC run in the
> boot stub if free space < (threshold * 2) might be the best plan.
>
 > 2) 123KB of variables, 5KB of free space

I don't know how to implement your idea. But if GC run by your idea, the free
space may spread out. So 1) case is solved. But it does not solve 2) case.
In my opinion, if system has sane firmware, all nvram strage should be used.
I will send whiltelist version which used DMI infomation. Please review it.

Thanks,
Yasuaki Ishimatsu

[PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Yasuaki Ishimatsu]

By following works, my system very often fails set_variable() to set new
variable to efi variable storage and shows "efivars: set_variable() failed:
status=-28" message.

- commit 68d929862e29a8b52a7f2f2f86a0600423b093cd
     efi: be more paranoid about available space when creating variables
- commit 31ff2f20d9003e74991d135f56e503fe776c127c
     efi: Distinguish between "remaining space" and actually used space
- commit 8c58bf3eec3b8fc8162fe557e9361891c20758f2
     x86,efi: Implement efi_no_storage_paranoia parameter
- commit f8b8404337de4e2466e2e1139ea68b1f8295974f
     Modify UEFI anti-bricking code

When booting my system, remaining space of efi variable storage is about
5KB. So there is no room that sets a new variable to the storage. On my
system, trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS
environment with UEFI. So if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte
threshold, nvram storage cannot be used until EFI_OUT_OF_RESOURCES occurs
on pre OS environment with UEFI.

This patch adds whitelist. If a server is in the whitelist,
efi_no_storage_paranoia is set to true. And the system can use all efi
variable storage.

Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
CC: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
CC: Lee, Chun-Y <jlee-IBi9RG/b67k@public.gmane.org>
CC: Madper Xie <cxie-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

---
  arch/x86/platform/efi/efi.c | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c7e22ab..f9fad62 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -42,6 +42,7 @@
  #include <linux/io.h>
  #include <linux/reboot.h>
  #include <linux/bcd.h>
+#include <linux/dmi.h>

  #include <asm/setup.h>
  #include <asm/efi.h>
@@ -116,6 +117,24 @@ static int __init setup_storage_paranoia(char *arg)
  }
  early_param("efi_no_storage_paranoia", setup_storage_paranoia);

+static struct dmi_system_id efi_whitelist_table[] __initdata = {
+	{
+		/* FUJITSU PRIMEQUEST */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "PRIMEQUEST"),
+		},
+	},
+	{}
+};
+
+static int __init init_efi_no_storage_paranoia(void)
+{
+	if (dmi_check_system(efi_whitelist_table))
+		efi_no_storage_paranoia = true;
+	return 0;
+}
+late_initcall(init_efi_no_storage_paranoia);

  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
  {

Re: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Matthew Garrett]

On Fri, 2013-11-22 at 15:29 +0900, Yasuaki Ishimatsu wrote:

> +			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "PRIMEQUEST"),

Can we guarantee that no Primequests have this issue? Can we guarantee
that that no prior firmware versions have this issue? Can we guarantee
that no future firmware versions will have this issue?

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH] x86, efi: add no_bricked_efi whitelist

[Matthew Garrett]

On Fri, 2013-11-22 at 15:23 +0900, Yasuaki Ishimatsu wrote:

> I don't know how to implement your idea. But if GC run by your idea, the free
> space may spread out. So 1) case is solved. But it does not solve 2) case.
> In my opinion, if system has sane firmware, all nvram strage should be used.
> I will send whiltelist version which used DMI infomation. Please review it.

We don't want to solve (2). The benefit in doing so is small compared to
the cost of maintaining a whitelist or risking breaking systems.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Yasuaki Ishimatsu]

(2013/11/23 2:47), Matthew Garrett wrote:
> On Fri, 2013-11-22 at 15:29 +0900, Yasuaki Ishimatsu wrote:
>
>> +			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "PRIMEQUEST"),
>
> Can we guarantee that no Primequests have this issue? Can we guarantee
> that that no prior firmware versions have this issue? Can we guarantee
> that no future firmware versions will have this issue?
>

Sorry for the late reply. I contacted our firmware team to confirm them.
They said that current implementation, thereshold is 5KB, is strange.

Currently many EFI firmwares are based on EFI Development Kit (EDK).
Accoding to EDK source, gc on boot time runs when free space of nvram
is less than MaximumVariableSize of QueryVariableInfo(). In my system
MaximumVariableSize is 1KB. So when free space reaches less than 5KB
and I reboot the system, gc does not run on boot time in my system.

Threshold of gc depends on a system. So at least, 5KB threshold must be
changed on each system.

Thanks,
Yasuaki Ishimatsu

RE: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Matthew Garrett]

The threshold of 5K is based on information we received from vendors. We won't be reducing it. Can you confirm that attempting to create a variable larger than the remaining space in boot services will trigger garbage collection?--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Yasuaki Ishimatsu]

(2013/12/03 12:45), Matthew Garrett wrote:
> The threshold of 5K is based on information we received from vendors. We won't be reducing it. Can you confirm that attempting to create a variable larger than the remaining space in boot services will trigger garbage collection?
>

What boot service is the "boot services" in these sentences?

Thanks,
Yasuaki Ishimatsu

Re: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Matthew Garrett]

On Tue, Dec 03, 2013 at 01:49:04PM +0900, Yasuaki Ishimatsu wrote:

> What boot service is the "boot services" in these sentences?

The environment prior to ExitBootServices() being called.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86, efi: add efi_whitelist_table to use all efi variable storage

[Yasuaki Ishimatsu]

(2013/12/03 14:04), Matthew Garrett wrote:
> On Tue, Dec 03, 2013 at 01:49:04PM +0900, Yasuaki Ishimatsu wrote:
>
>> What boot service is the "boot services" in these sentences?
>
> The environment prior to ExitBootServices() being called.
>

I see.
I'll check it.

Thank,
Yasuaki Ishimatsu