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

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

[Lin Feng]

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;
 		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

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

[Ard Biesheuvel]

(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;
>                 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
>

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

[Arvind Sankar]

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.

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?

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.


> >                 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
> >

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

[linfeng (M)]

> -----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
> > >

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

[linfeng (M)]

On Wed, 17 Mar 09:54, Lin Feng <linfeng23@huawei.com> wrote:
After more than one month testing, we find that it is not suitable to process
e820 directly in kexec to place the kernel code. Some regions, like tmplog
and memattr tables, are not marked as reserved in e820.
Take tmplog, for example, the memory of table is marked as E820_TYPE_RAM
in e820 and EFI_LOADER_DATA in efi. I wonder why not marked it as
E820_TYPE_RESERVED, which is contrary to our expectations. So processing
e820 directly in kexec is against the principles of the commit
    0982adc74673 ("x86/boot/KASLR: Work around firmware bugs by excluding
We try to avoid placing kernel code or data on tmplog memory range in
kexec. But unfortunately, the efi info is not runnable, so it is abandoned in function
efi_map_regions. We can not get the info in kexec.
Any way, we skill haven't found a suitable solution. Any ideas, friends?
> On Wed, 6 Jan 2021 03:04, Lin Feng <linfeng23@huawei.com> wrote:
> >
> > 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
> > > >