* [GIT PULL] x86 fixes for 3.9
@ 2013-04-25 21:44 H. Peter Anvin
2013-04-25 22:20 ` Linus Torvalds
0 siblings, 1 reply; 13+ messages in thread
From: H. Peter Anvin @ 2013-04-25 21:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Borislav Petkov, H. Peter Anvin, H. Peter Anvin, Ingo Molnar,
Josh Boyer, Linux Kernel Mailing List, Matt Fleming,
Matthew Garrett, Paul Bolle, Thomas Gleixner
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,
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 21:44 [GIT PULL] x86 fixes for 3.9 H. Peter Anvin
@ 2013-04-25 22:20 ` Linus Torvalds
2013-04-25 22:23 ` Matthew Garrett
0 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2013-04-25 22:20 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Borislav Petkov, H. Peter Anvin, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Matt Fleming, Matthew Garrett,
Paul Bolle, Thomas Gleixner
On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <hpa@linux.intel.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 22:20 ` Linus Torvalds
@ 2013-04-25 22:23 ` Matthew Garrett
2013-04-25 22:53 ` Michel Lespinasse
2013-04-26 7:29 ` Ingo Molnar
0 siblings, 2 replies; 13+ messages in thread
From: Matthew Garrett @ 2013-04-25 22:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: H. Peter Anvin, Borislav Petkov, H. Peter Anvin, Ingo Molnar,
Josh Boyer, Linux Kernel Mailing List, Matt Fleming, Paul Bolle,
Thomas Gleixner
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1291 bytes --]
On Thu, 2013-04-25 at 15:20 -0700, Linus Torvalds wrote:
> On Thu, Apr 25, 2013 at 2:44 PM, H. Peter Anvin <hpa@linux.intel.com> 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 | 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¥
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 22:23 ` Matthew Garrett
@ 2013-04-25 22:53 ` Michel Lespinasse
2013-04-25 22:54 ` H. Peter Anvin
2013-04-26 7:29 ` Ingo Molnar
1 sibling, 1 reply; 13+ messages in thread
From: Michel Lespinasse @ 2013-04-25 22:53 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, H. Peter Anvin, Borislav Petkov, H. Peter Anvin,
Ingo Molnar, Josh Boyer, Linux Kernel Mailing List, Matt Fleming,
Paul Bolle, Thomas Gleixner
On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
<matthew.garrett@nebula.com> 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 <hpa@linux.intel.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 22:53 ` Michel Lespinasse
@ 2013-04-25 22:54 ` H. Peter Anvin
2013-04-25 23:11 ` Michel Lespinasse
0 siblings, 1 reply; 13+ messages in thread
From: H. Peter Anvin @ 2013-04-25 22:54 UTC (permalink / raw)
To: Michel Lespinasse
Cc: Matthew Garrett, Linus Torvalds, H. Peter Anvin, Borislav Petkov,
Ingo Molnar, Josh Boyer, Linux Kernel Mailing List, Matt Fleming,
Paul Bolle, Thomas Gleixner
On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
> <matthew.garrett@nebula.com> 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 <hpa@linux.intel.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 22:54 ` H. Peter Anvin
@ 2013-04-25 23:11 ` Michel Lespinasse
2013-04-26 7:12 ` Matt Fleming
0 siblings, 1 reply; 13+ messages in thread
From: Michel Lespinasse @ 2013-04-25 23:11 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Matthew Garrett, Linus Torvalds, H. Peter Anvin, Borislav Petkov,
Ingo Molnar, Josh Boyer, Linux Kernel Mailing List, Matt Fleming,
Paul Bolle, Thomas Gleixner
On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
>> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
>> <matthew.garrett@nebula.com> 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 <hpa@linux.intel.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 23:11 ` Michel Lespinasse
@ 2013-04-26 7:12 ` Matt Fleming
2013-04-26 7:43 ` Michel Lespinasse
0 siblings, 1 reply; 13+ messages in thread
From: Matt Fleming @ 2013-04-26 7:12 UTC (permalink / raw)
To: Michel Lespinasse
Cc: H. Peter Anvin, Matthew Garrett, Linus Torvalds, H. Peter Anvin,
Borislav Petkov, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner
On 26/04/13 00:11, Michel Lespinasse wrote:
> On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 04/25/2013 03:53 PM, Michel Lespinasse wrote:
>>> On Thu, Apr 25, 2013 at 3:23 PM, Matthew Garrett
>>> <matthew.garrett@nebula.com> 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 <hpa@linux.intel.com> 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);
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-25 22:23 ` Matthew Garrett
2013-04-25 22:53 ` Michel Lespinasse
@ 2013-04-26 7:29 ` Ingo Molnar
2013-04-26 9:01 ` Matt Fleming
1 sibling, 1 reply; 13+ messages in thread
From: Ingo Molnar @ 2013-04-26 7:29 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, H. Peter Anvin, Borislav Petkov, H. Peter Anvin,
Ingo Molnar, Josh Boyer, Linux Kernel Mailing List, Matt Fleming,
Paul Bolle, Thomas Gleixner
* Matthew Garrett <matthew.garrett@nebula.com> 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 <hpa@linux.intel.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-26 7:12 ` Matt Fleming
@ 2013-04-26 7:43 ` Michel Lespinasse
2013-04-26 8:49 ` Matt Fleming
0 siblings, 1 reply; 13+ messages in thread
From: Michel Lespinasse @ 2013-04-26 7:43 UTC (permalink / raw)
To: Matt Fleming
Cc: H. Peter Anvin, Matthew Garrett, Linus Torvalds, H. Peter Anvin,
Borislav Petkov, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner
On Fri, Apr 26, 2013 at 12:12 AM, Matt Fleming <matt.fleming@intel.com> wrote:
> On 26/04/13 00:11, Michel Lespinasse wrote:
>> On Thu, Apr 25, 2013 at 3:54 PM, H. Peter Anvin <hpa@zytor.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-26 7:43 ` Michel Lespinasse
@ 2013-04-26 8:49 ` Matt Fleming
2013-04-26 9:02 ` Michel Lespinasse
0 siblings, 1 reply; 13+ messages in thread
From: Matt Fleming @ 2013-04-26 8:49 UTC (permalink / raw)
To: Michel Lespinasse
Cc: H. Peter Anvin, Matthew Garrett, Linus Torvalds, H. Peter Anvin,
Borislav Petkov, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner
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;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-26 7:29 ` Ingo Molnar
@ 2013-04-26 9:01 ` Matt Fleming
0 siblings, 0 replies; 13+ messages in thread
From: Matt Fleming @ 2013-04-26 9:01 UTC (permalink / raw)
To: Ingo Molnar
Cc: Matthew Garrett, Linus Torvalds, H. Peter Anvin, Borislav Petkov,
H. Peter Anvin, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-26 8:49 ` Matt Fleming
@ 2013-04-26 9:02 ` Michel Lespinasse
2013-04-26 9:44 ` Matt Fleming
0 siblings, 1 reply; 13+ messages in thread
From: Michel Lespinasse @ 2013-04-26 9:02 UTC (permalink / raw)
To: Matt Fleming
Cc: H. Peter Anvin, Matthew Garrett, Linus Torvalds, H. Peter Anvin,
Borislav Petkov, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner
On Fri, Apr 26, 2013 at 1:49 AM, Matt Fleming <matt.fleming@intel.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [GIT PULL] x86 fixes for 3.9
2013-04-26 9:02 ` Michel Lespinasse
@ 2013-04-26 9:44 ` Matt Fleming
0 siblings, 0 replies; 13+ messages in thread
From: Matt Fleming @ 2013-04-26 9:44 UTC (permalink / raw)
To: Michel Lespinasse
Cc: H. Peter Anvin, Matthew Garrett, Linus Torvalds, H. Peter Anvin,
Borislav Petkov, Ingo Molnar, Josh Boyer,
Linux Kernel Mailing List, Paul Bolle, Thomas Gleixner,
Seiji Aguchi, Mike Waychison
On 26/04/13 10:02, Michel Lespinasse wrote:
> On Fri, Apr 26, 2013 at 1:49 AM, Matt Fleming <matt.fleming@intel.com> 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 <matt.fleming@intel.com>
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 <walken@google.com>
Tested-by: Michel Lespinasse <walken@google.com>
Cc: Mike Waychison <mikew@google.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-04-26 9:44 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-25 21:44 [GIT PULL] x86 fixes for 3.9 H. Peter Anvin
2013-04-25 22:20 ` Linus Torvalds
2013-04-25 22:23 ` Matthew Garrett
2013-04-25 22:53 ` Michel Lespinasse
2013-04-25 22:54 ` H. Peter Anvin
2013-04-25 23:11 ` Michel Lespinasse
2013-04-26 7:12 ` Matt Fleming
2013-04-26 7:43 ` Michel Lespinasse
2013-04-26 8:49 ` Matt Fleming
2013-04-26 9:02 ` Michel Lespinasse
2013-04-26 9:44 ` Matt Fleming
2013-04-26 7:29 ` Ingo Molnar
2013-04-26 9:01 ` Matt Fleming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox