On Tue, 2017-02-14 at 20:51 +0000, Matt Fleming wrote:
> On Wed, 08 Feb, at 06:39:08PM, Sai Praneeth Prakhya wrote:
> > From: Sai Praneeth <sai.praneeth.prakhya@xxxxxxxxx>
> >
> > 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
>
> Could you include a small amount of the page fault output in the
> commit message? We don't need the callstack, but the IP of the
> faulting instruction and the rest of the page fault message would be
> good (along with which runtime service faulted).
>
Sure, will add them in V2.
> > 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.
>
> Oops, good catch.
>
> > 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.
> >
> > + /*
> > + * 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;
> > }
>
> Is there a reason you didn't add this code to
> efi_sync_low_kernel_mappings()? That would seem like the logical place
> to put it because that's where we already do some PGD copying.
It makes sense that "efi_sync_low_kernel_mappings()" would be the best
place but I added them here for these reasons
1. These mappings refer to "direct mapping of all physical memory" and
thus in "efi_map_regions()" we update *these* mappings to create 1:1
mappings (to allow physical mode accesses by buggy firmware), so if we
copy these mappings in "efi_sync_low_kernel_mappings()" which is called
later than efi_map_regions(), we will overwrite previous 1:1 mappings
created by efi_map_regions() and hence kernel panics.
2. The other reason for adding these mappings in
"efi_alloc_page_tables()" is because that's the way mappings looked in
swapper_pgd before we shifted EFI stuff to efi_pgd.
I have tested this by moving this code to
"efi_sync_low_kernel_mappings()" and I see that kernel panics in
set_virtual_address_map().
Please do correct me if you think otherwise.
Thanks for the review, Matt
Regards,
Sai
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
