Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On 07/26/17 at 09:13am, Baoquan He wrote:
> On 07/26/17 at 12:12am, Naoya Horiguchi wrote:
> > On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> > > On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > > > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > > > so we can clean up the check in efi_reserve_boot_services().
> > > >
> > > > Signed-off-by: Naoya Horiguchi
> > > > ---
> > > > arch/x86/platform/efi/quirks.c | 23 +--
> > > > 1 file changed, 1 insertion(+), 22 deletions(-)
> > >
> > > Is this true for kernels not using KASLR?
> >
> > Thank you for pointing out this. It's not true depending on memmap layout.
> > If a firmware does not define the memory around the kernel address
> > (0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
> > happens. That's true in my testing server, but I don't think that we can
> > expect it generally.
> >
> > So I think of adding some assertion in the patch 1/2 to detect this overlap
> > in extract_kernel() even for no KASLR case.
>
> EFI_BOOT_SERVICES_* memory are collected as e820 region of
> E820_TYPE_RAM, how can we guarantee kernel won't use them after jumping
> into the running kernel whether KASLR enabled or not? We can only wish
> that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from
sorry, typo. I meant EFI boot
service region need be put far from 0x100. Otherwise normal kernel could
allocate memory bottom up and stomp on them. It's embarassment caused by
the hardware flaw of x86 platfrom.
> 0x100 where normal kernel is loaded.
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On 07/26/17 at 09:13am, Baoquan He wrote:
> On 07/26/17 at 12:12am, Naoya Horiguchi wrote:
> > On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> > > On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > > > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > > > so we can clean up the check in efi_reserve_boot_services().
> > > >
> > > > Signed-off-by: Naoya Horiguchi
> > > > ---
> > > > arch/x86/platform/efi/quirks.c | 23 +--
> > > > 1 file changed, 1 insertion(+), 22 deletions(-)
> > >
> > > Is this true for kernels not using KASLR?
> >
> > Thank you for pointing out this. It's not true depending on memmap layout.
> > If a firmware does not define the memory around the kernel address
> > (0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
> > happens. That's true in my testing server, but I don't think that we can
> > expect it generally.
> >
> > So I think of adding some assertion in the patch 1/2 to detect this overlap
> > in extract_kernel() even for no KASLR case.
>
> EFI_BOOT_SERVICES_* memory are collected as e820 region of
> E820_TYPE_RAM, how can we guarantee kernel won't use them after jumping
> into the running kernel whether KASLR enabled or not? We can only wish
> that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from
sorry, typo. I meant EFI boot
service region need be put far from 0x100. Otherwise normal kernel could
allocate memory bottom up and stomp on them. It's embarassment caused by
the hardware flaw of x86 platfrom.
> 0x100 where normal kernel is loaded.
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On 07/26/17 at 12:12am, Naoya Horiguchi wrote:
> On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> > On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > > so we can clean up the check in efi_reserve_boot_services().
> > >
> > > Signed-off-by: Naoya Horiguchi
> > > ---
> > > arch/x86/platform/efi/quirks.c | 23 +--
> > > 1 file changed, 1 insertion(+), 22 deletions(-)
> >
> > Is this true for kernels not using KASLR?
>
> Thank you for pointing out this. It's not true depending on memmap layout.
> If a firmware does not define the memory around the kernel address
> (0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
> happens. That's true in my testing server, but I don't think that we can
> expect it generally.
>
> So I think of adding some assertion in the patch 1/2 to detect this overlap
> in extract_kernel() even for no KASLR case.
EFI_BOOT_SERVICES_* memory are collected as e820 region of
E820_TYPE_RAM, how can we guarantee kernel won't use them after jumping
into the running kernel whether KASLR enabled or not? We can only wish
that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from
0x100 where normal kernel is loaded.
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On 07/26/17 at 12:12am, Naoya Horiguchi wrote:
> On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> > On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > > so we can clean up the check in efi_reserve_boot_services().
> > >
> > > Signed-off-by: Naoya Horiguchi
> > > ---
> > > arch/x86/platform/efi/quirks.c | 23 +--
> > > 1 file changed, 1 insertion(+), 22 deletions(-)
> >
> > Is this true for kernels not using KASLR?
>
> Thank you for pointing out this. It's not true depending on memmap layout.
> If a firmware does not define the memory around the kernel address
> (0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
> happens. That's true in my testing server, but I don't think that we can
> expect it generally.
>
> So I think of adding some assertion in the patch 1/2 to detect this overlap
> in extract_kernel() even for no KASLR case.
EFI_BOOT_SERVICES_* memory are collected as e820 region of
E820_TYPE_RAM, how can we guarantee kernel won't use them after jumping
into the running kernel whether KASLR enabled or not? We can only wish
that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from
0x100 where normal kernel is loaded.
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > so we can clean up the check in efi_reserve_boot_services().
> >
> > Signed-off-by: Naoya Horiguchi
> > ---
> > arch/x86/platform/efi/quirks.c | 23 +--
> > 1 file changed, 1 insertion(+), 22 deletions(-)
>
> Is this true for kernels not using KASLR?
Thank you for pointing out this. It's not true depending on memmap layout.
If a firmware does not define the memory around the kernel address
(0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
happens. That's true in my testing server, but I don't think that we can
expect it generally.
So I think of adding some assertion in the patch 1/2 to detect this overlap
in extract_kernel() even for no KASLR case.
Thanks,
Naoya Horiguchi
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote:
> On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> > so we can clean up the check in efi_reserve_boot_services().
> >
> > Signed-off-by: Naoya Horiguchi
> > ---
> > arch/x86/platform/efi/quirks.c | 23 +--
> > 1 file changed, 1 insertion(+), 22 deletions(-)
>
> Is this true for kernels not using KASLR?
Thank you for pointing out this. It's not true depending on memmap layout.
If a firmware does not define the memory around the kernel address
(0x100 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap
happens. That's true in my testing server, but I don't think that we can
expect it generally.
So I think of adding some assertion in the patch 1/2 to detect this overlap
in extract_kernel() even for no KASLR case.
Thanks,
Naoya Horiguchi
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> so we can clean up the check in efi_reserve_boot_services().
>
> Signed-off-by: Naoya Horiguchi
> ---
> arch/x86/platform/efi/quirks.c | 23 +--
> 1 file changed, 1 insertion(+), 22 deletions(-)
Is this true for kernels not using KASLR?
Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote:
> EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
> so we can clean up the check in efi_reserve_boot_services().
>
> Signed-off-by: Naoya Horiguchi
> ---
> arch/x86/platform/efi/quirks.c | 23 +--
> 1 file changed, 1 insertion(+), 22 deletions(-)
Is this true for kernels not using KASLR?
[PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
so we can clean up the check in efi_reserve_boot_services().
Signed-off-by: Naoya Horiguchi
---
arch/x86/platform/efi/quirks.c | 23 +--
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git next-20170705/arch/x86/platform/efi/quirks.c
next-20170705_patched/arch/x86/platform/efi/quirks.c
index 8a99a2e..191f6f7 100644
--- next-20170705/arch/x86/platform/efi/quirks.c
+++ next-20170705_patched/arch/x86/platform/efi/quirks.c
@@ -292,27 +292,6 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64
size)
efi_memmap_install(new_phys, num_entries);
}
-/*
- * Helper function for efi_reserve_boot_services() to figure out if we
- * can free regions in efi_free_boot_services().
- *
- * Use this function to ensure we do not free regions owned by somebody
- * else. We must only reserve (and then free) regions:
- *
- * - Not within any part of the kernel
- * - Not the BIOS reserved area (E820_TYPE_RESERVED, E820_TYPE_NVS, etc)
- */
-static bool can_free_region(u64 start, u64 size)
-{
- if (start + size > __pa_symbol(_text) && start <= __pa_symbol(_end))
- return false;
-
- if (!e820__mapped_all(start, start+size, E820_TYPE_RAM))
- return false;
-
- return true;
-}
-
void __init efi_reserve_boot_services(void)
{
efi_memory_desc_t *md;
@@ -350,7 +329,7 @@ void __init efi_reserve_boot_services(void)
* one else cares about it. We own it and can
* free it later.
*/
- if (can_free_region(start, size))
+ if (e820__mapped_all(start, start+size, E820_TYPE_RAM))
continue;
}
--
2.7.0
[PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()
EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now,
so we can clean up the check in efi_reserve_boot_services().
Signed-off-by: Naoya Horiguchi
---
arch/x86/platform/efi/quirks.c | 23 +--
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git next-20170705/arch/x86/platform/efi/quirks.c
next-20170705_patched/arch/x86/platform/efi/quirks.c
index 8a99a2e..191f6f7 100644
--- next-20170705/arch/x86/platform/efi/quirks.c
+++ next-20170705_patched/arch/x86/platform/efi/quirks.c
@@ -292,27 +292,6 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64
size)
efi_memmap_install(new_phys, num_entries);
}
-/*
- * Helper function for efi_reserve_boot_services() to figure out if we
- * can free regions in efi_free_boot_services().
- *
- * Use this function to ensure we do not free regions owned by somebody
- * else. We must only reserve (and then free) regions:
- *
- * - Not within any part of the kernel
- * - Not the BIOS reserved area (E820_TYPE_RESERVED, E820_TYPE_NVS, etc)
- */
-static bool can_free_region(u64 start, u64 size)
-{
- if (start + size > __pa_symbol(_text) && start <= __pa_symbol(_end))
- return false;
-
- if (!e820__mapped_all(start, start+size, E820_TYPE_RAM))
- return false;
-
- return true;
-}
-
void __init efi_reserve_boot_services(void)
{
efi_memory_desc_t *md;
@@ -350,7 +329,7 @@ void __init efi_reserve_boot_services(void)
* one else cares about it. We own it and can
* free it later.
*/
- if (can_free_region(start, size))
+ if (e820__mapped_all(start, start+size, E820_TYPE_RAM))
continue;
}
--
2.7.0
