[PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Kairui Song]

Currently, kernel fails to boot on some HyperV VMs when using EFI.
And it's a potential issue on all platforms.

It's caused a broken kernel relocation on EFI systems, when below three
conditions are met:

1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
   by the loader.
2. There isn't enough room to contain the kernel, starting from the
   default load address (eg. something else occupied part the region).
3. In the memmap provided by EFI firmware, there is a memory region
   starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
   kernel.

Efi stub will perform a kernel relocation when condition 1 is met. But
due to condition 2, efi stub can't relocate kernel to the preferred
address, so it fallback to query and alloc from EFI firmware for lowest
usable memory region.

It's incorrect to use the lowest memory address. In later stage, kernel
will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
but efi stub will end up relocating kernel below it.

The later kernel decompressing code will forcefully correct the wrong
kernel load location, but it is not aware of the EFI's memory map, and
could over write critical memory, crashing the system.

To fix it, just don't let efi stub relocate the kernel to any address
lower than lowest acceptable address.

Signed-off-by: Kairui Song <kasong@redhat.com>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

---
Update from V2:
 - Update part of the commit message.

Update from V1:
 - Redo the commit message.

 arch/x86/boot/compressed/eboot.c               |  8 +++++---
 drivers/firmware/efi/libstub/arm32-stub.c      |  2 +-
 drivers/firmware/efi/libstub/arm64-stub.c      |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c | 12 ++++++++----
 include/linux/efi.h                            |  5 +++--
 5 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index d6662fdef300..e89e84b66527 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -13,6 +13,7 @@
 #include <asm/e820/types.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
+#include <asm/boot.h>
 
 #include "../string.h"
 #include "eboot.h"
@@ -413,7 +414,7 @@ struct boot_params *make_boot_params(struct efi_config *c)
 	}
 
 	status = efi_low_alloc(sys_table, 0x4000, 1,
-			       (unsigned long *)&boot_params);
+			       (unsigned long *)&boot_params, 0);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to allocate lowmem for boot params\n");
 		return NULL;
@@ -798,7 +799,7 @@ efi_main(struct efi_config *c, struct boot_params *boot_params)
 
 	gdt->size = 0x800;
 	status = efi_low_alloc(sys_table, gdt->size, 8,
-			   (unsigned long *)&gdt->address);
+			       (unsigned long *)&gdt->address, 0);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to allocate memory for 'gdt'\n");
 		goto fail;
@@ -813,7 +814,8 @@ efi_main(struct efi_config *c, struct boot_params *boot_params)
 		status = efi_relocate_kernel(sys_table, &bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
-					     hdr->kernel_alignment);
+					     hdr->kernel_alignment,
+					     LOAD_PHYSICAL_ADDR);
 		if (status != EFI_SUCCESS) {
 			efi_printk(sys_table, "efi_relocate_kernel() failed!\n");
 			goto fail;
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index e8f7aefb6813..bf6f954d6afe 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -220,7 +220,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 	*image_size = image->image_size;
 	status = efi_relocate_kernel(sys_table, image_addr, *image_size,
 				     *image_size,
-				     dram_base + MAX_UNCOMP_KERNEL_SIZE, 0);
+				     dram_base + MAX_UNCOMP_KERNEL_SIZE, 0, 0);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err(sys_table, "Failed to relocate kernel.\n");
 		efi_free(sys_table, *reserve_size, *reserve_addr);
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 1550d244e996..3d2e517e10f4 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -140,7 +140,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
 	if (status != EFI_SUCCESS) {
 		*reserve_size = kernel_memsize + TEXT_OFFSET;
 		status = efi_low_alloc(sys_table_arg, *reserve_size,
-				       MIN_KIMG_ALIGN, reserve_addr);
+				       MIN_KIMG_ALIGN, reserve_addr, 0);
 
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table_arg, "Failed to relocate kernel\n");
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3caae7f2cf56..00b00a2562aa 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -260,11 +260,11 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 }
 
 /*
- * Allocate at the lowest possible address.
+ * Allocate at the lowest possible address that is not below 'min'.
  */
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-			   unsigned long *addr)
+			   unsigned long *addr, unsigned long min)
 {
 	unsigned long map_size, desc_size, buff_size;
 	efi_memory_desc_t *map;
@@ -311,6 +311,9 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 		start = desc->phys_addr;
 		end = start + desc->num_pages * EFI_PAGE_SIZE;
 
+		if (start < min)
+			start = min;
+
 		/*
 		 * Don't allocate at 0x0. It will confuse code that
 		 * checks pointers against NULL. Skip the first 8
@@ -698,7 +701,8 @@ efi_status_t efi_relocate_kernel(efi_system_table_t *sys_table_arg,
 				 unsigned long image_size,
 				 unsigned long alloc_size,
 				 unsigned long preferred_addr,
-				 unsigned long alignment)
+				 unsigned long alignment,
+				 unsigned long min_addr)
 {
 	unsigned long cur_image_addr;
 	unsigned long new_addr = 0;
@@ -732,7 +736,7 @@ efi_status_t efi_relocate_kernel(efi_system_table_t *sys_table_arg,
 	 */
 	if (status != EFI_SUCCESS) {
 		status = efi_low_alloc(sys_table_arg, alloc_size, alignment,
-				       &new_addr);
+				       &new_addr, min_addr);
 	}
 	if (status != EFI_SUCCESS) {
 		pr_efi_err(sys_table_arg, "Failed to allocate usable memory for kernel.\n");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index bd3837022307..a5144cc44e54 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1581,7 +1581,7 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-			   unsigned long *addr);
+			   unsigned long *addr, unsigned long min);
 
 efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 			    unsigned long size, unsigned long align,
@@ -1592,7 +1592,8 @@ efi_status_t efi_relocate_kernel(efi_system_table_t *sys_table_arg,
 				 unsigned long image_size,
 				 unsigned long alloc_size,
 				 unsigned long preferred_addr,
-				 unsigned long alignment);
+				 unsigned long alignment,
+				 unsigned long min_addr);
 
 efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 				  efi_loaded_image_t *image,
-- 
2.21.0

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Borislav Petkov]

On Sat, Oct 12, 2019 at 11:44:21AM +0800, Kairui Song wrote:
> Currently, kernel fails to boot on some HyperV VMs when using EFI.
> And it's a potential issue on all platforms.
> 
> It's caused a broken kernel relocation on EFI systems, when below three
> conditions are met:
> 
> 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
>    by the loader.
> 2. There isn't enough room to contain the kernel, starting from the
>    default load address (eg. something else occupied part the region).
> 3. In the memmap provided by EFI firmware, there is a memory region
>    starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
>    kernel.
> 
> Efi stub will perform a kernel relocation when condition 1 is met. But
> due to condition 2, efi stub can't relocate kernel to the preferred
> address, so it fallback to query and alloc from EFI firmware for lowest

Your spelling of "EFI" is like a random number generator in this
paragraph: "Efi", "efi" and "EFI". Can you please be more careful when
writing your commit messages? They're not some random text you hurriedly
jot down before sending the patch but a most important description of
why a change is being done.

And if you don't see their importance now, just try doing some git
archeology, trying to understand why a change has been done in the past
and then encounter a commit message two-liner which doesn't say sh*t.
Then you'll start appreciating properly written commit messages.

> usable memory region.
> 
> It's incorrect to use the lowest memory address. In later stage, kernel
> will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> but efi stub will end up relocating kernel below it.

Why don't you simply explain what
choose_random_location()->find_random_virt_addr() does? That's the
problem you're solving, right? KASLR using LOAD_PHYSICAL_ADDR as the
minimum...

> The later kernel decompressing code will forcefully correct the wrong
> kernel load location,

... or do you mean by that the dance in
arch/x86/boot/compressed/head_64.S where we move the kernel temporarily
to LOAD_PHYSICAL_ADDR for the decompression?

You can simply say that here...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Jarkko Sakkinen]

On Mon, Oct 14, 2019 at 12:14:19PM +0200, Borislav Petkov wrote:
> Your spelling of "EFI" is like a random number generator in this
> paragraph: "Efi", "efi" and "EFI". Can you please be more careful when
> writing your commit messages? They're not some random text you hurriedly
> jot down before sending the patch but a most important description of
> why a change is being done.

Was there a section in the patch submission documentation to point out
when people send patches with all the possible twists for an acronym?

This is giving me constantly gray hairs with TPM patches.

/Jarkko

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Borislav Petkov]

On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote:
> Was there a section in the patch submission documentation to point out
> when people send patches with all the possible twists for an acronym?

I don't think so.

> This is giving me constantly gray hairs with TPM patches.

Well, I'm slowly getting tired of repeating the same crap over and over
again about how important it is to document one's changes and to write
good commit messages. The most repeated answers I'm simply putting into
canned reply templates because, well, saying it once or twice is not
enough anymore. :-\

And yeah, I see your pain. Same here, actually.

In the acronym case, I'd probably add a regex to my patch massaging
script and convert those typos automatically and be done with it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Kairui Song]

On Mon, Oct 14, 2019 at 6:14 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sat, Oct 12, 2019 at 11:44:21AM +0800, Kairui Song wrote:
> > Currently, kernel fails to boot on some HyperV VMs when using EFI.
> > And it's a potential issue on all platforms.
> >
> > It's caused a broken kernel relocation on EFI systems, when below three
> > conditions are met:
> >
> > 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
> >    by the loader.
> > 2. There isn't enough room to contain the kernel, starting from the
> >    default load address (eg. something else occupied part the region).
> > 3. In the memmap provided by EFI firmware, there is a memory region
> >    starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
> >    kernel.
> >
> > Efi stub will perform a kernel relocation when condition 1 is met. But
> > due to condition 2, efi stub can't relocate kernel to the preferred
> > address, so it fallback to query and alloc from EFI firmware for lowest
>
> Your spelling of "EFI" is like a random number generator in this
> paragraph: "Efi", "efi" and "EFI". Can you please be more careful when
> writing your commit messages? They're not some random text you hurriedly
> jot down before sending the patch but a most important description of
> why a change is being done.

Sorry I just ignored the acronym usage problems, I did double check the text but
didn't realize this is a problem... Will correct them.

>
> And if you don't see their importance now, just try doing some git
> archeology, trying to understand why a change has been done in the past
> and then encounter a commit message two-liner which doesn't say sh*t.
> Then you'll start appreciating properly written commit messages.
>
> > usable memory region.
> >
> > It's incorrect to use the lowest memory address. In later stage, kernel
> > will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> > but efi stub will end up relocating kernel below it.
>
> Why don't you simply explain what
> choose_random_location()->find_random_virt_addr() does? That's the
> problem you're solving, right? KASLR using LOAD_PHYSICAL_ADDR as the
> minimum...
>
> > The later kernel decompressing code will forcefully correct the wrong
> > kernel load location,
>
> ... or do you mean by that the dance in
> arch/x86/boot/compressed/head_64.S where we move the kernel temporarily
> to LOAD_PHYSICAL_ADDR for the decompression?

The kernel move in arch/x86/boot/compressed/head_64.S is the problem
I'm saying here.

I thought it's a bad idea to include too much details about codes and details in
the commit message, so tried to describe it without mentioning the
implementation details.
It's making things confusing indeed.

I'll rethink about how the commit message should be composed...

>
> You can simply say that here...
>

OK, then I'll do so. Will update the commit message.

--
Best Regards,
Kairui Song

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Jarkko Sakkinen]

On Mon, Oct 14, 2019 at 11:18:25PM +0200, Borislav Petkov wrote:
> On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote:
> > Was there a section in the patch submission documentation to point out
> > when people send patches with all the possible twists for an acronym?
> 
> I don't think so.
> 
> > This is giving me constantly gray hairs with TPM patches.
> 
> Well, I'm slowly getting tired of repeating the same crap over and over
> again about how important it is to document one's changes and to write
> good commit messages. The most repeated answers I'm simply putting into
> canned reply templates because, well, saying it once or twice is not
> enough anymore. :-\
> 
> And yeah, I see your pain. Same here, actually.
> 
> In the acronym case, I'd probably add a regex to my patch massaging
> script and convert those typos automatically and be done with it.

Wonder if checkpatch.pl could be extended to know acronyms e.g. have a
db of known acronyms.

/Jarkko

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Joe Perches]

On Wed, 2019-10-16 at 18:20 +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 14, 2019 at 11:18:25PM +0200, Borislav Petkov wrote:
> > On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote:
> > > Was there a section in the patch submission documentation to point out
> > > when people send patches with all the possible twists for an acronym?
> > 
> > I don't think so.
> > 
> > > This is giving me constantly gray hairs with TPM patches.
> > 
> > Well, I'm slowly getting tired of repeating the same crap over and over
> > again about how important it is to document one's changes and to write
> > good commit messages. The most repeated answers I'm simply putting into
> > canned reply templates because, well, saying it once or twice is not
> > enough anymore. :-\
> > 
> > And yeah, I see your pain. Same here, actually.
> > 
> > In the acronym case, I'd probably add a regex to my patch massaging
> > script and convert those typos automatically and be done with it.
> 
> Wonder if checkpatch.pl could be extended to know acronyms e.g. have a
> db of known acronyms.

?  examples please.

checkpatch has a db for misspellings, I supposed another for
acronyms could be added, but how would false positives be avoided?

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Borislav Petkov]

On Wed, Oct 16, 2019 at 08:23:56AM -0700, Joe Perches wrote:
> ?  examples please.

From this very thread:

\sEfi\s, \sefi\s, \seFI\s etc should be "EFI"

I'm thinking perhaps start conservatively and catch the most often
misspelled ones in commit messages or comments. "CPU", "SMT", "MCE",
"MCA", "PCI" etc come to mind.

> checkpatch has a db for misspellings, I supposed another for
> acronyms could be added,

Doesn't have to be another one - established acronyms are part of the
dictionary too.

> but how would false positives be avoided?

Perhaps delimited with spaces or non-word chars (\W) and when they're
part of a comment or the commit message...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Jarkko Sakkinen]

On Wed, Oct 16, 2019 at 08:23:56AM -0700, Joe Perches wrote:
> On Wed, 2019-10-16 at 18:20 +0300, Jarkko Sakkinen wrote: > > On Mon, Oct 14, 2019 at 11:18:25PM +0200, Borislav Petkov wrote:
> > > On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote:
> > > > Was there a section in the patch submission documentation to point out
> > > > when people send patches with all the possible twists for an acronym?
> > > 
> > > I don't think so.
> > > 
> > > > This is giving me constantly gray hairs with TPM patches.
> > > 
> > > Well, I'm slowly getting tired of repeating the same crap over and over
> > > again about how important it is to document one's changes and to write
> > > good commit messages. The most repeated answers I'm simply putting into
> > > canned reply templates because, well, saying it once or twice is not
> > > enough anymore. :-\
> > > 
> > > And yeah, I see your pain. Same here, actually.
> > > 
> > > In the acronym case, I'd probably add a regex to my patch massaging
> > > script and convert those typos automatically and be done with it.
> > 
> > Wonder if checkpatch.pl could be extended to know acronyms e.g. have a
> > db of known acronyms.
> 
> ?  examples please.
> 
> checkpatch has a db for misspellings, I supposed another for
> acronyms could be added, but how would false positives be avoided?

TPM should be always TPM, e.g. not tpm. EFI should be always, e.g.
not efi.

/Jarkko

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Joe Perches]

On Wed, 2019-10-16 at 19:27 +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 08:23:56AM -0700, Joe Perches wrote:
> > On Wed, 2019-10-16 at 18:20 +0300, Jarkko Sakkinen wrote: > > On Mon, Oct 14, 2019 at 11:18:25PM +0200, Borislav Petkov wrote:
> > > > On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote:
> > > > > Was there a section in the patch submission documentation to point out
> > > > > when people send patches with all the possible twists for an acronym?
> > > > 
> > > > I don't think so.
> > > > 
> > > > > This is giving me constantly gray hairs with TPM patches.
> > > > 
> > > > Well, I'm slowly getting tired of repeating the same crap over and over
> > > > again about how important it is to document one's changes and to write
> > > > good commit messages. The most repeated answers I'm simply putting into
> > > > canned reply templates because, well, saying it once or twice is not
> > > > enough anymore. :-\
> > > > 
> > > > And yeah, I see your pain. Same here, actually.
> > > > 
> > > > In the acronym case, I'd probably add a regex to my patch massaging
> > > > script and convert those typos automatically and be done with it.
> > > 
> > > Wonder if checkpatch.pl could be extended to know acronyms e.g. have a
> > > db of known acronyms.
> > 
> > ?  examples please.
> > 
> > checkpatch has a db for misspellings, I supposed another for
> > acronyms could be added, but how would false positives be avoided?
> 
> TPM should be always TPM, e.g. not tpm. EFI should be always, e.g.
> not efi.

I think it's not possible to distinguish between
proper and improper uses.  For instance:

$ git grep -w tpm | wc -l
328
$ git grep -w TPM | wc -l
566

$ git grep -w efi | wc -l
851
$ git grep -w EFI | wc -l
915

Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Joe Perches]

On Wed, 2019-10-16 at 17:48 +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2019 at 08:23:56AM -0700, Joe Perches wrote:
> > ?  examples please.
> 
> From this very thread:
> 
> \sEfi\s, \sefi\s, \seFI\s etc should be "EFI"
> 
> I'm thinking perhaps start conservatively and catch the most often
> misspelled ones in commit messages or comments. "CPU", "SMT", "MCE",
> "MCA", "PCI" etc come to mind.
> 
> > checkpatch has a db for misspellings, I supposed another for
> > acronyms could be added,
> 
> Doesn't have to be another one - established acronyms are part of the
> dictionary too.

Couldn't work.  The dictionary is case insensitive.