(+ Ingo, Andy, Peter) Hello Sai, I have added some x86/intel folks to cc. I am fine with these patches, and I think it is useful to be able to detect and recover from buggy UEFI implementations that use boot time regions at runtime. However, I need help from the x86 maintainers/developers to review this so please cc them on these patches. Thanks, Ard. On 22 July 2018 at 07:41, Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> wrote: > From: Sai Praneeth <sai.praneeth.prakhya@xxxxxxxxx> > > There may exist some buggy UEFI firmware implementations that access efi > memory regions other than EFI_RUNTIME_SERVICES_<CODE/DATA> even after > kernel has assumed control of the platform. This violates UEFI > specification. Here, we provide a debug config option which when enabled > detects and fixes up/recovers from page faults caused by buggy firmware. > > The above said illegal accesses trigger page fault in ring 0 because > firmware executes at ring 0 and if unhandled it hangs the kernel. We > provide an efi specific page fault handler to: > 1. Avoid panics/hangs caused by buggy firmware. > 2. Shout loud that the firmware is buggy and hence can save ourselves > from being blamed for not a fault of ours. > > Depending on the illegally accessed efi region, the efi page fault > handler handles illegal accesses differently. > 1. If the illegally accessed region is EFI_BOOT_SERVICES_<CODE/DATA>, > the page fault handler fixes it up by mapping the requested region. > 2. If the illegally accessed region is any other efi region (like > EFI_CONVENTIONAL_MEMORY or EFI_LOADER_<CODE/DATA>), the page fault > handler exits firmware context and disables EFI Runtime Services, so > that we will never again call buggy firmware. > > Page faults to efi regions are handled differently because, presently > during kernel boot, EFI_BOOT_SERVICES_<CODE/DATA> regions are reserved > by kernel and hence it's OK to dynamically map these regions in page > fault handler. The same approach cannot be followed for other efi > regions like EFI_CONVENTIONAL_MEMORY and EFI_LOADER_<CODE/DATA> as they > are very huge in size and reserving them could make the kernel > un-bootable. Hence, we take a different approach (exiting firmware > context) while dealing with page faults to these regions. This also > saves us from executing buggy firmware further. > > Exiting firmware context means that on every entry to firmware we save > the kernel context before calling firmware and if the firmware > misbehaves, in the page fault handler, we roll back to the saved kernel > context. Saving kernel context means saving the stack pointer and the > instruction that gets executed when firmware returns. In the page fault > handler we fix up these two things (RIP and RSP) so that when returning > from page fault handler it looks as if firmware has called RET. > > This issue was reported by Al Stone when he saw that reboot via EFI hangs > the machine. Upon debugging, I found that it's efi_reset_system() that's > touching memory regions which it shouldn't. To reproduce the same > behavior, I have hacked OVMF and made efi_reset_system() buggy. > > Testing the patch set: > ---------------------- > 1. Download buggy firmware from here [1]. > 2. Run a qemu instance with this buggy BIOS and boot mainline kernel. > Add reboot=efi to the kernel command line arguments and after the kernel > is up and running, type "reboot". The kernel should hang while rebooting. > 3. With the same setup, boot kernel after applying patches and the > reboot should work fine. Also please notice warning/error messages > printed by kernel. > > Note: > ----- > Patch set based on "next" branch in efi tree. > > [1] https://drive.google.com/open?id=1tkvT7GaVX2zSlzy1HK1T4Tv8cT36GP6R > > Sai Praneeth (8): > x86/efi: Remove __init attribute from memory mapping functions > x86/efi: Permanently save the EFI_MEMORY_MAP passed by firmware > x86/efi: Save kernel context before calling EFI Runtime Services > x86/efi: Add page fault handler to fixup/recover from page faults > caused by firmware > x86/efi: If EFI_WARN_ON_ILLEGAL_ACCESSES is enabled don't call > efi_free_boot_services() > x86/efi: Map EFI_BOOT_SERVICES_<CODE/DATA> regions only when > EFI_WARN_ON_ILLEGAL_ACCESSES is disabled > x86/mm: If in_atomic(), allocate pages without sleeping > x86/efi: Introduce EFI_WARN_ON_ILLEGAL_ACCESSES > > arch/x86/Kconfig | 17 +++ > arch/x86/include/asm/efi.h | 42 ++++++- > arch/x86/mm/fault.c | 9 ++ > arch/x86/mm/pageattr.c | 16 ++- > arch/x86/platform/efi/efi.c | 10 +- > arch/x86/platform/efi/efi_32.c | 2 +- > arch/x86/platform/efi/efi_64.c | 16 ++- > arch/x86/platform/efi/efi_stub_64.S | 101 ++++++++++++++++- > arch/x86/platform/efi/quirks.c | 193 ++++++++++++++++++++++++++++++++ > drivers/firmware/efi/efi.c | 6 +- > drivers/firmware/efi/runtime-wrappers.c | 6 + > include/linux/efi.h | 16 ++- > init/main.c | 3 +- > 13 files changed, 415 insertions(+), 22 deletions(-) > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> > Suggested-by: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> > Based-on-code-from: Ricardo Neri <ricardo.neri@xxxxxxxxx> > Cc: Al Stone <astone@xxxxxxxxxx> > Cc: Lee Chun-Yi <jlee@xxxxxxxx> > Cc: Borislav Petkov <bp@xxxxxxxxx> > Cc: Bhupesh Sharma <bhsharma@xxxxxxxxxx> > Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > -- > 2.7.4 > -- 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
