Hi Linus,
The following changes since commit 0fbd06761f5c17cc9b20e02af60fd7ee9c895996:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc (2013-04-24 17:10:18 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-urgent-for-linus
This is exclusively two EFI fixes:
* The EFI variable anti-bricking algorithm merged in -rc8 broke
booting on some Apple machines because they implement EFI spec
1.10, which doesn't provide a QueryVariableInfo() runtime function
and the logic used to check for the existence of that function was
insufficient. Fix from Josh Boyer.
* The anti-bricking algorithm also introduced a compiler warning on
32-bit. Fix from Borislav Petkov.
----------------------------------------------------------------
Borislav Petkov (1):
x86, efi: Fix a build warning
H. Peter Anvin (1):
Merge tag 'efi-urgent' into x86/urgent
Josh Boyer (1):
efi: Check EFI revision in setup_efi_vars
arch/x86/boot/compressed/eboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 8615f75..35ee62f 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -258,7 +258,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
u64 store_size, remaining_size, var_size;
efi_status_t status;
- if (!sys_table->runtime->query_variable_info)
+ 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;
@@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
while (data && data->next)
data = (struct setup_data *)(unsigned long)data->next;
- status = efi_call_phys4(sys_table->runtime->query_variable_info,
+ 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,
On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
>
> - if (!sys_table->runtime->query_variable_info)
> + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
> return EFI_UNSUPPORTED;
Is a EFI 2.00 system table *guaranteed* to have that
"query_variable_info" function? The above adds the version check, but
removes the check for a NULL pointer.
And why the hell don't we have a real structure that has been filled
out properly, and instead apparently just do this "point to random
memory that doesn't necessarily have the full structure?
Linus
On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
> >
> > - if (!sys_table->runtime->query_variable_info)
> > + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
> > return EFI_UNSUPPORTED;
>
> Is a EFI 2.00 system table *guaranteed* to have that
> "query_variable_info" function? The above adds the version check, but
> removes the check for a NULL pointer.
As far as the spec's concerned, yes. As far as reality's concerned - if
anything doesn't provide it, we're already crashing when
efi_virt_query_variable_info() gets called. Nobody's complained so far.
> And why the hell don't we have a real structure that has been filled
> out properly, and instead apparently just do this "point to random
> memory that doesn't necessarily have the full structure?
This is early boot code, we're not in the kernel proper yet. All we have
is the structure that's handed to us by the firmware, and the size of
that structure varies depending on its version.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
<[email protected]> wrote:
> On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
>> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
>> >
>> > - if (!sys_table->runtime->query_variable_info)
>> > + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
>> > return EFI_UNSUPPORTED;
>>
>> Is a EFI 2.00 system table *guaranteed* to have that
>> "query_variable_info" function? The above adds the version check, but
>> removes the check for a NULL pointer.
>
> As far as the spec's concerned, yes. As far as reality's concerned - if
> anything doesn't provide it, we're already crashing when
> efi_virt_query_variable_info() gets called. Nobody's complained so far.
Well, I don't know if this is related, but commit e971318bbed6 broke
the google EFI SMI driver with
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
Call Trace:
[<ffffffff814a9936>] register_efivars+0x106/0x370
[<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
[<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
[<ffffffff8100020f>] do_one_initcall+0x3f/0x170
...
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
> <[email protected]> wrote:
>> On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
>>> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
>>>>
>>>> - if (!sys_table->runtime->query_variable_info)
>>>> + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
>>>> return EFI_UNSUPPORTED;
>>>
>>> Is a EFI 2.00 system table *guaranteed* to have that
>>> "query_variable_info" function? The above adds the version check, but
>>> removes the check for a NULL pointer.
>>
>> As far as the spec's concerned, yes. As far as reality's concerned - if
>> anything doesn't provide it, we're already crashing when
>> efi_virt_query_variable_info() gets called. Nobody's complained so far.
>
> Well, I don't know if this is related, but commit e971318bbed6 broke
> the google EFI SMI driver with
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
> Call Trace:
> [<ffffffff814a9936>] register_efivars+0x106/0x370
> [<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
> [<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
> [<ffffffff8100020f>] do_one_initcall+0x3f/0x170
> ...
>
I don't know either. Could you test this patch and see if it does anything?
-hpa
On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <[email protected]> wrote:
> On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
>> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
>> <[email protected]> wrote:
>>> On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
>>>> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
>>>>>
>>>>> - if (!sys_table->runtime->query_variable_info)
>>>>> + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
>>>>> return EFI_UNSUPPORTED;
>>>>
>>>> Is a EFI 2.00 system table *guaranteed* to have that
>>>> "query_variable_info" function? The above adds the version check, but
>>>> removes the check for a NULL pointer.
>>>
>>> As far as the spec's concerned, yes. As far as reality's concerned - if
>>> anything doesn't provide it, we're already crashing when
>>> efi_virt_query_variable_info() gets called. Nobody's complained so far.
>>
>> Well, I don't know if this is related, but commit e971318bbed6 broke
>> the google EFI SMI driver with
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
>> Call Trace:
>> [<ffffffff814a9936>] register_efivars+0x106/0x370
>> [<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
>> [<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
>> [<ffffffff8100020f>] do_one_initcall+0x3f/0x170
>> ...
>
> I don't know either. Could you test this patch and see if it does anything?
Nope, still seeing the crash with this patch applied.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On 26/04/13 00:11, Michel Lespinasse wrote:
> On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <[email protected]> wrote:
>> On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
>>> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
>>> <[email protected]> wrote:
>>>> On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
>>>>> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
>>>>>>
>>>>>> - if (!sys_table->runtime->query_variable_info)
>>>>>> + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
>>>>>> return EFI_UNSUPPORTED;
>>>>>
>>>>> Is a EFI 2.00 system table *guaranteed* to have that
>>>>> "query_variable_info" function? The above adds the version check, but
>>>>> removes the check for a NULL pointer.
>>>>
>>>> As far as the spec's concerned, yes. As far as reality's concerned - if
>>>> anything doesn't provide it, we're already crashing when
>>>> efi_virt_query_variable_info() gets called. Nobody's complained so far.
>>>
>>> Well, I don't know if this is related, but commit e971318bbed6 broke
>>> the google EFI SMI driver with
>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>> IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
>>> Call Trace:
>>> [<ffffffff814a9936>] register_efivars+0x106/0x370
>>> [<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
>>> [<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
>>> [<ffffffff8100020f>] do_one_initcall+0x3f/0x170
>>> ...
>>
>> I don't know either. Could you test this patch and see if it does anything?
>
> Nope, still seeing the crash with this patch applied.
Could you try the following patch against Linus' tree? The bug you've
found and the changes in the pull request are unrelated.
---
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 182ce94..2a4f619 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1635,6 +1635,9 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
unsigned long strsize1, strsize2;
bool found = false;
+ if (list_empty(&efivars->list))
+ return false;
+
strsize1 = ucs2_strsize(variable_name, 1024);
list_for_each_entry_safe(entry, n, &efivars->list, list) {
strsize2 = ucs2_strsize(entry->var.VariableName, 1024);
* Matthew Garrett <[email protected]> wrote:
> On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
> > On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <[email protected]> wrote:
> > >
> > > - if (!sys_table->runtime->query_variable_info)
> > > + if (sys_table->runtime->hdr.revision < EFI_2_00_SYSTEM_TABLE_REVISION)
> > > return EFI_UNSUPPORTED;
> >
> > Is a EFI 2.00 system table *guaranteed* to have that
> > "query_variable_info" function? The above adds the version check, but
> > removes the check for a NULL pointer.
>
> As far as the spec's concerned, yes. As far as reality's concerned - if
> anything doesn't provide it, we're already crashing when
> efi_virt_query_variable_info() gets called. Nobody's complained so far.
I'm worried about the fragility of this code - this is firmware code ...
I think firmware code should be fundamentally paranoid and robust, and in
this case treat all EFI-provided data as hostile and do a much sanity
checking of it as possible - and provide an actionable error message if
the checks fail, not just 'crash'.
Even if no-one complained, yet.
Thanks,
Ingo
On Fri, Apr 26, 2013 at 12:12 AM, Matt Fleming <[email protected]> wrote:
> On 26/04/13 00:11, Michel Lespinasse wrote:
>> On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <[email protected]> wrote:
>>> On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
>>>> Well, I don't know if this is related, but commit e971318bbed6 broke
>>>> the google EFI SMI driver with
>>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>>> IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
>>>> Call Trace:
>>>> [<ffffffff814a9936>] register_efivars+0x106/0x370
>>>> [<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
>>>> [<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
>>>> [<ffffffff8100020f>] do_one_initcall+0x3f/0x170
>>>> ...
>>>
>>> I don't know either. Could you test this patch and see if it does anything?
>>
>> Nope, still seeing the crash with this patch applied.
>
> Could you try the following patch against Linus' tree? The bug you've
> found and the changes in the pull request are unrelated.
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 182ce94..2a4f619 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1635,6 +1635,9 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
> unsigned long strsize1, strsize2;
> bool found = false;
>
> + if (list_empty(&efivars->list))
> + return false;
> +
> strsize1 = ucs2_strsize(variable_name, 1024);
> list_for_each_entry_safe(entry, n, &efivars->list, list) {
> strsize2 = ucs2_strsize(entry->var.VariableName, 1024);
Still seeing the crash.
I went and compared the crash dump with the vmlinux disassembly; the
issue is a NULL pointer dereference in list_for_each_entry_safe().
list_empty() checks that the head node points to itself, but here the
head node has NULL. I think this may be due to gsmi_init() being
called before efivars_init(). Not sure what's the proper fix though.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On 26/04/13 08:43, Michel Lespinasse wrote:
> Still seeing the crash.
>
> I went and compared the crash dump with the vmlinux disassembly; the
> issue is a NULL pointer dereference in list_for_each_entry_safe().
> list_empty() checks that the head node points to itself, but here the
> head node has NULL. I think this may be due to gsmi_init() being
> called before efivars_init(). Not sure what's the proper fix though.
Ohh... I see what you mean. The bug is in variable_is_present() because
it accesses __efivars directly, which a) isn't the struct efivars gsmi.c
uses and b) hasn't been initialised. Something like this might work.
---
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 182ce94..f4baa11 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1628,10 +1628,11 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
return count;
}
-static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
+static bool variable_is_present(struct efivars *efivars,
+ efi_char16_t *variable_name,
+ efi_guid_t *vendor)
{
struct efivar_entry *entry, *n;
- struct efivars *efivars = &__efivars;
unsigned long strsize1, strsize2;
bool found = false;
@@ -1703,8 +1704,8 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
if (status != EFI_SUCCESS) {
break;
} else {
- if (!variable_is_present(variable_name,
- &vendor)) {
+ if (!variable_is_present(efivars,
+ variable_name, &vendor)) {
found = true;
break;
}
@@ -2008,7 +2009,8 @@ int register_efivars(struct efivars *efivars,
* we'll ever see a different variable name,
* and may end up looping here forever.
*/
- if (variable_is_present(variable_name, &vendor_guid)) {
+ if (variable_is_present(efivars, variable_name,
+ &vendor_guid)) {
dup_variable_bug(variable_name, &vendor_guid,
variable_name_size);
status = EFI_NOT_FOUND;
On 26/04/13 08:29, Ingo Molnar wrote:
> I'm worried about the fragility of this code - this is firmware code ...
>
> I think firmware code should be fundamentally paranoid and robust, and in
> this case treat all EFI-provided data as hostile and do a much sanity
> checking of it as possible - and provide an actionable error message if
> the checks fail, not just 'crash'.
>
> Even if no-one complained, yet.
I'm not sure how much more robust checking for NULL makes it, it's not
going to save us from garbage pointers, etc. Instead of the pointer
being NULL a more likely bug is that query_variable_info() exists (for
those firmware that implement the relevant spec version), but doesn't
work correctly. That's the kind of bug we've been seeing in other
functions.
To be fair to Josh, his original submission did include the NULL check,
but I asked him to remove it. If someone really wants to re-add the
check for a NULL pointer, I'm not super opposed to it.
On Fri, Apr 26, 2013 at 1:49 AM, Matt Fleming <[email protected]> wrote:
> On 26/04/13 08:43, Michel Lespinasse wrote:
>> Still seeing the crash.
>>
>> I went and compared the crash dump with the vmlinux disassembly; the
>> issue is a NULL pointer dereference in list_for_each_entry_safe().
>> list_empty() checks that the head node points to itself, but here the
>> head node has NULL. I think this may be due to gsmi_init() being
>> called before efivars_init(). Not sure what's the proper fix though.
>
> Ohh... I see what you mean. The bug is in variable_is_present() because
> it accesses __efivars directly, which a) isn't the struct efivars gsmi.c
> uses and b) hasn't been initialised. Something like this might work.
[... skipping patch ...]
Yes, this one fixes it. Thanks !
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On 26/04/13 10:02, Michel Lespinasse wrote:
> On Fri, Apr 26, 2013 at 1:49 AM, Matt Fleming <[email protected]> wrote:
>> On 26/04/13 08:43, Michel Lespinasse wrote:
>>> Still seeing the crash.
>>>
>>> I went and compared the crash dump with the vmlinux disassembly; the
>>> issue is a NULL pointer dereference in list_for_each_entry_safe().
>>> list_empty() checks that the head node points to itself, but here the
>>> head node has NULL. I think this may be due to gsmi_init() being
>>> called before efivars_init(). Not sure what's the proper fix though.
>>
>> Ohh... I see what you mean. The bug is in variable_is_present() because
>> it accesses __efivars directly, which a) isn't the struct efivars gsmi.c
>> uses and b) hasn't been initialised. Something like this might work.
>
> [... skipping patch ...]
>
> Yes, this one fixes it. Thanks !
Thanks for testing!
Linus, did you want to take this one directly?
---
>From f576b789b29dab31ddc1c7d37af63f10fb203fb7 Mon Sep 17 00:00:00 2001
From: Matt Fleming <[email protected]>
Date: Fri, 26 Apr 2013 10:10:55 +0100
Subject: [PATCH] efivars: only check for duplicates on the registered list
variable_is_present() accesses '__efivars' directly, but when called
via gsmi_init() Michel reports observing the following crash,
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff814a7245>] variable_is_present+0x55/0x170
Call Trace:
[<ffffffff814a9936>] register_efivars+0x106/0x370
[<ffffffff818ff430>] ? firmware_map_add_early+0xb1/0xb1
[<ffffffff818ff6dd>] gsmi_init+0x2ad/0x3da
[<ffffffff8100020f>] do_one_initcall+0x3f/0x170
The reason for the crash is that '__efivars' hasn't been initialised nor
has it been registered with register_efivars() by the time the google
EFI SMI driver runs. The gsmi code uses its own struct efivars, and
therefore, a different variable list. Fix the above crash by passing the
registered struct efivars to variable_is_present(), so that we traverse
the correct list.
Reported-by: Michel Lespinasse <[email protected]>
Tested-by: Michel Lespinasse <[email protected]>
Cc: Mike Waychison <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Seiji Aguchi <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
drivers/firmware/efivars.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 182ce94..f4baa11 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1628,10 +1628,11 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
return count;
}
-static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
+static bool variable_is_present(struct efivars *efivars,
+ efi_char16_t *variable_name,
+ efi_guid_t *vendor)
{
struct efivar_entry *entry, *n;
- struct efivars *efivars = &__efivars;
unsigned long strsize1, strsize2;
bool found = false;
@@ -1703,8 +1704,8 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
if (status != EFI_SUCCESS) {
break;
} else {
- if (!variable_is_present(variable_name,
- &vendor)) {
+ if (!variable_is_present(efivars,
+ variable_name, &vendor)) {
found = true;
break;
}
@@ -2008,7 +2009,8 @@ int register_efivars(struct efivars *efivars,
* we'll ever see a different variable name,
* and may end up looping here forever.
*/
- if (variable_is_present(variable_name, &vendor_guid)) {
+ if (variable_is_present(efivars, variable_name,
+ &vendor_guid)) {
dup_variable_bug(variable_name, &vendor_guid,
variable_name_size);
status = EFI_NOT_FOUND;
--
1.7.10.4