Curious crash with secure variables

Curious crash with secure variables

[James Bottomley]

The crash is attached below.  The curiosity is that the failing
"virtual" address is actually a physical address inside the EFI runtime.
It looks like either SetVirtualAddressMap() failed to relocate
something, or there are caching effects on pre-relocated addresses.

The way to trigger this is to run tianocore in kvm and boot to an
initial ramdisk with the efi tools and a shell.  If I insert a PK in the
UEFI shell and then try to remove it in the initrd, the crash happens.
If, however, I try to insert and remove the PK in the initrd without
touching the secure variables in the UEFI shell, everything works

James

---

[    0.998342] BUG: unable to handle kernel paging request at 000000001e339788
[    1.000046] IP: [<ffff88001e3e4989>] 0xffff88001e3e4988
[    1.000046] PGD 18211067 PUD 181e8067 PMD 0 
[    1.000046] Oops: 0002 [#1] SMP 
[    1.000046] Modules linked in:
[    1.000046] CPU 0 
[    1.000046] Pid: 34, comm: efi-updatevar Not tainted 3.9.0-rc2+ #45  
[    1.000046] RIP: 0010:[<ffff88001e3e4989>]  [<ffff88001e3e4989>] 0xffff88001e3e4988
[    1.000046] RSP: 0018:ffff88001821b9f0  EFLAGS: 00010086
[    1.000046] RAX: 000000001e3396e0 RBX: ffffffff818537c0 RCX: 0000000000000000
[    1.000046] RDX: ffff88001e36eee0 RSI: ffff88001e36eee0 RDI: 000000001e3396e0
[    1.000046] RBP: ffff88001821ba30 R08: ffff88001f9dfd72 R09: 00000000000002cc
[    1.000046] R10: ffff8800181ff800 R11: ffffffff8152573a R12: ffff8800181ff800
[    1.000046] R13: ffff880018193000 R14: 0000000000000573 R15: ffff88001ed654c0
[    1.000046] FS:  00007f161a916700(0000) GS:ffff88001d200000(0000) knlGS:0000000000000000
[    1.000046] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.000046] CR2: 000000001e339788 CR3: 00000000181f4000 CR4: 00000000000006f0
[    1.000046] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    1.000046] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[    1.000046] Process efi-updatevar (pid: 34, threadinfo ffff88001821a000, task ffff880018203870)
[    1.000046] Stack:
[    1.000046]  0000000000000000 0000000000000018 ffff88001e36eee0 000000001e3396e0
[    1.000046]  0000000000000000 0000001800000000 ffff88001e40e670 ffff88001e36eee0
[    1.000046]  ffff88001821ba80 ffff88001e3ab99d 11d293ca8be4df61 ffff88001e4460a0
[    1.000046] Call Trace:
[    1.000046]  [<ffffffff8103a25b>] ? efi_call5+0x4b/0x80
[    1.000046]  [<ffffffff812aaa2e>] ? efivarfs_file_write+0x1f7/0x351
[    1.000046]  [<ffffffff8117999d>] ? security_file_permission+0x15/0x2b
[    1.000046]  [<ffffffff8110dfae>] ? vfs_write+0x96/0xf8
[    1.000046]  [<ffffffff8110e1d6>] ? sys_write+0x51/0x80
[    1.000046]  [<ffffffff813937ed>] ? system_call_fastpath+0x1a/0x1f
[    1.000046] Code: 8b 45 d8 48 89 c7 48 b8 8b 56 3a 1e 00 88 ff ff ff d0 eb 01 90 c9 c3 55 48 89 e5 48 83 ec 40 48 89 7d d8 48 89 75 d0 48 8b 45 d8 <c7> 80 a8 00 00 00 00 00 00 00 48 8b 45 d8 48 8b 48 28 48 8b 45 
[    1.000046] RIP  [<ffff88001e3e4989>] 0xffff88001e3e4988
[    1.000046]  RSP <ffff88001821b9f0>
[    1.000046] CR2: 000000001e339788
[    1.000046] ---[ end trace ee19301618adf435 ]---

Re: Curious crash with secure variables

[Matt Fleming]

On Mon, 2013-03-18 at 08:01 +0000, James Bottomley wrote:
> The crash is attached below.  The curiosity is that the failing
> "virtual" address is actually a physical address inside the EFI runtime.
> It looks like either SetVirtualAddressMap() failed to relocate
> something, or there are caching effects on pre-relocated addresses.
> 
> The way to trigger this is to run tianocore in kvm and boot to an
> initial ramdisk with the efi tools and a shell.  If I insert a PK in the
> UEFI shell and then try to remove it in the initrd, the crash happens.
> If, however, I try to insert and remove the PK in the initrd without
> touching the secure variables in the UEFI shell, everything works
> 
> James

Crashes like this are typical of firmware that fails to update its
internal pointers when SetVirtualAddressMap() is called. There's no
reason that any of the EFI runtime services regions should be skipped
when establishing virtual kernel mappings, unless those regions are
missing the EFI_MEMORY_RUNTIME attribute, which seems unlikely.

Cc'ing Jordan as he may have some idea where the missing calls to
ConvertPointer() are.

> ---
> 
> [    0.998342] BUG: unable to handle kernel paging request at 000000001e339788
> [    1.000046] IP: [<ffff88001e3e4989>] 0xffff88001e3e4988
> [    1.000046] PGD 18211067 PUD 181e8067 PMD 0 
> [    1.000046] Oops: 0002 [#1] SMP 
> [    1.000046] Modules linked in:
> [    1.000046] CPU 0 
> [    1.000046] Pid: 34, comm: efi-updatevar Not tainted 3.9.0-rc2+ #45  
> [    1.000046] RIP: 0010:[<ffff88001e3e4989>]  [<ffff88001e3e4989>] 0xffff88001e3e4988
> [    1.000046] RSP: 0018:ffff88001821b9f0  EFLAGS: 00010086
> [    1.000046] RAX: 000000001e3396e0 RBX: ffffffff818537c0 RCX: 0000000000000000
> [    1.000046] RDX: ffff88001e36eee0 RSI: ffff88001e36eee0 RDI: 000000001e3396e0
> [    1.000046] RBP: ffff88001821ba30 R08: ffff88001f9dfd72 R09: 00000000000002cc
> [    1.000046] R10: ffff8800181ff800 R11: ffffffff8152573a R12: ffff8800181ff800
> [    1.000046] R13: ffff880018193000 R14: 0000000000000573 R15: ffff88001ed654c0
> [    1.000046] FS:  00007f161a916700(0000) GS:ffff88001d200000(0000) knlGS:0000000000000000
> [    1.000046] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.000046] CR2: 000000001e339788 CR3: 00000000181f4000 CR4: 00000000000006f0
> [    1.000046] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    1.000046] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [    1.000046] Process efi-updatevar (pid: 34, threadinfo ffff88001821a000, task ffff880018203870)
> [    1.000046] Stack:
> [    1.000046]  0000000000000000 0000000000000018 ffff88001e36eee0 000000001e3396e0
> [    1.000046]  0000000000000000 0000001800000000 ffff88001e40e670 ffff88001e36eee0
> [    1.000046]  ffff88001821ba80 ffff88001e3ab99d 11d293ca8be4df61 ffff88001e4460a0
> [    1.000046] Call Trace:
> [    1.000046]  [<ffffffff8103a25b>] ? efi_call5+0x4b/0x80
> [    1.000046]  [<ffffffff812aaa2e>] ? efivarfs_file_write+0x1f7/0x351
> [    1.000046]  [<ffffffff8117999d>] ? security_file_permission+0x15/0x2b
> [    1.000046]  [<ffffffff8110dfae>] ? vfs_write+0x96/0xf8
> [    1.000046]  [<ffffffff8110e1d6>] ? sys_write+0x51/0x80
> [    1.000046]  [<ffffffff813937ed>] ? system_call_fastpath+0x1a/0x1f
> [    1.000046] Code: 8b 45 d8 48 89 c7 48 b8 8b 56 3a 1e 00 88 ff ff ff d0 eb 01 90 c9 c3 55 48 89 e5 48 83 ec 40 48 89 7d d8 48 89 75 d0 48 8b 45 d8 <c7> 80 a8 00 00 00 00 00 00 00 48 8b 45 d8 48 8b 48 28 48 8b 45 
> [    1.000046] RIP  [<ffff88001e3e4989>] 0xffff88001e3e4988
> [    1.000046]  RSP <ffff88001821b9f0>
> [    1.000046] CR2: 000000001e339788
> [    1.000046] ---[ end trace ee19301618adf435 ]---

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Curious crash with secure variables

[James Bottomley]

On Mon, 2013-03-18 at 11:49 +0000, Matt Fleming wrote:
> On Mon, 2013-03-18 at 08:01 +0000, James Bottomley wrote:
> > The crash is attached below.  The curiosity is that the failing
> > "virtual" address is actually a physical address inside the EFI runtime.
> > It looks like either SetVirtualAddressMap() failed to relocate
> > something, or there are caching effects on pre-relocated addresses.
> > 
> > The way to trigger this is to run tianocore in kvm and boot to an
> > initial ramdisk with the efi tools and a shell.  If I insert a PK in the
> > UEFI shell and then try to remove it in the initrd, the crash happens.
> > If, however, I try to insert and remove the PK in the initrd without
> > touching the secure variables in the UEFI shell, everything works
> > 
> > James
> 
> Crashes like this are typical of firmware that fails to update its
> internal pointers when SetVirtualAddressMap() is called. There's no
> reason that any of the EFI runtime services regions should be skipped
> when establishing virtual kernel mappings, unless those regions are
> missing the EFI_MEMORY_RUNTIME attribute, which seems unlikely.

Yes, it's a phenomenally complicated operation from looking at the
TianoCore source ... might we not be better off not bothering to
relocate and just using a private physical mapping for the calls?

> Cc'ing Jordan as he may have some idea where the missing calls to
> ConvertPointer() are.

I'm betting it's in SecurityPkg  ... I seem to have run into a roadblock
getting TianoCore to cough up its symbol table, though (qemu -s isn't
working because of some x86_64 problem with gdb).  Is there an easy way
to get it?

James


> > ---
> > 
> > [    0.998342] BUG: unable to handle kernel paging request at 000000001e339788
> > [    1.000046] IP: [<ffff88001e3e4989>] 0xffff88001e3e4988
> > [    1.000046] PGD 18211067 PUD 181e8067 PMD 0 
> > [    1.000046] Oops: 0002 [#1] SMP 
> > [    1.000046] Modules linked in:
> > [    1.000046] CPU 0 
> > [    1.000046] Pid: 34, comm: efi-updatevar Not tainted 3.9.0-rc2+ #45  
> > [    1.000046] RIP: 0010:[<ffff88001e3e4989>]  [<ffff88001e3e4989>] 0xffff88001e3e4988
> > [    1.000046] RSP: 0018:ffff88001821b9f0  EFLAGS: 00010086
> > [    1.000046] RAX: 000000001e3396e0 RBX: ffffffff818537c0 RCX: 0000000000000000
> > [    1.000046] RDX: ffff88001e36eee0 RSI: ffff88001e36eee0 RDI: 000000001e3396e0
> > [    1.000046] RBP: ffff88001821ba30 R08: ffff88001f9dfd72 R09: 00000000000002cc
> > [    1.000046] R10: ffff8800181ff800 R11: ffffffff8152573a R12: ffff8800181ff800
> > [    1.000046] R13: ffff880018193000 R14: 0000000000000573 R15: ffff88001ed654c0
> > [    1.000046] FS:  00007f161a916700(0000) GS:ffff88001d200000(0000) knlGS:0000000000000000
> > [    1.000046] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    1.000046] CR2: 000000001e339788 CR3: 00000000181f4000 CR4: 00000000000006f0
> > [    1.000046] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [    1.000046] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > [    1.000046] Process efi-updatevar (pid: 34, threadinfo ffff88001821a000, task ffff880018203870)
> > [    1.000046] Stack:
> > [    1.000046]  0000000000000000 0000000000000018 ffff88001e36eee0 000000001e3396e0
> > [    1.000046]  0000000000000000 0000001800000000 ffff88001e40e670 ffff88001e36eee0
> > [    1.000046]  ffff88001821ba80 ffff88001e3ab99d 11d293ca8be4df61 ffff88001e4460a0
> > [    1.000046] Call Trace:
> > [    1.000046]  [<ffffffff8103a25b>] ? efi_call5+0x4b/0x80
> > [    1.000046]  [<ffffffff812aaa2e>] ? efivarfs_file_write+0x1f7/0x351
> > [    1.000046]  [<ffffffff8117999d>] ? security_file_permission+0x15/0x2b
> > [    1.000046]  [<ffffffff8110dfae>] ? vfs_write+0x96/0xf8
> > [    1.000046]  [<ffffffff8110e1d6>] ? sys_write+0x51/0x80
> > [    1.000046]  [<ffffffff813937ed>] ? system_call_fastpath+0x1a/0x1f
> > [    1.000046] Code: 8b 45 d8 48 89 c7 48 b8 8b 56 3a 1e 00 88 ff ff ff d0 eb 01 90 c9 c3 55 48 89 e5 48 83 ec 40 48 89 7d d8 48 89 75 d0 48 8b 45 d8 <c7> 80 a8 00 00 00 00 00 00 00 48 8b 45 d8 48 8b 48 28 48 8b 45 
> > [    1.000046] RIP  [<ffff88001e3e4989>] 0xffff88001e3e4988
> > [    1.000046]  RSP <ffff88001821b9f0>
> > [    1.000046] CR2: 000000001e339788
> > [    1.000046] ---[ end trace ee19301618adf435 ]---
> 

Re: Curious crash with secure variables

[David Woodhouse]

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

On Mon, 2013-03-18 at 14:23 +0000, James Bottomley wrote:
> On Mon, 2013-03-18 at 11:49 +0000, Matt Fleming wrote:
> > On Mon, 2013-03-18 at 08:01 +0000, James Bottomley wrote:
> > > The crash is attached below.  The curiosity is that the failing
> > > "virtual" address is actually a physical address inside the EFI runtime.
> > > It looks like either SetVirtualAddressMap() failed to relocate
> > > something, or there are caching effects on pre-relocated addresses.
> > > 
> > > The way to trigger this is to run tianocore in kvm and boot to an
> > > initial ramdisk with the efi tools and a shell.  If I insert a PK in the
> > > UEFI shell and then try to remove it in the initrd, the crash happens.
> > > If, however, I try to insert and remove the PK in the initrd without
> > > touching the secure variables in the UEFI shell, everything works
> > > 
> > > James
> > 
> > Crashes like this are typical of firmware that fails to update its
> > internal pointers when SetVirtualAddressMap() is called. There's no
> > reason that any of the EFI runtime services regions should be skipped
> > when establishing virtual kernel mappings, unless those regions are
> > missing the EFI_MEMORY_RUNTIME attribute, which seems unlikely.
> 
> Yes, it's a phenomenally complicated operation from looking at the
> TianoCore source ... might we not be better off not bothering to
> relocate and just using a private physical mapping for the calls?

There's a lot to be said for that. It would fix kexec too — which is
currently broken because you're only allowed to call
SetVirtualAddressMap once.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: Curious crash with secure variables

[Matt Fleming]

On 03/18/2013 02:23 PM, James Bottomley wrote:
> Yes, it's a phenomenally complicated operation from looking at the
> TianoCore source ... might we not be better off not bothering to
> relocate and just using a private physical mapping for the calls?

Yeah, there have been various discussions about doing this. I sent some
patches last year but they broke various non-EFI machines and I haven't
had chance to pick it up again.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Curious crash with secure variables

[David Woodhouse]

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

On Mon, 2013-03-18 at 15:02 +0000, Matt Fleming wrote:
> On 03/18/2013 02:23 PM, James Bottomley wrote:
> > Yes, it's a phenomenally complicated operation from looking at the
> > TianoCore source ... might we not be better off not bothering to
> > relocate and just using a private physical mapping for the calls?
> 
> Yeah, there have been various discussions about doing this. I sent some
> patches last year but they broke various non-EFI machines and I haven't
> had chance to pick it up again.

Got a pointer? I may take a look...

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: Curious crash with secure variables

[Matt Fleming]

On 03/18/2013 03:04 PM, David Woodhouse wrote:
> On Mon, 2013-03-18 at 15:02 +0000, Matt Fleming wrote:
>> On 03/18/2013 02:23 PM, James Bottomley wrote:
>>> Yes, it's a phenomenally complicated operation from looking at the
>>> TianoCore source ... might we not be better off not bothering to
>>> relocate and just using a private physical mapping for the calls?
>>
>> Yeah, there have been various discussions about doing this. I sent some
>> patches last year but they broke various non-EFI machines and I haven't
>> had chance to pick it up again.
> 
> Got a pointer? I may take a look...
> 

See,

  commit 53b87cf0 ("x86, mm: Include the entire kernel memory map in trampoline_pgd"),
  commit 185034e7 ("x86, efi: 1:1 pagetable mapping for virtual EFI calls"),
  commit da5a108d05b4 ("x86/kernel: remove tboot 1:1 page table creation code") and 
  commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)")

 and the two revert commits from Linus, be354f40 and 11520e5e.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Curious crash with secure variables

[David Woodhouse]

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

On Mon, 2013-03-18 at 15:16 +0000, Matt Fleming wrote:
> 
> See,
> 
>   commit 53b87cf0 ("x86, mm: Include the entire kernel memory map in trampoline_pgd"),
>   commit 185034e7 ("x86, efi: 1:1 pagetable mapping for virtual EFI calls"),
>   commit da5a108d05b4 ("x86/kernel: remove tboot 1:1 page table creation code") and 
>   commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)")
> 
>  and the two revert commits from Linus, be354f40 and 11520e5e.

Thanks. That seems like a rather scary approach. I was thinking of just
setting up a dedicated kernel thread for making runtime services calls,
and giving it some "userspace" page tables with a 1:1 mapping. No
messing around with %cr3 directly.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: Curious crash with secure variables

[Matt Fleming]

On 03/18/2013 03:32 PM, David Woodhouse wrote:
> On Mon, 2013-03-18 at 15:16 +0000, Matt Fleming wrote:
>>
>> See,
>>
>>   commit 53b87cf0 ("x86, mm: Include the entire kernel memory map in trampoline_pgd"),
>>   commit 185034e7 ("x86, efi: 1:1 pagetable mapping for virtual EFI calls"),
>>   commit da5a108d05b4 ("x86/kernel: remove tboot 1:1 page table creation code") and 
>>   commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)")
>>
>>  and the two revert commits from Linus, be354f40 and 11520e5e.
> 
> Thanks. That seems like a rather scary approach. I was thinking of just
> setting up a dedicated kernel thread for making runtime services calls,
> and giving it some "userspace" page tables with a 1:1 mapping. No
> messing around with %cr3 directly.

How would that work? Would it be a real, executable thread context as
opposed to just an address space? In which case would we be passing data
to this thread for it to execute on our behalf? One thing to be aware of
is that sometimes we need to make EFI calls when the sky is falling,
such as writing EFI variables in the pstore code paths when crashing.
Scheduling things at that point may be difficult.

Provided that you can still do things like that, it seems like a nice
solution.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: Curious crash with secure variables

[H. Peter Anvin]

On 03/18/2013 11:04 AM, Matt Fleming wrote:
> On 03/18/2013 03:32 PM, David Woodhouse wrote:
>> On Mon, 2013-03-18 at 15:16 +0000, Matt Fleming wrote:
>>>
>>> See,
>>>
>>>   commit 53b87cf0 ("x86, mm: Include the entire kernel memory map in trampoline_pgd"),
>>>   commit 185034e7 ("x86, efi: 1:1 pagetable mapping for virtual EFI calls"),
>>>   commit da5a108d05b4 ("x86/kernel: remove tboot 1:1 page table creation code") and 
>>>   commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall clock (again)")
>>>
>>>  and the two revert commits from Linus, be354f40 and 11520e5e.
>>
>> Thanks. That seems like a rather scary approach. I was thinking of just
>> setting up a dedicated kernel thread for making runtime services calls,
>> and giving it some "userspace" page tables with a 1:1 mapping. No
>> messing around with %cr3 directly.
> 
> How would that work? Would it be a real, executable thread context as
> opposed to just an address space? In which case would we be passing data
> to this thread for it to execute on our behalf? One thing to be aware of
> is that sometimes we need to make EFI calls when the sky is falling,
> such as writing EFI variables in the pstore code paths when crashing.
> Scheduling things at that point may be difficult.
> 
> Provided that you can still do things like that, it seems like a nice
> solution.
> 

What is the point?

We don't need the scheduler to be involved, we just want to do a
temporary context switch.  We can't preempt EFI anyway, so given that,
we are non-preemptable and switching %cr3 is fine.

	-hpa