On 4 November 2018 at 21:10, Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> wrote: > CC'ing x86 folks because this patch set touches x86/mm which I am no expert of. > > Ideally, after kernel assumes control of the platform, firmware shouldn't access > EFI boot services code/data regions. But, it's noticed that this is not so true > in many x86 platforms. Hence, during boot, kernel reserves EFI boot services > code/data regions [1] and maps [2] them to efi_pgd so that call to > set_virtual_address_map() doesn't fail. After returning from > set_virtual_address_map(), kernel frees the reserved regions [3] but they still > remain mapped. > > This means that any code that's running in efi_pgd address space (e.g: any EFI > runtime service) would still be able to access EFI boot services code/data > regions but the contents of these regions would have long been over written by > someone else as they are freed by efi_free_boot_services(). So, it's important > to unmap these regions. After unmapping EFI boot services code/data regions, any > illegal access by buggy firmware to these regions would result in page fault > which will be handled by efi specific fault handler. > > Unmapping EFI boot services code/data regions will result in clearing > PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already handled > by protnone_mask() at arch/x86/include/asm/pgtable-invert.h. > > [1] Please see efi_reserve_boot_services() > [2] Please see efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd() > [3] Please see efi_free_boot_services() > > Testing the patch set: > ---------------------- > 1. Download buggy firmware (which accesses boot regions even after kernel has > booted) from here [1]. > 2. Without the patch set, you shouldn't see any kernel warning/error messages > (i.e. kernel allows accesses to EFI boot services code/data regions even after > call to set_virtual_address_map()). > 3. With the patch set, you should see a kernel warning about buggy firmware, > efi_rts_wq beeing freezed and disabling runtime services forever. > > Please note that this patch will change kernel's existing behavior for some EFI > runtime services but I think it's OK because kernel should have never allowed > those accesses in the first place. > > Also please note that this patch set needs lot of real time trashing as I just > tested it out with OVMF. > > Note: > ----- > Patch set based on "next" branch in efi tree. > > Changes from V2 -> V3: > ---------------------- > 1. Expliclty set pfn to 0 in kernel_unmap_pages_in_pgd(). > 2. Add __init modifier to kernel_<map/unmap>_pages_in_pgd(). > 3. Warn if kernel_<map/unmap>_pages_in_pgd() are called after smp_init(). > 4. Split efi_unmap_pages() into a separate patch. > > Changes from V1 -> V2: > ---------------------- > 1. Rewrite the cpa initializer in a more readable fashion. > 2. Don't use cpa->pfn while unmapping, as it's not useful. > 3. Unmap regions before freeing them up. > 4. Fix spelling nits. > > Sai Praneeth (3): > x86/mm/pageattr: Introduce helper function to unmap EFI boot services > x86/efi: Unmap EFI boot services code/data regions from efi_pgd > x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86 > > arch/x86/include/asm/efi.h | 2 ++ > arch/x86/include/asm/pgtable_types.h | 8 ++++++-- > arch/x86/mm/pageattr.c | 40 ++++++++++++++++++++++++++++++++++-- > arch/x86/platform/efi/efi.c | 2 ++ > arch/x86/platform/efi/quirks.c | 25 ++++++++++++++++++++++ > include/linux/efi.h | 3 --- > init/main.c | 4 ---- > 7 files changed, 73 insertions(+), 11 deletions(-) > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> > Cc: Borislav Petkov <bp@xxxxxxxxx> > Cc: Ingo Molnar <mingo@xxxxxxxxxx> > Cc: Andy Lutomirski <luto@xxxxxxxxxx> > Cc: Dave Hansen <dave.hansen@xxxxxxxxx> > Cc: Bhupesh Sharma <bhsharma@xxxxxxxxxx> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > Thanks Sai. I have queued these up in efi/next for now, but I'd still like a fresh ack from the x86 maintainers.
