Re: [PATCH 1/2] x86/efi: Unmap efi boot services code/data regions from efi_pgd

[Andy Lutomirski]

On Sun, Oct 21, 2018 at 6:57 PM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> wrote:
>
> > 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 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.
> >
> > [1] Please see efi_reserve_boot_services()
> > [2] Please see efi_map_region() -> __map_region()
> > [3] Please see efi_free_boot_services()
> >
> > 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>
> > ---
> >  arch/x86/include/asm/pgtable_types.h |  2 ++
> >  arch/x86/mm/pageattr.c               | 21 +++++++++++++++++++++
> >  arch/x86/platform/efi/quirks.c       | 26 ++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> > index b64acb08a62b..796476f11151 100644
> > --- a/arch/x86/include/asm/pgtable_types.h
> > +++ b/arch/x86/include/asm/pgtable_types.h
> > @@ -566,6 +566,8 @@ extern pmd_t *lookup_pmd_address(unsigned long address);
> >  extern phys_addr_t slow_virt_to_phys(void *__address);
> >  extern int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
> >                                  unsigned numpages, unsigned long page_flags);
> > +extern int kernel_unmap_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
> > +                                  unsigned long numpages);
> >  #endif       /* !__ASSEMBLY__ */
> >
> >  #endif /* _ASM_X86_PGTABLE_DEFS_H */
> > diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> > index 51a5a69ecac9..b88ed8e91790 100644
> > --- a/arch/x86/mm/pageattr.c
> > +++ b/arch/x86/mm/pageattr.c
> > @@ -2147,6 +2147,27 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
> >       return retval;
> >  }
> >
> > +int kernel_unmap_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
> > +                           unsigned long numpages)
> > +{
> > +     int retval;
> > +
> > +     struct cpa_data cpa = {
> > +             .vaddr = &address,
> > +             .pfn = pfn,
> > +             .pgd = pgd,
> > +             .numpages = numpages,
> > +             .mask_set = __pgprot(0),
> > +             .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > +             .flags = 0,
> > +     };
> > +
> > +     retval = __change_page_attr_set_clr(&cpa, 0);
> > +     __flush_tlb_all();
> > +
> > +     return retval;
> > +}
>
> That's certainly a creative use of __change_page_attr_set_clr() by EFI
> used for mapping in pages so far (kernel_map_pages_in_pgd()), and now
> used for unmapping as well. Doesn't look wrong, just a bit weird as part
> of CPA.
>
> Could you please write the initializer in an easier to read fashion:
>
>         struct cpa_data cpa = {
>                 .vaddr          = &address,
>                 .pfn            = pfn,
>                 .pgd            = pgd,
>                 .numpages       = numpages,
>                 .mask_set       = __pgprot(0),
>                 .mask_clr       = __pgprot(_PAGE_PRESENT | _PAGE_RW),
>                 .flags          = 0,
>         };
>
> ?
>
> The one bit that is odd is the cpa->pfn field - for unmapped pages that's
> totally uninteresting and I'm wondering whether setting it to 0 wouldn't
> be better.
>
> Does the CPU _ever_ look look at the PFN if the page is !_PAGE_PRESENT,
> for example speculatively? If yes then what is the recommended value for
> the pfn - zero perhaps?
>

This is L1TF, right?  Isn't all ones the recommended value?

I would love to see EFI start treating its mm just like any other mm
at some point, though.  That is, it should not modify any mappings in
the kernel range, and it could use the regular mm modification APIs
for the user range.  But maybe this is a pipe dream.