RE: [PATCH] x86/kaslr: try process e820 entries if can not get suitable regions from efi

["linfeng (M)" <linfeng23@huawei.com>]

> -----Original Message-----
> From: Arvind Sankar [mailto:niveditas98@gmail.com] On Behalf Of Arvind
> Sankar
> Sent: Wednesday, January 6, 2021 5:01 AM
> To: Ard Biesheuvel <ardb@kernel.org>
> Cc: linfeng (M) <linfeng23@huawei.com>; Arvind Sankar
> <nivedita@alum.mit.edu>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar
> <mingo@redhat.com>; Borislav Petkov <bp@alien8.de>; Huangweidong (C)
> <weidong.huang@huawei.com>; Wangjing (Hogan, Cloud Infrastructure
> Service Product Dept.) <hogan.wang@huawei.com>; Wangxin (Alexander)
> <wangxinxin.wang@huawei.com>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Eric W. Biederman <ebiederm@xmission.com>
> Subject: Re: [PATCH] x86/kaslr: try process e820 entries if can not get suitable
> regions from efi
> 
> On Tue, Jan 05, 2021 at 09:54:52AM +0100, Ard Biesheuvel wrote:
> > (cc Arvind)
> >
> > On Tue, 5 Jan 2021 at 09:54, Lin Feng <linfeng23@huawei.com> wrote:
> > >
> > > On efi64 x86_64 system, the EFI_CONVENTIONAL_MEMORY regions will
> not
> > > be mapped when making EFI runtime calls. So kexec-tools can not get
> > > these from /sys/firmware/efi/runtime-map. Then compressed boot os
> > > can not get suitable regions in process_efi_entries and print debug
> > > message as follow:
> > >         Physical KASLR disabled: no suitable memory region!
> > > To enable physical kaslr with kexec, call process_e820_entries when
> > > no suitable regions in efi memmaps.
> > >
> > > Signed-off-by: Lin Feng <linfeng23@huawei.com>
> > >
> > > ---
> > >
> > > I find a regular of Kernel code and data placement with kexec. It
> > > seems unsafe. The reason is showed above.
> > >
> > > I'm not familiar with efi firmware. I wonder if there are some risks
> > > to get regions according to e820 when there is no suitable region in
> > > efi memmaps.
> > > ---
> > >  arch/x86/boot/compressed/kaslr.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/boot/compressed/kaslr.c
> > > b/arch/x86/boot/compressed/kaslr.c
> > > index b92fffbe761f..dbd7244b71aa 100644
> > > --- a/arch/x86/boot/compressed/kaslr.c
> > > +++ b/arch/x86/boot/compressed/kaslr.c
> > > @@ -685,6 +685,7 @@ process_efi_entries(unsigned long minimum,
> > > unsigned long image_size)  {
> > >         struct efi_info *e = &boot_params->efi_info;
> > >         bool efi_mirror_found = false;
> > > +       bool efi_mem_region_found = false;
> > >         struct mem_vector region;
> > >         efi_memory_desc_t *md;
> > >         unsigned long pmap;
> > > @@ -742,12 +743,13 @@ process_efi_entries(unsigned long minimum,
> unsigned long image_size)
> > >                     !(md->attribute &
> EFI_MEMORY_MORE_RELIABLE))
> > >                         continue;
> > >
> > > +               efi_mem_region_found = false;
> 					   ^^ this should be true, not false.
You're right. It should be true here. Thanks for pointing out.
> 
> Other than that, I think this should be okay. The reason EFI memmap is
> preferred over E820, according to commit
> 
>   0982adc74673 ("x86/boot/KASLR: Work around firmware bugs by excluding
> EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice")
> 
> was to avoid allocating inside EFI_BOOT_SERVICES/EFI_LOADER_DATA etc.
> That's not a danger during kexec, and I believe runtime services regions should
> be marked as reserved in the E820 map, right?
Yes.
> 
> Also, something a little fishy-looking here is that the first loop to see if there is
> any EFI_MEMORY_MORE_RELIABLE region does not apply any of the checks on
> the memory region type/attributes. If there is a mirror region but it isn't
> conventional memory, or if it was soft-reserved, we shouldn't be setting
> efi_mirror_found.
I think so. And I wonder if the memory mirror doesn't work with kexec and ksalr
only this patch used, because a lot of efi information is lost and e820 don't have any
mirror regions information. Due to resource constraints, I haven't tested it yet.
But it seems so.
> 
> 
> > >                 region.start = md->phys_addr;
> > >                 region.size = md->num_pages << EFI_PAGE_SHIFT;
> > >                 if (process_mem_region(&region, minimum,
> image_size))
> > >                         break;
> > >         }
> > > -       return true;
> > > +       return efi_mem_region_found;
> > >  }
> > >  #else
> > >  static inline bool
> > > --
> > > 2.23.0
> > >