Re: efi: be more paranoid about available space when creating variables

Re: efi: be more paranoid about available space when creating variables

[Fleming, Matt]

On 23/03/13 03:47, Ben Hutchings wrote:
> On Sat, 2013-03-23 at 02:16 +0000, Ben Hutchings wrote:
>> Commit 68d929862e29 'efi: be more paranoid about available space when
>> creating variables' caused a regression on my EFI-booting desktop.
>> (I'm actually testing on 3.2.41 with several more fixes backported, but
>> this shouldn't matter.)
>>
>> The meaningful DMI IDs for this system are:
>>
>> /sys/devices/virtual/dmi/id/bios_date:08/22/2012
>> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
>> /sys/devices/virtual/dmi/id/bios_version:4003
>> /sys/devices/virtual/dmi/id/board_name:P8Z68-V LX
>> /sys/devices/virtual/dmi/id/board_vendor:ASUSTeK Computer INC.
>>
> So it looks like we may actually have to disable all the size checks on
> this firmware!

Well, crap. I guess this was bound to be an issue sooner or later.
Thanks for doing the investigative work.

Looks like we're going to have to go down the road of blacklisting
known bad machines after all. I was reluctant to push for that
originally as the result of missing a machine from the blacklist might
result in said machine becoming bricked, but we've now got patches in
place to turn off the pstore efivars code, and I suspect that most
distros indeed do that.

Re: efi: be more paranoid about available space when creating variables

[Matthew Garrett]

On Sat, 2013-03-23 at 20:24 +0000, Fleming, Matt wrote:

> Looks like we're going to have to go down the road of blacklisting
> known bad machines after all. I was reluctant to push for that
> originally as the result of missing a machine from the blacklist might
> result in said machine becoming bricked, but we've now got patches in
> place to turn off the pstore efivars code, and I suspect that most
> distros indeed do that.

Handwaving idea - leave the size restriction in place for pstore. Remove
it for normal setvariable calls. Downside of this is that pstore will be
broken on machines that only clear space when the variable store is
mostly full, but it should avoid cases where we automatically kill
machines without also having to disable a useful feature.

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

Re: efi: be more paranoid about available space when creating variables

[Matt Fleming]

(Let's try this again without using HTML)

On 23/03/13 20:32, Matthew Garrett wrote:
> On Sat, 2013-03-23 at 20:24 +0000, Fleming, Matt wrote:
> 
>> Looks like we're going to have to go down the road of blacklisting
>> known bad machines after all. I was reluctant to push for that
>> originally as the result of missing a machine from the blacklist might
>> result in said machine becoming bricked, but we've now got patches in
>> place to turn off the pstore efivars code, and I suspect that most
>> distros indeed do that.
> 
> Handwaving idea - leave the size restriction in place for pstore. Remove
> it for normal setvariable calls. Downside of this is that pstore will be
> broken on machines that only clear space when the variable store is
> mostly full, but it should avoid cases where we automatically kill
> machines without also having to disable a useful feature.

Right, but it will still be possible to brick machines via the code path
that doesnt check the remaining storage space. Its less likely sure, but
its still entirely possible.

The most robust solution may be to combine the two approaches - remove the
size checks from everything but the pstore paths but enforce them across
the board for known problem machines?

Re: efi: be more paranoid about available space when creating variables

[Matthew Garrett]

Ok, so having thought some more about the actual problem (ie, Samsungs 
go wrong if there's too much used space marked as being active, not 
merely too much used space) I think we really want to be looking for the 
amount of active space rather than the remaining space value that 
QueryVariableInfo() gives us. Something like this (absolutely untested, 
provided here for comment) patch? I'll try rigging something up under 
OVMF to test it. This version certainly seems a little over-naive, since 
it won't handle the case of a variable that's being overwritten with 
something of a different size.

commit 263c2ee36c67dfa6d869304a3b5aef7a14f1ec4e
Author: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
Date:   Mon Mar 25 13:40:28 2013 -0400

    efi: Distinguish between "remaining space" and actually used space
    
    EFI implementations distinguish between space that is actively used by a
    variable and space that merely hasn't been garbage collected yet. Space
    that hasn't yet been garbage collected isn't available for use and so isn't
    counted in the remaining_space field returned by QueryVariableInfo().
    
    Combined with commit 68d9298 this can cause problems. Some implementations
    don't garbage collect until the remaining space is smaller than the maximum
    variable size, and as a result check_var_size() will always fail once more
    than 50% of the variable store has been used even if most of that space is
    marked as available for garbage collection. The user is unable to create
    new variables, and deleting variables doesn't increase the remaining space.
    
    The problem that 68d9298 was attempting to avoid was one where certain
    platforms fail if the actively used space is greater than 50% of the
    available storage space. We should be able to calculate that by simply
    summing the size of each available variable and subtracting that from
    the total storage space. With luck this will fix the problem described in
    https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
    damage to occur to the machines 68d9298 was attempting to fix.
    
    Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7acafb8..731ac7b 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -436,9 +436,12 @@ static efi_status_t
 check_var_size_locked(struct efivars *efivars, u32 attributes,
 			unsigned long size)
 {
-	u64 storage_size, remaining_size, max_size;
+	u64 storage_size, remaining_size, max_size, active_available;
+	struct efivar_entry *entry;
+	struct efi_variable *var;
 	efi_status_t status;
 	const struct efivar_operations *fops = efivars->ops;
+	unsigned long active_size = 0;
 
 	if (!efivars->ops->query_variable_info)
 		return EFI_UNSUPPORTED;
@@ -449,8 +452,16 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
 	if (status != EFI_SUCCESS)
 		return status;
 
+	list_for_each_entry(entry, &efivars->list, list) {
+		var = &entry->var;
+		get_var_data_locked(efivars, var);
+		active_size += var->DataSize;
+	}
+
+	active_available = storage_size - active_size;
+
 	if (!storage_size || size > remaining_size || size > max_size ||
-	    (remaining_size - size) < (storage_size / 2))
+	    (active_available - size) < (storage_size / 2))
 		return EFI_OUT_OF_RESOURCES;
 
 	return status;

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: efi: be more paranoid about available space when creating variables

[Lingzhu Xiang]

On 03/26/2013 11:56 AM, Matthew Garrett wrote:
> Ok, so having thought some more about the actual problem (ie, Samsungs
> go wrong if there's too much used space marked as being active, not
> merely too much used space) I think we really want to be looking for the
> amount of active space rather than the remaining space value that
> QueryVariableInfo() gives us. Something like this (absolutely untested,
> provided here for comment) patch? I'll try rigging something up under
> OVMF to test it. This version certainly seems a little over-naive, since
> it won't handle the case of a variable that's being overwritten with
> something of a different size.
>
> commit 263c2ee36c67dfa6d869304a3b5aef7a14f1ec4e
> Author: Matthew Garrett <matthew.garrett@nebula.com>
> Date:   Mon Mar 25 13:40:28 2013 -0400
>
>      efi: Distinguish between "remaining space" and actually used space
>
>      EFI implementations distinguish between space that is actively used by a
>      variable and space that merely hasn't been garbage collected yet. Space
>      that hasn't yet been garbage collected isn't available for use and so isn't
>      counted in the remaining_space field returned by QueryVariableInfo().
>
>      Combined with commit 68d9298 this can cause problems. Some implementations
>      don't garbage collect until the remaining space is smaller than the maximum
>      variable size, and as a result check_var_size() will always fail once more
>      than 50% of the variable store has been used even if most of that space is
>      marked as available for garbage collection. The user is unable to create
>      new variables, and deleting variables doesn't increase the remaining space.

Do you mean they don't garbage collect across reboots?

>      The problem that 68d9298 was attempting to avoid was one where certain
>      platforms fail if the actively used space is greater than 50% of the
>      available storage space. We should be able to calculate that by simply
>      summing the size of each available variable and subtracting that from
>      the total storage space. With luck this will fix the problem described in

What about the size of variable name, paddings, headers (especially 
authenticated ones) and other internal stuff?

>      https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
>      damage to occur to the machines 68d9298 was attempting to fix.

This bug is reporting "query_variable_info sets max_size as zero". If 
that's true, this patch doesn't seem to account for that case.

>
>      Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7acafb8..731ac7b 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -436,9 +436,12 @@ static efi_status_t
>   check_var_size_locked(struct efivars *efivars, u32 attributes,
>   			unsigned long size)
>   {
> -	u64 storage_size, remaining_size, max_size;
> +	u64 storage_size, remaining_size, max_size, active_available;
> +	struct efivar_entry *entry;
> +	struct efi_variable *var;
>   	efi_status_t status;
>   	const struct efivar_operations *fops = efivars->ops;
> +	unsigned long active_size = 0;
>
>   	if (!efivars->ops->query_variable_info)
>   		return EFI_UNSUPPORTED;
> @@ -449,8 +452,16 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>   	if (status != EFI_SUCCESS)
>   		return status;
>
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		get_var_data_locked(efivars, var);
> +		active_size += var->DataSize;
> +	}
> +
> +	active_available = storage_size - active_size;
> +
>   	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	    (active_available - size) < (storage_size / 2))
>   		return EFI_OUT_OF_RESOURCES;
>
>   	return status;
>

Re: efi: be more paranoid about available space when creating variables

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

On Tue, 2013-03-26 at 12:31 +0800, Lingzhu Xiang wrote:
> On 03/26/2013 11:56 AM, Matthew Garrett wrote:
> > Ok, so having thought some more about the actual problem (ie, Samsungs
> > go wrong if there's too much used space marked as being active, not
> > merely too much used space) I think we really want to be looking for the
> > amount of active space rather than the remaining space value that
> > QueryVariableInfo() gives us. Something like this (absolutely untested,
> > provided here for comment) patch? I'll try rigging something up under
> > OVMF to test it. This version certainly seems a little over-naive, since
> > it won't handle the case of a variable that's being overwritten with
> > something of a different size.
> >
> > commit 263c2ee36c67dfa6d869304a3b5aef7a14f1ec4e
> > Author: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> > Date:   Mon Mar 25 13:40:28 2013 -0400
> >
> >      efi: Distinguish between "remaining space" and actually used space
> >
> >      EFI implementations distinguish between space that is actively used by a
> >      variable and space that merely hasn't been garbage collected yet. Space
> >      that hasn't yet been garbage collected isn't available for use and so isn't
> >      counted in the remaining_space field returned by QueryVariableInfo().
> >
> >      Combined with commit 68d9298 this can cause problems. Some implementations
> >      don't garbage collect until the remaining space is smaller than the maximum
> >      variable size, and as a result check_var_size() will always fail once more
> >      than 50% of the variable store has been used even if most of that space is
> >      marked as available for garbage collection. The user is unable to create
> >      new variables, and deleting variables doesn't increase the remaining space.
> 
> Do you mean they don't garbage collect across reboots?
[...]

Yes, see <http://article.gmane.org/gmane.linux.kernel.stable/47156>.

Ben.

-- 
Ben Hutchings
I'm not a reverse psychological virus.  Please don't copy me into your sig.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: efi: be more paranoid about available space when creating variables

[Matthew Garrett]

On Tue, Mar 26, 2013 at 12:31:11PM +0800, Lingzhu Xiang wrote:
> On 03/26/2013 11:56 AM, Matthew Garrett wrote:
> >     Combined with commit 68d9298 this can cause problems. Some implementations
> >     don't garbage collect until the remaining space is smaller than the maximum
> >     variable size, and as a result check_var_size() will always fail once more
> >     than 50% of the variable store has been used even if most of that space is
> >     marked as available for garbage collection. The user is unable to create
> >     new variables, and deleting variables doesn't increase the remaining space.
> 
> Do you mean they don't garbage collect across reboots?

Only if the remaining space has fallen below a threshold.

> >     The problem that 68d9298 was attempting to avoid was one where certain
> >     platforms fail if the actively used space is greater than 50% of the
> >     available storage space. We should be able to calculate that by simply
> >     summing the size of each available variable and subtracting that from
> >     the total storage space. With luck this will fix the problem described in
> 
> What about the size of variable name, paddings, headers (especially
> authenticated ones) and other internal stuff?

Ah, true, it should at least include the name. As for padding, headers 
etc - these are great questions and ideally we'd get some feedback from 
(say) Samsung as to how much extra data is used in the variables.

> >     https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
> >     damage to occur to the machines 68d9298 was attempting to fix.
> 
> This bug is reporting "query_variable_info sets max_size as zero".
> If that's true, this patch doesn't seem to account for that case.

Oh huh. You're right, I missed that. In which case we should remove that 
chunk - presumably we'll get an error on write then anyway. It does 
appear to have fixed Oleg's problem in the same bug.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: efi: be more paranoid about available space when creating variables

[Lingzhu Xiang]

On 03/26/2013 11:56 AM, Matthew Garrett wrote:
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7acafb8..731ac7b 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -436,9 +436,12 @@ static efi_status_t
>   check_var_size_locked(struct efivars *efivars, u32 attributes,
>   			unsigned long size)
>   {
> -	u64 storage_size, remaining_size, max_size;
> +	u64 storage_size, remaining_size, max_size, active_available;
> +	struct efivar_entry *entry;
> +	struct efi_variable *var;
>   	efi_status_t status;
>   	const struct efivar_operations *fops = efivars->ops;
> +	unsigned long active_size = 0;
>
>   	if (!efivars->ops->query_variable_info)
>   		return EFI_UNSUPPORTED;
> @@ -449,8 +452,16 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>   	if (status != EFI_SUCCESS)
>   		return status;
>
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		get_var_data_locked(efivars, var);

Check the return value here? Like for an empty efivarfs file,
it would return EFI_NOT_FOUND and active_size += 1024.

> +		active_size += var->DataSize;
> +	}
> +
> +	active_available = storage_size - active_size;
> +
>   	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	    (active_available - size) < (storage_size / 2))
>   		return EFI_OUT_OF_RESOURCES;
>
>   	return status;
>

Re: efi: be more paranoid about available space when creating variables

[Matt Fleming]

On 26/03/13 03:56, Matthew Garrett wrote:
> Ok, so having thought some more about the actual problem (ie, Samsungs 
> go wrong if there's too much used space marked as being active, not 
> merely too much used space) I think we really want to be looking for the 
> amount of active space rather than the remaining space value that 
> QueryVariableInfo() gives us. Something like this (absolutely untested, 
> provided here for comment) patch? I'll try rigging something up under 
> OVMF to test it. This version certainly seems a little over-naive, since 
> it won't handle the case of a variable that's being overwritten with 
> something of a different size.

Hmm... I'm not convinced this will provide a long-term solution. Is there
anything that mandates that the size of all variables has to correlate
sensibly
with the results from QueryVariableInfo()? Even if there is in theory, I
doubt
it'll be true everywhere in practice, and trying to workaround
implementation
bugs in our workarounds for other bugs is the path to madness.

We can't continue this approach where we attempt to guess how the firmware
implements variable storage, because as we've seen, there are various
schemes
out there. We're taking one step forward and two steps backwards at the
moment
with these kinds of patches. As you've seen, it's like playing
whac-a-mole :-(

I think, rather unfortunately, it comes down to solving these problems on a
case-by-case basis. We've tried broad, all-encompassing solutions, but
they're
just not working. Lets contain the damage with a blacklist, because even
if the
list is really vague, i.e. "Apply size checks for all Samsung and Lenovo
machines with Phoenix BIOS", it's still more relaxed than what we have
now, and
there's more chance of finding a single way to reliably workaround the
bug if
we restrict it to one IBV and a couple of OEMs.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: efi: be more paranoid about available space when creating variables

[Matthew Garrett]

On Tue, Mar 26, 2013 at 10:37:13AM +0000, Matt Fleming wrote:

> Hmm... I'm not convinced this will provide a long-term solution. Is there
> anything that mandates that the size of all variables has to correlate
> sensibly
> with the results from QueryVariableInfo()? Even if there is in theory, I
> doubt
> it'll be true everywhere in practice, and trying to workaround
> implementation
> bugs in our workarounds for other bugs is the path to madness.

I'm not quite clear what you mean. We have a fairly solid idea as to 
what the underlying problem here is, and I don't think this makes any 
more assumptions than the existing code does.

> We can't continue this approach where we attempt to guess how the firmware
> implements variable storage, because as we've seen, there are various
> schemes
> out there. We're taking one step forward and two steps backwards at the
> moment
> with these kinds of patches. As you've seen, it's like playing
> whac-a-mole :-(

That's always been the way we end up dealing with firmware interactions. 
Read the git log for the ACPI video driver for an example...

> I think, rather unfortunately, it comes down to solving these problems on a
> case-by-case basis. We've tried broad, all-encompassing solutions, but
> they're
> just not working. Lets contain the damage with a blacklist, because even
> if the
> list is really vague, i.e. "Apply size checks for all Samsung and Lenovo
> machines with Phoenix BIOS", it's still more relaxed than what we have
> now, and
> there's more chance of finding a single way to reliably workaround the
> bug if
> we restrict it to one IBV and a couple of OEMs.

I'm kind of reluctant, because the cost of getting this wrong is 
hardware damage. Right now we believe that we're resistant to hardware 
damage but have introduced another problem in the process. If we can 
find a way to satisfy both constraints then I think that that's 
worthwhile.

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

RE: efi: be more paranoid about available space when creating variables

[Seiji Aguchi]

>    We should be able to calculate that by simply
>     summing the size of each available variable and subtracting that from
>     the total storage space. With luck this will fix the problem described in
>     https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
>     damage to occur to the machines 68d9298 was attempting to fix.

I like the idea of this patch. It could be a long term solution.

If we can calculate the size of available variables by ourselves,
 we don't need to call QueryVariableInfo().
It makes efivars robust against buggy firmwares.
Also, this patch actually fixes a serious bug, failing boot up.

In theory, QueryVariableInfo() should return accurate values to kernel.
But, it doesn't in practice.

I think creating blacklist will be like playing whac-a-mole, because
we can't anticipate how many buggy firmwares will appear in the future.

Seiji

Re: efi: be more paranoid about available space when creating variables

[Matt Fleming]

On 26/03/13 15:43, Matthew Garrett wrote:
> I'm not quite clear what you mean. We have a fairly solid idea as to 
> what the underlying problem here is, and I don't think this makes any 
> more assumptions than the existing code does.

Right, it doesn't make more assumptions, but the assumptions that the
existing code makes is causing problems, hence the need for your updated
patch. I'm fully expecting that the updated patch will also cause us
problems/require additional patches. I really don't want to keep
patching/tweaking this all the way to -rc6 and beyond.

Have you tested this patch against one of those machines that suffers
from the original bricking problem? Ben, does this patch fix your issue?
I dug my buggy-as-hell ASUS machine out from under my desk and it
suffers from the max_size == 0 problem which this patch doesn't fix, but
that's already been discussed.

> I'm kind of reluctant, because the cost of getting this wrong is 
> hardware damage. Right now we believe that we're resistant to hardware 
> damage but have introduced another problem in the process. If we can 
> find a way to satisfy both constraints then I think that that's 
> worthwhile.

I'm concerned we're going to end up introducing more constraints because
some other class of platforms doesn't work with this patch. It expects
DataSize and storage_size to correlate meaningfully, and I'm not at all
convinced that will be true everywhere, even though it's not an
unreasonable expectation.

Maybe we should leave the existing checks in place and create a
whitelist for those machines that absolutely must be able to write to
the variable store, e.g. to initiate garbage collection, or where
QueryVariableInfo() is known to be inaccurate, or where we know we can
have the full use of pstore without bricking the machine. Yes, we'll
have to maintain the whitelist and we'll no doubt get bug reports for
machines that need to be added to the list, but at least no one is going
to brick their laptops, and the fix is simply adding an ID to the list,
not reimplementing our workarounds with the risk of breaking X number of
machines that were previously working just fine.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: efi: be more paranoid about available space when creating variables

[Matt Fleming]

On 26/03/13 23:33, Seiji Aguchi wrote:
> If we can calculate the size of available variables by ourselves,
>  we don't need to call QueryVariableInfo().
> It makes efivars robust against buggy firmwares.
> Also, this patch actually fixes a serious bug, failing boot up.
> 
> In theory, QueryVariableInfo() should return accurate values to kernel.
> But, it doesn't in practice.

Exactly and this patch is *still* relying on the output of
QueryVariableInfo(). I doubt DataSize will be accurate everywhere either.

> I think creating blacklist will be like playing whac-a-mole, because
> we can't anticipate how many buggy firmwares will appear in the future.

You wouldn't need to reimplement the workaround for every broken
machine, you just add the machine's identifiers to the list. And no, we
can't anticipate the firmware bugs we'll see in the future which is why
we need to solve this on a case-by-case basis.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: efi: be more paranoid about available space when creating variables

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

On Wed, 2013-03-27 at 09:09 +0000, Matt Fleming wrote:
> On 26/03/13 15:43, Matthew Garrett wrote:
> > I'm not quite clear what you mean. We have a fairly solid idea as to 
> > what the underlying problem here is, and I don't think this makes any 
> > more assumptions than the existing code does.
> 
> Right, it doesn't make more assumptions, but the assumptions that the
> existing code makes is causing problems, hence the need for your updated
> patch. I'm fully expecting that the updated patch will also cause us
> problems/require additional patches. I really don't want to keep
> patching/tweaking this all the way to -rc6 and beyond.
> 
> Have you tested this patch against one of those machines that suffers
> from the original bricking problem? Ben, does this patch fix your issue?

It doesn't entirely fix it - that would require ignoring the max_size
and remaining_size values, and only considering available_size.

But it does mean that the system will only rarely get into a state where
efibootmgr can't write variables, and that it should take only a few
reboots to get out of that.

[...]
> Maybe we should leave the existing checks in place and create a
> whitelist for those machines that absolutely must be able to write to
> the variable store, e.g. to initiate garbage collection, or where
> QueryVariableInfo() is known to be inaccurate, or where we know we can
> have the full use of pstore without bricking the machine. Yes, we'll
> have to maintain the whitelist and we'll no doubt get bug reports for
> machines that need to be added to the list, but at least no one is going
> to brick their laptops, and the fix is simply adding an ID to the list,
> not reimplementing our workarounds with the risk of breaking X number of
> machines that were previously working just fine.

Let's also have a module parameter to set this behaviour, and some more
useful error logging in check_var_size_locked() that can help people to
determine the behaviour of their firmware.

Ben.

-- 
Ben Hutchings
DNRC Motto:  I can please only one person per day.
Today is not your day.  Tomorrow isn't looking good either.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Matthew Garrett]

EFI variables can be flagged as being accessible only within boot services.
This makes it awkward for us to figure out how much space they use at
runtime. In theory we could figure this out by simply comparing the results
from QueryVariableInfo() to the space used by all of our variables, but
that fails if the platform doesn't garbage collect on every boot. Thankfully,
calling QueryVariableInfo() while still inside boot services gives a more
reliable answer. This patch passes that information from the EFI boot stub
up to the efivars code, letting us calculate a reasonably accurate value.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 arch/x86/boot/compressed/eboot.c      | 47 +++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/efi.h            | 10 ++++++++
 arch/x86/include/uapi/asm/bootparam.h |  1 +
 arch/x86/platform/efi/efi.c           | 21 ++++++++++++++++
 drivers/firmware/efivars.c            | 29 +++++++++++++++++++++
 5 files changed, 108 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c205035..8615f75 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	*size = len;
 }
 
+static efi_status_t setup_efi_vars(struct boot_params *params)
+{
+	struct setup_data *data;
+	struct efi_var_bootdata *efidata;
+	u64 store_size, remaining_size, var_size;
+	efi_status_t status;
+
+	if (!sys_table->runtime->query_variable_info)
+		return EFI_UNSUPPORTED;
+
+	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+
+	while (data && data->next)
+		data = (struct setup_data *)(unsigned long)data->next;
+
+	status = efi_call_phys4(sys_table->runtime->query_variable_info,
+				EFI_VARIABLE_NON_VOLATILE |
+				EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
+				&remaining_size, &var_size);
+
+	if (status != EFI_SUCCESS)
+		return status;
+
+	status = efi_call_phys3(sys_table->boottime->allocate_pool,
+				EFI_LOADER_DATA, sizeof(*efidata), &efidata);
+
+	if (status != EFI_SUCCESS)
+		return status;
+
+	efidata->data.type = SETUP_EFI_VARS;
+	efidata->data.len = sizeof(struct efi_var_bootdata) -
+		sizeof(struct setup_data);
+	efidata->data.next = 0;
+	efidata->store_size = store_size;
+	efidata->remaining_size = remaining_size;
+	efidata->max_var_size = var_size;
+
+	if (data)
+		data->next = (unsigned long)efidata;
+	else
+		params->hdr.setup_data = (unsigned long)efidata;
+
+}
+
 static efi_status_t setup_efi_pci(struct boot_params *params)
 {
 	efi_pci_io_protocol *pci;
@@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 
 	setup_graphics(boot_params);
 
+	setup_efi_vars(boot_params);
+
 	setup_efi_pci(boot_params);
 
 	status = efi_call_phys3(sys_table->boottime->allocate_pool,
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 60c89f3..6c3a154 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -93,6 +93,9 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
 
 #endif /* CONFIG_X86_32 */
 
+extern u64 efi_var_store_size;
+extern u64 efi_var_remaining_size;
+extern u64 efi_var_max_var_size;
 extern int add_efi_memmap;
 extern unsigned long x86_efi_facility;
 extern void efi_set_executable(efi_memory_desc_t *md, bool executable);
@@ -102,6 +105,13 @@ extern void efi_call_phys_epilog(void);
 extern void efi_unmap_memmap(void);
 extern void efi_memory_uc(u64 addr, unsigned long size);
 
+struct efi_var_bootdata {
+	struct setup_data data;
+	u64 store_size;
+	u64 remaining_size;
+	u64 max_var_size;
+};
+
 #ifdef CONFIG_EFI
 
 static inline bool efi_is_native(void)
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..0874424 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
 #define SETUP_PCI			3
+#define SETUP_EFI_VARS			4
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 5f2ecaf..cd2f020 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata;
 
 unsigned long x86_efi_facility;
 
+u64 efi_var_store_size;
+u64 efi_var_remaining_size;
+u64 efi_var_max_var_size;
+
 /*
  * Returns 1 if 'facility' is enabled, 0 otherwise.
  */
@@ -682,6 +686,9 @@ void __init efi_init(void)
 	char vendor[100] = "unknown";
 	int i = 0;
 	void *tmp;
+	struct setup_data *data;
+	struct efi_var_bootdata *efi_var_data;
+	u64 pa_data;
 
 #ifdef CONFIG_X86_32
 	if (boot_params.efi_info.efi_systab_hi ||
@@ -699,6 +706,20 @@ void __init efi_init(void)
 	if (efi_systab_init(efi_phys.systab))
 		return;
 
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, sizeof(*efi_var_data));
+		if (data->type == SETUP_EFI_VARS) {
+			efi_var_data = (struct efi_var_bootdata *)data;
+
+			efi_var_store_size = efi_var_data->store_size;
+			efi_var_remaining_size = efi_var_data->remaining_size;
+			efi_var_max_var_size = efi_var_data->max_var_size;
+		}
+		pa_data = data->next;
+		early_iounmap(data, sizeof(*efi_var_data));
+	}
+
 	set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
 
 	/*
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7acafb8..60e7d8f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -85,6 +85,7 @@
 #include <linux/ramfs.h>
 #include <linux/pagemap.h>
 
+#include <asm/efi.h>
 #include <asm/uaccess.h>
 
 #define EFIVARS_VERSION "0.08"
@@ -139,6 +140,8 @@ struct efivar_attribute {
 
 static struct efivars __efivars;
 static struct efivar_operations ops;
+static u64 boot_var_size;
+static u64 boot_used_size;
 
 #define PSTORE_EFI_ATTRIBUTES \
 	(EFI_VARIABLE_NON_VOLATILE | \
@@ -2014,6 +2017,9 @@ int register_efivars(struct efivars *efivars,
 	efi_char16_t *variable_name;
 	unsigned long variable_name_size = 1024;
 	int error = 0;
+	struct efivar_entry *entry;
+	struct efi_variable *var;
+	u64 active_size = 0;
 
 	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
 	if (!variable_name) {
@@ -2086,6 +2092,25 @@ int register_efivars(struct efivars *efivars,
 		}
 	} while (status != EFI_NOT_FOUND);
 
+	if (boot_used_size) {
+		list_for_each_entry(entry, &efivars->list, list) {
+			var = &entry->var;
+			status = get_var_data(efivars, var);
+
+			if (status ||
+			    !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
+				continue;
+
+			active_size += var->DataSize;
+			active_size += utf16_strsize(var->VariableName, 1024);
+		}
+
+		if (active_size < boot_used_size)
+			boot_var_size = boot_used_size - active_size;
+		else
+			printk(KERN_WARNING FW_BUG  "efivars: Inconsistent initial sizes\n");
+	}
+
 	error = create_efivars_bin_attributes(efivars);
 	if (error)
 		unregister_efivars(efivars);
@@ -2133,6 +2158,10 @@ efivars_init(void)
 	ops.get_next_variable = efi.get_next_variable;
 	ops.query_variable_info = efi.query_variable_info;
 
+#ifdef CONFIG_X86
+	boot_used_size = efi_var_store_size - efi_var_remaining_size;
+#endif
+
 	error = register_efivars(&__efivars, &ops, efi_kobj);
 	if (error)
 		goto err_put;
-- 
1.8.1.2

[PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

EFI implementations distinguish between space that is actively used by a
variable and space that merely hasn't been garbage collected yet. Space
that hasn't yet been garbage collected isn't available for use and so isn't
counted in the remaining_space field returned by QueryVariableInfo().

Combined with commit 68d9298 this can cause problems. Some implementations
don't garbage collect until the remaining space is smaller than the maximum
variable size, and as a result check_var_size() will always fail once more
than 50% of the variable store has been used even if most of that space is
marked as available for garbage collection. The user is unable to create
new variables, and deleting variables doesn't increase the remaining space.

The problem that 68d9298 was attempting to avoid was one where certain
platforms fail if the actively used space is greater than 50% of the
available storage space. We should be able to calculate that by simply
summing the size of each available variable and subtracting that from
the total storage space. With luck this will fix the problem described in
https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
damage to occur to the machines 68d9298 was attempting to fix.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 60e7d8f..a6d8a58 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -439,9 +439,19 @@ static efi_status_t
 check_var_size_locked(struct efivars *efivars, u32 attributes,
 			unsigned long size)
 {
-	u64 storage_size, remaining_size, max_size;
+	u64 storage_size, remaining_size, max_size, active_available;
+	struct efivar_entry *entry;
+	struct efi_variable *var;
 	efi_status_t status;
 	const struct efivar_operations *fops = efivars->ops;
+	unsigned long active_size = 0;
+
+	/*
+	 * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit
+	 * RAM, not flash, so ignore them.
+	 */
+	if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+		return EFI_SUCCESS;
 
 	if (!efivars->ops->query_variable_info)
 		return EFI_UNSUPPORTED;
@@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
 	if (status != EFI_SUCCESS)
 		return status;
 
-	if (!storage_size || size > remaining_size || size > max_size ||
-	    (remaining_size - size) < (storage_size / 2))
+	list_for_each_entry(entry, &efivars->list, list) {
+		var = &entry->var;
+		status = get_var_data_locked(efivars, var);
+
+		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
+			continue;
+
+		active_size += var->DataSize;
+		active_size += utf16_strsize(var->VariableName, 1024);
+		/*
+		 * There's some additional metadata associated with each
+		 * variable. Intel's reference implementation is 60 bytes -
+		 * bump that to 128 to account for vendor additions and
+		 * potential alignment constraints
+		 */
+		active_size += 128;
+	}
+
+	/*
+	 * Add on the size of any boot services-only variables
+	 */
+	active_size += boot_var_size;
+
+	active_available = storage_size - active_size;
+
+	if (!storage_size || size > remaining_size ||
+	    (active_available - size) < (storage_size / 2))
 		return EFI_OUT_OF_RESOURCES;
 
 	return status;
-- 
1.8.1.2

[PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

EFI implementations distinguish between space that is actively used by a
variable and space that merely hasn't been garbage collected yet. Space
that hasn't yet been garbage collected isn't available for use and so isn't
counted in the remaining_space field returned by QueryVariableInfo().

Combined with commit 68d9298 this can cause problems. Some implementations
don't garbage collect until the remaining space is smaller than the maximum
variable size, and as a result check_var_size() will always fail once more
than 50% of the variable store has been used even if most of that space is
marked as available for garbage collection. The user is unable to create
new variables, and deleting variables doesn't increase the remaining space.

The problem that 68d9298 was attempting to avoid was one where certain
platforms fail if the actively used space is greater than 50% of the
available storage space. We should be able to calculate that by simply
summing the size of each available variable and subtracting that from
the total storage space. With luck this will fix the problem described in
https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
damage to occur to the machines 68d9298 was attempting to fix.

Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 60e7d8f..a6d8a58 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -439,9 +439,19 @@ static efi_status_t
 check_var_size_locked(struct efivars *efivars, u32 attributes,
 			unsigned long size)
 {
-	u64 storage_size, remaining_size, max_size;
+	u64 storage_size, remaining_size, max_size, active_available;
+	struct efivar_entry *entry;
+	struct efi_variable *var;
 	efi_status_t status;
 	const struct efivar_operations *fops = efivars->ops;
+	unsigned long active_size = 0;
+
+	/*
+	 * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit
+	 * RAM, not flash, so ignore them.
+	 */
+	if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+		return EFI_SUCCESS;
 
 	if (!efivars->ops->query_variable_info)
 		return EFI_UNSUPPORTED;
@@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
 	if (status != EFI_SUCCESS)
 		return status;
 
-	if (!storage_size || size > remaining_size || size > max_size ||
-	    (remaining_size - size) < (storage_size / 2))
+	list_for_each_entry(entry, &efivars->list, list) {
+		var = &entry->var;
+		status = get_var_data_locked(efivars, var);
+
+		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
+			continue;
+
+		active_size += var->DataSize;
+		active_size += utf16_strsize(var->VariableName, 1024);
+		/*
+		 * There's some additional metadata associated with each
+		 * variable. Intel's reference implementation is 60 bytes -
+		 * bump that to 128 to account for vendor additions and
+		 * potential alignment constraints
+		 */
+		active_size += 128;
+	}
+
+	/*
+	 * Add on the size of any boot services-only variables
+	 */
+	active_size += boot_var_size;
+
+	active_available = storage_size - active_size;
+
+	if (!storage_size || size > remaining_size ||
+	    (active_available - size) < (storage_size / 2))
 		return EFI_OUT_OF_RESOURCES;
 
 	return status;
-- 
1.8.1.2

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On Mon, 2013-04-01 at 11:13 -0400, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.

Good thinking.

> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
[...]
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata;
>  
>  unsigned long x86_efi_facility;
>  
> +u64 efi_var_store_size;
> +u64 efi_var_remaining_size;
> +u64 efi_var_max_var_size;
[...]
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -2133,6 +2158,10 @@ efivars_init(void)
>  	ops.get_next_variable = efi.get_next_variable;
>  	ops.query_variable_info = efi.query_variable_info;
>  
> +#ifdef CONFIG_X86
> +	boot_used_size = efi_var_store_size - efi_var_remaining_size;
> +#endif

efivars can be built as a module, but these aren't exported.

Ben.

>  	error = register_efivars(&__efivars, &ops, efi_kobj);
>  	if (error)
>  		goto err_put;

-- 
Ben Hutchings
DNRC Motto:  I can please only one person per day.
Today is not your day.  Tomorrow isn't looking good either.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

On Mon, 2013-04-01 at 11:13 -0400, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.

Good thinking.

> Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> ---
[...]
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata;
>  
>  unsigned long x86_efi_facility;
>  
> +u64 efi_var_store_size;
> +u64 efi_var_remaining_size;
> +u64 efi_var_max_var_size;
[...]
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -2133,6 +2158,10 @@ efivars_init(void)
>  	ops.get_next_variable = efi.get_next_variable;
>  	ops.query_variable_info = efi.query_variable_info;
>  
> +#ifdef CONFIG_X86
> +	boot_used_size = efi_var_store_size - efi_var_remaining_size;
> +#endif

efivars can be built as a module, but these aren't exported.

Ben.

>  	error = register_efivars(&__efivars, &ops, efi_kobj);
>  	if (error)
>  		goto err_put;

-- 
Ben Hutchings
DNRC Motto:  I can please only one person per day.
Today is not your day.  Tomorrow isn't looking good either.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On Mon, 2013-04-01 at 11:14 -0400, Matthew Garrett wrote:
> EFI implementations distinguish between space that is actively used by a
> variable and space that merely hasn't been garbage collected yet. Space
> that hasn't yet been garbage collected isn't available for use and so isn't
> counted in the remaining_space field returned by QueryVariableInfo().
> 
> Combined with commit 68d9298 this can cause problems. Some implementations
> don't garbage collect until the remaining space is smaller than the maximum
> variable size, and as a result check_var_size() will always fail once more
> than 50% of the variable store has been used even if most of that space is
> marked as available for garbage collection. The user is unable to create
> new variables, and deleting variables doesn't increase the remaining space.
> 
> The problem that 68d9298 was attempting to avoid was one where certain
> platforms fail if the actively used space is greater than 50% of the
> available storage space. We should be able to calculate that by simply
> summing the size of each available variable and subtracting that from
> the total storage space. With luck this will fix the problem described in
> https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
> damage to occur to the machines 68d9298 was attempting to fix.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 60e7d8f..a6d8a58 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> -	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		status = get_var_data_locked(efivars, var);
> +
> +		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> +			continue;
> +
> +		active_size += var->DataSize;
> +		active_size += utf16_strsize(var->VariableName, 1024);
> +		/*
> +		 * There's some additional metadata associated with each
> +		 * variable. Intel's reference implementation is 60 bytes -
> +		 * bump that to 128 to account for vendor additions and
> +		 * potential alignment constraints
> +		 */
> +		active_size += 128;
> +	}
> +
> +	/*
> +	 * Add on the size of any boot services-only variables
> +	 */
> +	active_size += boot_var_size;
> +
> +	active_available = storage_size - active_size;
> +
> +	if (!storage_size || size > remaining_size ||
> +	    (active_available - size) < (storage_size / 2))
>  		return EFI_OUT_OF_RESOURCES;

Both substractions here could quite possibly underflow due to
over-estimation of active_size.

We can rearrange the condition to be:

	active_size + size > storage_size / 2

and delete the active_available variable.  But I think metadata for the
new variable should be accounted as well:

	active_size + size + 128 > storage_size / 2

Since we'll now be using that magic number twice, it needs a name.

Ben.

>  	return status;

-- 
Ben Hutchings
DNRC Motto:  I can please only one person per day.
Today is not your day.  Tomorrow isn't looking good either.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Mon, 2013-04-01 at 11:14 -0400, Matthew Garrett wrote:
> EFI implementations distinguish between space that is actively used by a
> variable and space that merely hasn't been garbage collected yet. Space
> that hasn't yet been garbage collected isn't available for use and so isn't
> counted in the remaining_space field returned by QueryVariableInfo().
> 
> Combined with commit 68d9298 this can cause problems. Some implementations
> don't garbage collect until the remaining space is smaller than the maximum
> variable size, and as a result check_var_size() will always fail once more
> than 50% of the variable store has been used even if most of that space is
> marked as available for garbage collection. The user is unable to create
> new variables, and deleting variables doesn't increase the remaining space.
> 
> The problem that 68d9298 was attempting to avoid was one where certain
> platforms fail if the actively used space is greater than 50% of the
> available storage space. We should be able to calculate that by simply
> summing the size of each available variable and subtracting that from
> the total storage space. With luck this will fix the problem described in
> https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
> damage to occur to the machines 68d9298 was attempting to fix.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 60e7d8f..a6d8a58 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> -	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		status = get_var_data_locked(efivars, var);
> +
> +		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> +			continue;
> +
> +		active_size += var->DataSize;
> +		active_size += utf16_strsize(var->VariableName, 1024);
> +		/*
> +		 * There's some additional metadata associated with each
> +		 * variable. Intel's reference implementation is 60 bytes -
> +		 * bump that to 128 to account for vendor additions and
> +		 * potential alignment constraints
> +		 */
> +		active_size += 128;
> +	}
> +
> +	/*
> +	 * Add on the size of any boot services-only variables
> +	 */
> +	active_size += boot_var_size;
> +
> +	active_available = storage_size - active_size;
> +
> +	if (!storage_size || size > remaining_size ||
> +	    (active_available - size) < (storage_size / 2))
>  		return EFI_OUT_OF_RESOURCES;

Both substractions here could quite possibly underflow due to
over-estimation of active_size.

We can rearrange the condition to be:

	active_size + size > storage_size / 2

and delete the active_available variable.  But I think metadata for the
new variable should be accounted as well:

	active_size + size + 128 > storage_size / 2

Since we'll now be using that magic number twice, it needs a name.

Ben.

>  	return status;

-- 
Ben Hutchings
DNRC Motto:  I can please only one person per day.
Today is not your day.  Tomorrow isn't looking good either.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Matt Fleming]

On 01/04/13 16:13, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  arch/x86/boot/compressed/eboot.c      | 47 +++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/efi.h            | 10 ++++++++
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  arch/x86/platform/efi/efi.c           | 21 ++++++++++++++++
>  drivers/firmware/efivars.c            | 29 +++++++++++++++++++++
>  5 files changed, 108 insertions(+)

We're fixing a regression in efivars.c, but only for those users that
boot via the EFI boot stub? That seems likely to upset some people.

Introducing new features via the EFI boot stub is fine, and working
around firmware bugs so that we can use some feature is also cool, but
we can't start fixing regressions from other subsystems in the EFI boot
stub.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Matt Fleming]

On 01/04/13 16:13, Matthew Garrett wrote:
> EFI variables can be flagged as being accessible only within boot services.
> This makes it awkward for us to figure out how much space they use at
> runtime. In theory we could figure this out by simply comparing the results
> from QueryVariableInfo() to the space used by all of our variables, but
> that fails if the platform doesn't garbage collect on every boot. Thankfully,
> calling QueryVariableInfo() while still inside boot services gives a more
> reliable answer. This patch passes that information from the EFI boot stub
> up to the efivars code, letting us calculate a reasonably accurate value.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/boot/compressed/eboot.c      | 47 +++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/efi.h            | 10 ++++++++
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  arch/x86/platform/efi/efi.c           | 21 ++++++++++++++++
>  drivers/firmware/efivars.c            | 29 +++++++++++++++++++++
>  5 files changed, 108 insertions(+)

We're fixing a regression in efivars.c, but only for those users that
boot via the EFI boot stub? That seems likely to upset some people.

Introducing new features via the EFI boot stub is fine, and working
around firmware bugs so that we can use some feature is also cool, but
we can't start fixing regressions from other subsystems in the EFI boot
stub.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matt Fleming]

On 01/04/13 16:14, Matthew Garrett wrote:
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> -	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		status = get_var_data_locked(efivars, var);
> +
> +		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> +			continue;
> +
> +		active_size += var->DataSize;
> +		active_size += utf16_strsize(var->VariableName, 1024);
> +		/*
> +		 * There's some additional metadata associated with each
> +		 * variable. Intel's reference implementation is 60 bytes -
> +		 * bump that to 128 to account for vendor additions and
> +		 * potential alignment constraints
> +		 */
> +		active_size += 128;
> +	}

This is the kind of thing I was referring to when I said, 

  Hmm... I'm not convinced this will provide a long-term solution. Is there
  anything that mandates that the size of all variables has to correlate
  sensibly with the results from QueryVariableInfo()? Even if there is in
  theory, I doubt it'll be true everywhere in practice, and trying to
  workaround implementation bugs in our workarounds for other bugs is the
  path to madness.

  We can't continue this approach where we attempt to guess how the firmware
  implements variable storage, because as we've seen, there are various
  schemes out there. 

This looks like something that will differ between implementations, and the
fact that it's appearing in our code is a sure sign that this isn't the way to
go.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matt Fleming]

On 01/04/13 16:14, Matthew Garrett wrote:
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> -	if (!storage_size || size > remaining_size || size > max_size ||
> -	    (remaining_size - size) < (storage_size / 2))
> +	list_for_each_entry(entry, &efivars->list, list) {
> +		var = &entry->var;
> +		status = get_var_data_locked(efivars, var);
> +
> +		if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> +			continue;
> +
> +		active_size += var->DataSize;
> +		active_size += utf16_strsize(var->VariableName, 1024);
> +		/*
> +		 * There's some additional metadata associated with each
> +		 * variable. Intel's reference implementation is 60 bytes -
> +		 * bump that to 128 to account for vendor additions and
> +		 * potential alignment constraints
> +		 */
> +		active_size += 128;
> +	}

This is the kind of thing I was referring to when I said, 

  Hmm... I'm not convinced this will provide a long-term solution. Is there
  anything that mandates that the size of all variables has to correlate
  sensibly with the results from QueryVariableInfo()? Even if there is in
  theory, I doubt it'll be true everywhere in practice, and trying to
  workaround implementation bugs in our workarounds for other bugs is the
  path to madness.

  We can't continue this approach where we attempt to guess how the firmware
  implements variable storage, because as we've seen, there are various
  schemes out there. 

This looks like something that will differ between implementations, and the
fact that it's appearing in our code is a sure sign that this isn't the way to
go.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 527 bytes --]

On Wed, 2013-04-03 at 14:09 +0100, Matt Fleming wrote:

> We're fixing a regression in efivars.c, but only for those users that
> boot via the EFI boot stub? That seems likely to upset some people.

Not really - it just makes the estimates more accurate. Applying (2)
without (1) should still fix the functional regression.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables

[Matthew Garrett]

On Wed, 2013-04-03 at 14:09 +0100, Matt Fleming wrote:

> We're fixing a regression in efivars.c, but only for those users that
> boot via the EFI boot stub? That seems likely to upset some people.

Not really - it just makes the estimates more accurate. Applying (2)
without (1) should still fix the functional regression.

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

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 890 bytes --]

On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:

> This looks like something that will differ between implementations, and the
> fact that it's appearing in our code is a sure sign that this isn't the way to
> go.

Our choices right now are:

1) Break machines that don't garbage collect on every reboot
2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
3) Maintain a whitelist or blacklist that will inevitably be inadequate,
either breaking otherwise working machines or risking bricking of broken
ones
4) Attempt to implement something that'll work in all cases

Dealing with firmware is hard. This fixes (1) without leaving us with
(2), which seems like a net win.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:

> This looks like something that will differ between implementations, and the
> fact that it's appearing in our code is a sure sign that this isn't the way to
> go.

Our choices right now are:

1) Break machines that don't garbage collect on every reboot
2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
3) Maintain a whitelist or blacklist that will inevitably be inadequate,
either breaking otherwise working machines or risking bricking of broken
ones
4) Attempt to implement something that'll work in all cases

Dealing with firmware is hard. This fixes (1) without leaving us with
(2), which seems like a net win.

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

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matt Fleming]

On 03/04/13 14:48, Matthew Garrett wrote:
> On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:
> 
>> This looks like something that will differ between implementations, and the
>> fact that it's appearing in our code is a sure sign that this isn't the way to
>> go.
> 
> Our choices right now are:
> 
> 1) Break machines that don't garbage collect on every reboot
> 2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
> 3) Maintain a whitelist or blacklist that will inevitably be inadequate,
> either breaking otherwise working machines or risking bricking of broken
> ones
> 4) Attempt to implement something that'll work in all cases

The solution you're proposing has the same downsides as 3) - we risk
having to tweak things either way. The difference is that in the case of
3) the tweaking is adding entries to the whitelist, whereas tweaking
your solution has more chance of introducing further unwanted
regressions because you'd be tweaking an algorithm, an algorithm that
relies on the internal implementation of the variable storage code.

> Dealing with firmware is hard. This fixes (1) without leaving us with
> (2), which seems like a net win.

I'm not convinced that implementing 3) would inevitably lead to 2),
provided that we apply a bit of common sense when adding entries. I'm
not advocating some kind of flag day where we add umpteen machines to
the whitelist.

For reference, I just pushed two patches to the 'whitelist' branch at,

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git

which should hopefully illustrate the kind of thing that I'm talking about.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matt Fleming]

On 03/04/13 14:48, Matthew Garrett wrote:
> On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote:
> 
>> This looks like something that will differ between implementations, and the
>> fact that it's appearing in our code is a sure sign that this isn't the way to
>> go.
> 
> Our choices right now are:
> 
> 1) Break machines that don't garbage collect on every reboot
> 2) Leave Samsungs (and some Lenovos?) vulnerable to bricking
> 3) Maintain a whitelist or blacklist that will inevitably be inadequate,
> either breaking otherwise working machines or risking bricking of broken
> ones
> 4) Attempt to implement something that'll work in all cases

The solution you're proposing has the same downsides as 3) - we risk
having to tweak things either way. The difference is that in the case of
3) the tweaking is adding entries to the whitelist, whereas tweaking
your solution has more chance of introducing further unwanted
regressions because you'd be tweaking an algorithm, an algorithm that
relies on the internal implementation of the variable storage code.

> Dealing with firmware is hard. This fixes (1) without leaving us with
> (2), which seems like a net win.

I'm not convinced that implementing 3) would inevitably lead to 2),
provided that we apply a bit of common sense when adding entries. I'm
not advocating some kind of flag day where we add umpteen machines to
the whitelist.

For reference, I just pushed two patches to the 'whitelist' branch at,

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git

which should hopefully illustrate the kind of thing that I'm talking about.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1495 bytes --]

On Wed, 2013-04-03 at 18:12 +0100, Matt Fleming wrote:

> The solution you're proposing has the same downsides as 3) - we risk
> having to tweak things either way. The difference is that in the case of
> 3) the tweaking is adding entries to the whitelist, whereas tweaking
> your solution has more chance of introducing further unwanted
> regressions because you'd be tweaking an algorithm, an algorithm that
> relies on the internal implementation of the variable storage code.

We *risk* having to tweak things, and we fail on the side of safety. 

> > Dealing with firmware is hard. This fixes (1) without leaving us with
> > (2), which seems like a net win.
> 
> I'm not convinced that implementing 3) would inevitably lead to 2),
> provided that we apply a bit of common sense when adding entries. I'm
> not advocating some kind of flag day where we add umpteen machines to
> the whitelist.
> 
> For reference, I just pushed two patches to the 'whitelist' branch at,
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> 
> which should hopefully illustrate the kind of thing that I'm talking about.

I don't think that works. People are complaining that we broke some
Thinkpads as well, but we also have reports that Thinkpads can be
bricked if we use too much space.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space

[Matthew Garrett]

On Wed, 2013-04-03 at 18:12 +0100, Matt Fleming wrote:

> The solution you're proposing has the same downsides as 3) - we risk
> having to tweak things either way. The difference is that in the case of
> 3) the tweaking is adding entries to the whitelist, whereas tweaking
> your solution has more chance of introducing further unwanted
> regressions because you'd be tweaking an algorithm, an algorithm that
> relies on the internal implementation of the variable storage code.

We *risk* having to tweak things, and we fail on the side of safety. 

> > Dealing with firmware is hard. This fixes (1) without leaving us with
> > (2), which seems like a net win.
> 
> I'm not convinced that implementing 3) would inevitably lead to 2),
> provided that we apply a bit of common sense when adding entries. I'm
> not advocating some kind of flag day where we add umpteen machines to
> the whitelist.
> 
> For reference, I just pushed two patches to the 'whitelist' branch at,
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> 
> which should hopefully illustrate the kind of thing that I'm talking about.

I don't think that works. People are complaining that we broke some
Thinkpads as well, but we also have reports that Thinkpads can be
bricked if we use too much space.

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