* [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware @ 2017-02-15 4:03 Sai Praneeth Prakhya [not found] ` <1487131421-23703-1-git-send-email-sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Sai Praneeth Prakhya @ 2017-02-15 4:03 UTC (permalink / raw) To: linux-efi-u79uwXL29TY76Z2rM5mHXA Cc: Sai Praneeth, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Matt Fleming, Ard Biesheuvel, Ravi Shankar, Fenghua Yu From: Sai Praneeth <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> There are some machines with buggy firmware that access EFI regions in 1:1 mode (or physical mode) rather than virtual mode even after kernel being booted. On these machines, if we invoke an EFI runtime service (that does these buggy accesses) then it causes a page fault and hence results in kernel hang. The page fault happens because the requested region doesn't have appropriate page attributes set or the mapping for the region might be missing. This issue was introduced by commit 67a9108ed431 ("x86/efi: Build our own page table structures"). Before this commit, 1:1 mappings for EFI regions were in swapper_pgd and were not needed to be synced, but this commit introduced efi_pgd which missed these mappings. Below is the edited version of the page fault output that I noticed: BUG: unable to handle kernel paging request at 0000000018847980 IP: [<fffffffef6981ec3>] 0xfffffffef6981ec3 PGD 4324d3063 PUD 4324e4063 PMD 0 Oops: 0000 [#1] SMP Modules linked in: efi_runtime(O) chipsec(O) igb e1000e [last unloaded: efi_runtime] CPU: 5 PID: 2308 Comm: fwts Tainted: G W O 4.7.0-yocto-standard #1 Hardware name: Gigabyte Technology Co., Ltd. Z87X-UD5H/Z87X-UD5H-CF, BIOS F9 03/18/2014 task: ffff88041974d900 ti: ffff880418844000 task.ti: ffff880418844000 RIP: 0010:[<fffffffef6981ec3>] [<fffffffef6981ec3>] 0xfffffffef6981ec3 RSP: 0018:ffff880418847868 EFLAGS: 00010297 RAX: 00000000000000e0 RBX: 0000000000000000 RCX: ffff880418847a50 RDX: 0000000018847980 RSI: ffff880418847a10 RDI: 0000000000000020 RBP: 0000000000000001 R08: ffff880418847a68 R09: 0000000000000000 R10: ffff880418847a70 R11: 0000000000000008 R12: ffff88042005c130 R13: 0000000000000010 R14: ffff880418847e30 R15: 0000000000000000 FS: 00007fb94251e700(0000) GS:ffff880432b40000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000018847980 CR3: 00000004324d1000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: [<ffffffff810abf01>] ? __lock_acquire+0x121/0x1480 [<ffffffff810ac57f>] ? __lock_acquire+0x79f/0x1480 [<ffffffff810599a8>] ? efi_call+0x58/0x90 [<ffffffff81748e4b>] ? virt_efi_set_variable+0x7b/0x190 [<ffffffffa0065e63>] ? efi_runtime_ioctl+0xe43/0x108c [efi_runtime] [<ffffffff810893b4>] ? __might_sleep+0x44/0x80 [<ffffffff811e07de>] ? do_vfs_ioctl+0x8e/0x660 [<ffffffff8133dd4e>] ? security_file_ioctl+0x3e/0x60 [<ffffffff811e0e24>] ? SyS_ioctl+0x74/0x80 [<ffffffff81a20de5>] ? entry_SYSCALL_64_fastpath+0x18/0xa8 I have looked at EFI Memory map for the faulted address and found that it belongs to memory region of type "Conventional Memory". So, this firmware bug is not same as accessing EFI_BOOT_SERVICES_* regions after boot, but firmware accessing *illegal regions* in *1:1 mode*. Below shown are the efi_pgd dumps before and after the bad commit. efi_dump_pagetable() is called before calling efi_merge_regions() in __efi_enter_virtual_mode() and this kernel is booted on qemu to obtain page table dumps. EFI_PGT_DUMP before commit: --------------------------- [0.007041] ---[ User Space ]--- [0.007427] 0x0000000000000000-0x0000000000200000 2M RW GLB NX pte [0.008609] 0x0000000000200000-0x0000000000800000 6M RW PSE GLB NX pmd [0.010069] 0x0000000000800000-0x0000000000808000 32K pte [0.011068] 0x0000000000808000-0x0000000000810000 32K RW GLB NX pte [0.012325] 0x0000000000810000-0x0000000000900000 960K pte [0.013071] 0x0000000000900000-0x0000000000a00000 1M RW GLB NX pte [0.014579] 0x0000000000a00000-0x000000007e800000 2014M RW PSE GLB NX pmd [0.015593] 0x000000007e800000-0x000000007e9b6000 1752K RW GLB NX pte [0.016600] 0x000000007e9b6000-0x000000007e9fe000 288K pte [0.018003] 0x000000007e9fe000-0x000000007ea00000 8K RW GLB NX pte [0.019165] 0x000000007ea00000-0x000000007ec00000 2M RW PSE GLB NX pmd [0.020331] 0x000000007ec00000-0x000000007eda9000 1700K RW GLB NX pte [0.021483] 0x000000007eda9000-0x000000007ee14000 428K pte [0.022500] 0x000000007ee14000-0x000000007f000000 1968K RW GLB NX pte [0.023596] 0x000000007f000000-0x000000007fe00000 14M RW PSE GLB NX pmd [0.025004] 0x000000007fe00000-0x000000007fe94000 592K RW GLB NX pte [0.026220] 0x000000007fe94000-0x000000007fef8000 400K pte [0.027069] 0x000000007fef8000-0x000000007ffd0000 864K RW GLB NX pte [0.028420] 0x000000007ffd0000-0x000000007fff0000 128K pte [0.029551] 0x000000007fff0000-0x0000000080000000 64K RW GLB NX pte [0.030601] 0x0000000080000000-0x0000008000000000 510G pud [0.031499] 0x0000008000000000-0xffff800000000000 17179737600G pgd [0.032152] ---[ Kernel Space ]--- EFI_PGT_DUMP after commit: -------------------------- [0.005620] ---[ User Space ]--- [0.005838] 0x0000000000000000-0xffff800000000000 16777088T pgd [0.005873] ---[ Kernel Space ]--- While not having these mappings isn't a bug but we need these mappings to support machines with buggy firmware. Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> Cc: Ricardo Neri <ricardo.neri-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Cc: Ravi Shankar <ravi.v.shankar-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> --- arch/x86/platform/efi/efi_64.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index de9652bd19d4..8d2fd6568b91 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -139,6 +139,7 @@ int __init efi_alloc_page_tables(void) pgd_t *pgd; pud_t *pud; gfp_t gfp_mask; + unsigned num_pgds; if (efi_enabled(EFI_OLD_MEMMAP)) return 0; @@ -158,6 +159,13 @@ int __init efi_alloc_page_tables(void) pgd_populate(NULL, pgd, pud); + /* + * Sync 1:1 mappings to support buggy firmware which haven't updated + * their addresses even after kernel has booted. + */ + num_pgds = pgd_index(VMALLOC_START) - pgd_index(PAGE_OFFSET); + memcpy(efi_pgd, pgd_offset_k(PAGE_OFFSET), sizeof(pgd_t) * num_pgds); + return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <1487131421-23703-1-git-send-email-sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware [not found] ` <1487131421-23703-1-git-send-email-sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> @ 2017-02-28 11:51 ` Matt Fleming [not found] ` <20170228115104.GC28416-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Matt Fleming @ 2017-02-28 11:51 UTC (permalink / raw) To: Sai Praneeth Prakhya Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Ard Biesheuvel, Ravi Shankar, Fenghua Yu On Tue, 14 Feb, at 08:03:41PM, Sai Praneeth Prakhya wrote: > From: Sai Praneeth <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > There are some machines with buggy firmware that access EFI regions in > 1:1 mode (or physical mode) rather than virtual mode even after kernel > being booted. On these machines, if we invoke an EFI runtime service > (that does these buggy accesses) then it causes a page fault and hence > results in kernel hang. The page fault happens because the requested > region doesn't have appropriate page attributes set or the mapping for > the region might be missing. This issue was introduced by commit > 67a9108ed431 ("x86/efi: Build our own page table structures"). Before > this commit, 1:1 mappings for EFI regions were in swapper_pgd and were > not needed to be synced, but this commit introduced efi_pgd which missed > these mappings. The subject line needs to mention EFI_CONVENTIONAL_MEMORY, because we already have 1:1 mappings for some regions, just not for EFI_CONVENTIONAL_MEMORY (by default). Those are the missing mappings. > Below is the edited version of the page fault output that I noticed: > > BUG: unable to handle kernel paging request at 0000000018847980 > IP: [<fffffffef6981ec3>] 0xfffffffef6981ec3 > PGD 4324d3063 PUD 4324e4063 PMD 0 > Oops: 0000 [#1] SMP > Modules linked in: efi_runtime(O) chipsec(O) igb e1000e [last unloaded: > efi_runtime] > CPU: 5 PID: 2308 Comm: fwts Tainted: G W O 4.7.0-yocto-standard #1 > Hardware name: Gigabyte Technology Co., Ltd. Z87X-UD5H/Z87X-UD5H-CF, > BIOS F9 03/18/2014 > task: ffff88041974d900 ti: ffff880418844000 task.ti: ffff880418844000 > RIP: 0010:[<fffffffef6981ec3>] [<fffffffef6981ec3>] 0xfffffffef6981ec3 > RSP: 0018:ffff880418847868 EFLAGS: 00010297 > RAX: 00000000000000e0 RBX: 0000000000000000 RCX: ffff880418847a50 > RDX: 0000000018847980 RSI: ffff880418847a10 RDI: 0000000000000020 > RBP: 0000000000000001 R08: ffff880418847a68 R09: 0000000000000000 > R10: ffff880418847a70 R11: 0000000000000008 R12: ffff88042005c130 > R13: 0000000000000010 R14: ffff880418847e30 R15: 0000000000000000 > FS: 00007fb94251e700(0000) GS:ffff880432b40000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000018847980 CR3: 00000004324d1000 CR4: 00000000001406e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > [<ffffffff810abf01>] ? __lock_acquire+0x121/0x1480 > [<ffffffff810ac57f>] ? __lock_acquire+0x79f/0x1480 > [<ffffffff810599a8>] ? efi_call+0x58/0x90 > [<ffffffff81748e4b>] ? virt_efi_set_variable+0x7b/0x190 > [<ffffffffa0065e63>] ? efi_runtime_ioctl+0xe43/0x108c [efi_runtime] > [<ffffffff810893b4>] ? __might_sleep+0x44/0x80 > [<ffffffff811e07de>] ? do_vfs_ioctl+0x8e/0x660 > [<ffffffff8133dd4e>] ? security_file_ioctl+0x3e/0x60 > [<ffffffff811e0e24>] ? SyS_ioctl+0x74/0x80 > [<ffffffff81a20de5>] ? entry_SYSCALL_64_fastpath+0x18/0xa8 > > I have looked at EFI Memory map for the faulted address and found that > it belongs to memory region of type "Conventional Memory". So, this > firmware bug is not same as accessing EFI_BOOT_SERVICES_* regions after > boot, but firmware accessing *illegal regions* in *1:1 mode*. Can you confirm that the firmware is accessing the corresponding physical address of a virtual address that was passed an argument to the EFI runtime service? In other words, is the firmware accessing memory (via the wrong mapping) that the kernel has passed, or is it accessing memory locations it shouldn't be? > Below shown are the efi_pgd dumps before and after the bad commit. > efi_dump_pagetable() is called before calling efi_merge_regions() in > __efi_enter_virtual_mode() and this kernel is booted on qemu to obtain > page table dumps. > > EFI_PGT_DUMP before commit: > --------------------------- > [0.007041] ---[ User Space ]--- > [0.007427] 0x0000000000000000-0x0000000000200000 2M RW GLB NX pte > [0.008609] 0x0000000000200000-0x0000000000800000 6M RW PSE GLB NX pmd > [0.010069] 0x0000000000800000-0x0000000000808000 32K pte > [0.011068] 0x0000000000808000-0x0000000000810000 32K RW GLB NX pte > [0.012325] 0x0000000000810000-0x0000000000900000 960K pte > [0.013071] 0x0000000000900000-0x0000000000a00000 1M RW GLB NX pte > [0.014579] 0x0000000000a00000-0x000000007e800000 2014M RW PSE GLB NX pmd > [0.015593] 0x000000007e800000-0x000000007e9b6000 1752K RW GLB NX pte > [0.016600] 0x000000007e9b6000-0x000000007e9fe000 288K pte > [0.018003] 0x000000007e9fe000-0x000000007ea00000 8K RW GLB NX pte > [0.019165] 0x000000007ea00000-0x000000007ec00000 2M RW PSE GLB NX pmd > [0.020331] 0x000000007ec00000-0x000000007eda9000 1700K RW GLB NX pte > [0.021483] 0x000000007eda9000-0x000000007ee14000 428K pte > [0.022500] 0x000000007ee14000-0x000000007f000000 1968K RW GLB NX pte > [0.023596] 0x000000007f000000-0x000000007fe00000 14M RW PSE GLB NX pmd > [0.025004] 0x000000007fe00000-0x000000007fe94000 592K RW GLB NX pte > [0.026220] 0x000000007fe94000-0x000000007fef8000 400K pte > [0.027069] 0x000000007fef8000-0x000000007ffd0000 864K RW GLB NX pte > [0.028420] 0x000000007ffd0000-0x000000007fff0000 128K pte > [0.029551] 0x000000007fff0000-0x0000000080000000 64K RW GLB NX pte > [0.030601] 0x0000000080000000-0x0000008000000000 510G pud > [0.031499] 0x0000008000000000-0xffff800000000000 17179737600G pgd > [0.032152] ---[ Kernel Space ]--- > > EFI_PGT_DUMP after commit: > -------------------------- > [0.005620] ---[ User Space ]--- > [0.005838] 0x0000000000000000-0xffff800000000000 16777088T pgd > [0.005873] ---[ Kernel Space ]--- > > While not having these mappings isn't a bug but we need these mappings > to support machines with buggy firmware. If buggy machines do not boot because we do not have these mappings, then yes, it is a bug. > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> > Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> > Cc: Ricardo Neri <ricardo.neri-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> > Cc: Ravi Shankar <ravi.v.shankar-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > --- > arch/x86/platform/efi/efi_64.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index de9652bd19d4..8d2fd6568b91 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -139,6 +139,7 @@ int __init efi_alloc_page_tables(void) > pgd_t *pgd; > pud_t *pud; > gfp_t gfp_mask; > + unsigned num_pgds; > > if (efi_enabled(EFI_OLD_MEMMAP)) > return 0; > @@ -158,6 +159,13 @@ int __init efi_alloc_page_tables(void) > > pgd_populate(NULL, pgd, pud); > > + /* > + * Sync 1:1 mappings to support buggy firmware which haven't updated > + * their addresses even after kernel has booted. > + */ > + num_pgds = pgd_index(VMALLOC_START) - pgd_index(PAGE_OFFSET); > + memcpy(efi_pgd, pgd_offset_k(PAGE_OFFSET), sizeof(pgd_t) * num_pgds); > + > return 0; > } > I don't think we should be adding yet another place in the EFI code where we're modifying the page tables. We already have the ability to map EFI_CONVENTIONAL_MEMORY regions inside of efi_map_regions() via the should_map_region() function. Currently, unless you're booting in mixed mode that function will return 'false' if the region type is EFI_CONVENTIONAL_MEMORY, so to get your machine booting you need to do two things, 1) Modify should_map_region() to allow EFI_CONVENTIONAL_MEMORY to be mapped 2) Modify the 64-bit version of efi_map_region() to *only* create 1:1 mapping for EFI_CONVENTIONAL_MEMORY regions. ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20170228115104.GC28416-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]
* Re: [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware [not found] ` <20170228115104.GC28416-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> @ 2017-03-01 1:39 ` Sai Praneeth Prakhya [not found] ` <1488332358.4028.15.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Sai Praneeth Prakhya @ 2017-03-01 1:39 UTC (permalink / raw) To: Matt Fleming Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Ard Biesheuvel, Ravi Shankar, Fenghua Yu On Tue, 2017-02-28 at 11:51 +0000, Matt Fleming wrote: > On Tue, 14 Feb, at 08:03:41PM, Sai Praneeth Prakhya wrote: > > From: Sai Praneeth <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > > > There are some machines with buggy firmware that access EFI regions in > > 1:1 mode (or physical mode) rather than virtual mode even after kernel > > being booted. On these machines, if we invoke an EFI runtime service > > (that does these buggy accesses) then it causes a page fault and hence > > results in kernel hang. The page fault happens because the requested > > region doesn't have appropriate page attributes set or the mapping for > > the region might be missing. This issue was introduced by commit > > 67a9108ed431 ("x86/efi: Build our own page table structures"). Before > > this commit, 1:1 mappings for EFI regions were in swapper_pgd and were > > not needed to be synced, but this commit introduced efi_pgd which missed > > these mappings. > > The subject line needs to mention EFI_CONVENTIONAL_MEMORY, because we > already have 1:1 mappings for some regions, just not for > EFI_CONVENTIONAL_MEMORY (by default). Those are the missing mappings. Sorry! for the confusing subject line. > > > Below is the edited version of the page fault output that I noticed: > > > > BUG: unable to handle kernel paging request at 0000000018847980 > > I have looked at EFI Memory map for the faulted address and found that > > it belongs to memory region of type "Conventional Memory". So, this > > firmware bug is not same as accessing EFI_BOOT_SERVICES_* regions after > > boot, but firmware accessing *illegal regions* in *1:1 mode*. > > Can you confirm that the firmware is accessing the corresponding > physical address of a virtual address that was passed an argument to > the EFI runtime service? In other words, is the firmware accessing > memory (via the wrong mapping) that the kernel has passed, or is it > accessing memory locations it shouldn't be? > Sure! Will do that. Will change the patch according to your suggestion (mapping EFI_CONVENTIONAL_MEMORY only in 1:1 mode) and see if that fixes the issue. I am sure that it's the buggy firmware causing the issue but let me just confirm. > > Below shown are the efi_pgd dumps before and after the bad commit. > > efi_dump_pagetable() is called before calling efi_merge_regions() in > > __efi_enter_virtual_mode() and this kernel is booted on qemu to obtain > > page table dumps. > > While not having these mappings isn't a bug but we need these mappings > > to support machines with buggy firmware. > > If buggy machines do not boot because we do not have these mappings, > then yes, it is a bug. > Actually, the machine does boot. But after booting when I run some efi tests (using LUV) and when the tests call some EFI_RUNTIME_SERVICE (efi_query_capsule_caps(), efi_get_next_variable()), the system hangs. It hangs because page fault occurs in kernel mode and it is not resolved. > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> > > Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> > > Cc: Ricardo Neri <ricardo.neri-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> > > I don't think we should be adding yet another place in the EFI code > where we're modifying the page tables. > > We already have the ability to map EFI_CONVENTIONAL_MEMORY regions > inside of efi_map_regions() via the should_map_region() function. > > Currently, unless you're booting in mixed mode that function will > return 'false' if the region type is EFI_CONVENTIONAL_MEMORY, so to > get your machine booting you need to do two things, > > 1) Modify should_map_region() to allow EFI_CONVENTIONAL_MEMORY to be > mapped > > 2) Modify the 64-bit version of efi_map_region() to *only* create > 1:1 mapping for EFI_CONVENTIONAL_MEMORY regions. Thanks for the suggestions! Will try these and will let you know if that fixes the issue. ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1488332358.4028.15.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware [not found] ` <1488332358.4028.15.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> @ 2017-03-01 3:25 ` Sai Praneeth Prakhya [not found] ` <1488338734.4028.32.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Sai Praneeth Prakhya @ 2017-03-01 3:25 UTC (permalink / raw) To: Matt Fleming Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Ard Biesheuvel, Ravi Shankar, Fenghua Yu > > > > I don't think we should be adding yet another place in the EFI code > > where we're modifying the page tables. > > > > We already have the ability to map EFI_CONVENTIONAL_MEMORY regions > > inside of efi_map_regions() via the should_map_region() function. > > > > Currently, unless you're booting in mixed mode that function will > > return 'false' if the region type is EFI_CONVENTIONAL_MEMORY, so to > > get your machine booting you need to do two things, > > > > 1) Modify should_map_region() to allow EFI_CONVENTIONAL_MEMORY to be > > mapped > > > > 2) Modify the 64-bit version of efi_map_region() to *only* create > > 1:1 mapping for EFI_CONVENTIONAL_MEMORY regions. > > Thanks for the suggestions! Will try these and will let you know if that > fixes the issue. > I have noticed this issue on two machines HP laptop and a Desktop (Gigabyte). Adding mappings for EFI_CONVENTIONAL_MEMORY and *EFI_LOADER_DATA* solves the issue. Presently, I only have access to one machine (Desktop) and as soon as I test this on laptop (hopefully it does not access any other EFI regions illegally), I will send version 3 of the patch. Since this patch will be mapping EFI_CONVENTIONAL_MEMORY and EFI_LOADER_DATA in 1:1 mode on 64-bit machines, I think we could also do the same with EFI_BOOT_SERVICES_DATA and EFI_BOOT_SERVICES_CODE. In other words we could remove the existing mappings for EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA from VA mapping. I think; this should not break any machines because firmware could access illegal addresses only in 1:1 mode and not in VA mode because that's the only address space firmware has knowledge about. Firmware doesn't know about virtual addresses until we pass them through SetVirtualAddressMap(). So Matt, could you please confirm if you had come across any machines that did illegal access to EFI regions using virtual addresses? Regards, Sai ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1488338734.4028.32.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware [not found] ` <1488338734.4028.32.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> @ 2017-03-16 11:47 ` Matt Fleming [not found] ` <20170316114722.GD6261-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Matt Fleming @ 2017-03-16 11:47 UTC (permalink / raw) To: Sai Praneeth Prakhya Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Ard Biesheuvel, Ravi Shankar, Fenghua Yu On Tue, 28 Feb, at 07:25:34PM, Sai Praneeth Prakhya wrote: > > > > > > > I don't think we should be adding yet another place in the EFI code > > > where we're modifying the page tables. > > > > > > We already have the ability to map EFI_CONVENTIONAL_MEMORY regions > > > inside of efi_map_regions() via the should_map_region() function. > > > > > > Currently, unless you're booting in mixed mode that function will > > > return 'false' if the region type is EFI_CONVENTIONAL_MEMORY, so to > > > get your machine booting you need to do two things, > > > > > > 1) Modify should_map_region() to allow EFI_CONVENTIONAL_MEMORY to be > > > mapped > > > > > > 2) Modify the 64-bit version of efi_map_region() to *only* create > > > 1:1 mapping for EFI_CONVENTIONAL_MEMORY regions. > > > > Thanks for the suggestions! Will try these and will let you know if that > > fixes the issue. > > > > I have noticed this issue on two machines HP laptop and a Desktop > (Gigabyte). Adding mappings for EFI_CONVENTIONAL_MEMORY and > *EFI_LOADER_DATA* solves the issue. Presently, I only have access to one > machine (Desktop) and as soon as I test this on laptop (hopefully it > does not access any other EFI regions illegally), I will send version 3 > of the patch. > > Since this patch will be mapping EFI_CONVENTIONAL_MEMORY and > EFI_LOADER_DATA in 1:1 mode on 64-bit machines, I think we could also do > the same with EFI_BOOT_SERVICES_DATA and EFI_BOOT_SERVICES_CODE. In > other words we could remove the existing mappings for > EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA from VA mapping. Do you know what data is being access in the EFI_LOADER_DATA region? Accessing that via the 1:1 mapping is really strange because the firmware will have had to convert any addresses the kernel gave it from virtual to physical (the kernel stores things in EFI_LOADER_DATA regions during boot). > I think; this should not break any machines because firmware could > access illegal addresses only in 1:1 mode and not in VA mode because > that's the only address space firmware has knowledge about. Firmware > doesn't know about virtual addresses until we pass them through > SetVirtualAddressMap(). > > So Matt, could you please confirm if you had come across any machines > that did illegal access to EFI regions using virtual addresses? I don't think I do have access to such machines, but what would removing the virtual mappings buy us? The risk of breaking machines with buggy firmware outweighs any benefit that I can think of. Am I missing something? ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20170316114722.GD6261-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]
* Re: [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware [not found] ` <20170316114722.GD6261-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> @ 2017-03-20 18:52 ` Sai Praneeth Prakhya 0 siblings, 0 replies; 6+ messages in thread From: Sai Praneeth Prakhya @ 2017-03-20 18:52 UTC (permalink / raw) To: Matt Fleming Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Lee, Chun-Yi, Borislav Petkov, Ricardo Neri, Ard Biesheuvel, Ravi Shankar, Fenghua Yu On Thu, 2017-03-16 at 11:47 +0000, Matt Fleming wrote: > On Tue, 28 Feb, at 07:25:34PM, Sai Praneeth Prakhya wrote: > > > > > > > > > > I don't think we should be adding yet another place in the EFI code > > > > where we're modifying the page tables. > > > > > > > > We already have the ability to map EFI_CONVENTIONAL_MEMORY regions > > > > inside of efi_map_regions() via the should_map_region() function. > > > > > > > > Currently, unless you're booting in mixed mode that function will > > > > return 'false' if the region type is EFI_CONVENTIONAL_MEMORY, so to > > > > get your machine booting you need to do two things, > > > > > > > > 1) Modify should_map_region() to allow EFI_CONVENTIONAL_MEMORY to be > > > > mapped > > > > > > > > 2) Modify the 64-bit version of efi_map_region() to *only* create > > > > 1:1 mapping for EFI_CONVENTIONAL_MEMORY regions. > > > > > > Thanks for the suggestions! Will try these and will let you know if that > > > fixes the issue. > > > > > > > I have noticed this issue on two machines HP laptop and a Desktop > > (Gigabyte). Adding mappings for EFI_CONVENTIONAL_MEMORY and > > *EFI_LOADER_DATA* solves the issue. Presently, I only have access to one > > machine (Desktop) and as soon as I test this on laptop (hopefully it > > does not access any other EFI regions illegally), I will send version 3 > > of the patch. > > > > Since this patch will be mapping EFI_CONVENTIONAL_MEMORY and > > EFI_LOADER_DATA in 1:1 mode on 64-bit machines, I think we could also do > > the same with EFI_BOOT_SERVICES_DATA and EFI_BOOT_SERVICES_CODE. In > > other words we could remove the existing mappings for > > EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA from VA mapping. > > Do you know what data is being access in the EFI_LOADER_DATA region? > Accessing that via the 1:1 mapping is really strange because the > firmware will have had to convert any addresses the kernel gave it > from virtual to physical (the kernel stores things in EFI_LOADER_DATA > regions during boot). By looking up the EFI_MEMORY_MAP for the faulted phys_addr, I have confirmed that firmware tried to access EFI_CONVENTIONAL_MEMORY. I don't know what data is being accessed, but I can do a hexdump of that region (If you are interested in the data). (According to my speculation) The buggy firmware here is not converting virtual addresses given by kernel to physical addresses and then triggering these illegal accesses, but the firmware might be referencing some data that is in EFI_CONVENTIONAL_MEMORY or EFI_LOADER_DATA. This might happen because firmware didn't copy all the data that it needs for runtime into EFI_RUNTIME_DATA regions before giving control to kernel or hasn't updated all it's pointers during ConvertPointer() or maybe something else. Based on this speculation I have written patches to kernel which are in Linux UEFI Validation (LUV) and they detected illegal accesses from efi_runtime_services(). When firmware does these illegal accesses, kernel will lookup EFI_MEMORY_MAP and will fix up page faults dynamically. Please have a look at these patches for more information. https://lists.01.org/pipermail/luv/2017-March/001636.html Recently, we helped a firmware developer debug implementation of QueryCapsuleCapabilities() (a efi_runtime_service()). He said that, his implementation did access EFI_BOOT_SERVICES_CODE/DATA regions when it was called (which is illegal). So, we can assume that there might be some firmware implementations which access efi regions *other than EFI_RUNTIME and kernel passed arguments* even after booting (i.e. from some efi_runtime_service()) and most importantly these accesses will *always* occur only in 1:1 mode (as buggy firmware has knowledge of only physical addresses and not virtual mode). Please let me know if you think my speculation is not right. > > > I think; this should not break any machines because firmware could > > access illegal addresses only in 1:1 mode and not in VA mode because > > that's the only address space firmware has knowledge about. Firmware > > doesn't know about virtual addresses until we pass them through > > SetVirtualAddressMap(). > > > > So Matt, could you please confirm if you had come across any machines > > that did illegal access to EFI regions using virtual addresses? > > I don't think I do have access to such machines, but what would > removing the virtual mappings buy us? The risk of breaking machines > with buggy firmware outweighs any benefit that I can think of. I think we might not break any machines because we will have 1:1 mappings to support buggy firmware and remove VA mappings because they aren't used at all (according to the reasons I mentioned above). > > Am I missing something? The reasons that we might consider to remove these mappings are: 1. They are unnecessary mappings according to UEFI spec and the logic in kernel. In kernel we wanted to reserve EFI_BOOT_SERVICES_CODE/DATA regions *only* until SetVirtualAddressMap() but presently as we have mappings for these regions we are preserving these regions forever. I mean we have two mappings pointing to the same physical addresses, one being 1:1 mapping and the other being VA mapping. So, I think we can remove VA mapping and have 1:1 mappings to support buggy firmware. 2. As we have mappings we now allow firmware/kernel to access these regions (that shouldn't be accessed). This can can be considered as a security issue (maybe highly unlikely but nevertheless). 3. If we remove these mappings we will know about machines that does access EFI_BOOT_SERVICES_CODE/DATA regions even after booting in VA mode. The main reason this patch and the discussion started because of commit 67a9108ed431 ("x86/efi: Build our own page table structures"). In this commit we moved to efi_pgd and hence we didn't have 1:1 mappings for EFI_CONVENTIONAL_MEMORY and EFI_LOADER_DATA/CODE and hence I came across the bug. Hadn't we moved to efi_pgd at all from swapper_pgd I would have never noticed that efi_runtime_services() will do illegal accesses and a firmware bug would go unnoticed. So to conclude, as you said we *might* break some machines (highly unlikely) but at least we will get to know that one more different bug in firmware. Sorry! for making it too long. But working on these bug gave me new insights into efi code and hence have put some here. Regards, Sai ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-20 18:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-15 4:03 [PATCH V2] x86/efi: Add missing 1:1 mappings to support buggy firmware Sai Praneeth Prakhya [not found] ` <1487131421-23703-1-git-send-email-sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 2017-02-28 11:51 ` Matt Fleming [not found] ` <20170228115104.GC28416-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 2017-03-01 1:39 ` Sai Praneeth Prakhya [not found] ` <1488332358.4028.15.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 2017-03-01 3:25 ` Sai Praneeth Prakhya [not found] ` <1488338734.4028.32.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 2017-03-16 11:47 ` Matt Fleming [not found] ` <20170316114722.GD6261-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 2017-03-20 18:52 ` Sai Praneeth Prakhya
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.