diff mbox series

[v4,08/26] x86/boot: Map memory explicitly

Message ID 760c19466ac26c09edb76e64d8c4812ff4aa7365.1671098103.git.baskov@ispras.ru
State Superseded
Headers show
Series x86_64: Improvements at compressed kernel stage | expand

Commit Message

Evgeniy Baskov Dec. 15, 2022, 12:37 p.m. UTC 
Implicit mappings hide possible memory errors, e.g. allocations for
ACPI tables were not included in boot page table size.

Replace all implicit mappings from page fault handler with
explicit mappings.

Tested-by: Mario Limonciello <mario.limonciello@amd.com>
Tested-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/boot/compressed/acpi.c  | 25 ++++++++++++++++++++++++-
 arch/x86/boot/compressed/efi.c   | 19 ++++++++++++++++++-
 arch/x86/boot/compressed/kaslr.c |  4 ++++
 3 files changed, 46 insertions(+), 2 deletions(-)

Comments

Ard Biesheuvel March 8, 2023, 9:38 a.m. UTC | #1 
On Thu, 15 Dec 2022 at 13:38, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> Implicit mappings hide possible memory errors, e.g. allocations for
> ACPI tables were not included in boot page table size.
>
> Replace all implicit mappings from page fault handler with
> explicit mappings.
>

I agree with the motivation but this patch seems to break the boot
under SeaBIOS/QEMU, and I imagine other legacy BIOS boot scenarios as
well.

Naively, I would assume that there is simply a legacy BIOS region that
we fail to map here, but I am fairly clueless when it comes to non-EFI
x86 boot so take this with a grain of salt.


> Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> Tested-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> ---
>  arch/x86/boot/compressed/acpi.c  | 25 ++++++++++++++++++++++++-
>  arch/x86/boot/compressed/efi.c   | 19 ++++++++++++++++++-
>  arch/x86/boot/compressed/kaslr.c |  4 ++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 9caf89063e77..c775e01fc7db 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -93,6 +93,8 @@ static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
>
>         end = start + length;
>
> +       kernel_add_identity_map((unsigned long)start, (unsigned long)end, 0);
> +
>         /* Search from given start address for the requested length */
>         for (address = start; address < end; address += ACPI_RSDP_SCAN_STEP) {
>                 /*
> @@ -128,6 +130,9 @@ static acpi_physical_address bios_get_rsdp_addr(void)
>         unsigned long address;
>         u8 *rsdp;
>
> +       kernel_add_identity_map((unsigned long)ACPI_EBDA_PTR_LOCATION,
> +                               (unsigned long)ACPI_EBDA_PTR_LOCATION + 2, 0);
> +
>         /* Get the location of the Extended BIOS Data Area (EBDA) */
>         address = *(u16 *)ACPI_EBDA_PTR_LOCATION;
>         address <<= 4;
> @@ -215,6 +220,9 @@ static unsigned long get_acpi_srat_table(void)
>         if (!rsdp)
>                 return 0;
>
> +       kernel_add_identity_map((unsigned long)rsdp,
> +                               (unsigned long)(rsdp + 1), 0);
> +
>         /* Get ACPI root table from RSDP.*/
>         if (!(cmdline_find_option("acpi", arg, sizeof(arg)) == 4 &&
>             !strncmp(arg, "rsdt", 4)) &&
> @@ -231,10 +239,17 @@ static unsigned long get_acpi_srat_table(void)
>                 return 0;
>
>         header = (struct acpi_table_header *)root_table;
> +
> +       kernel_add_identity_map((unsigned long)header,
> +                               (unsigned long)(header + 1), 0);
> +
>         len = header->length;
>         if (len < sizeof(struct acpi_table_header) + size)
>                 return 0;
>
> +       kernel_add_identity_map((unsigned long)header,
> +                               (unsigned long)header + len, 0);
> +
>         num_entries = (len - sizeof(struct acpi_table_header)) / size;
>         entry = (u8 *)(root_table + sizeof(struct acpi_table_header));
>
> @@ -247,8 +262,16 @@ static unsigned long get_acpi_srat_table(void)
>                 if (acpi_table) {
>                         header = (struct acpi_table_header *)acpi_table;
>
> -                       if (ACPI_COMPARE_NAMESEG(header->signature, ACPI_SIG_SRAT))
> +                       kernel_add_identity_map(acpi_table,
> +                                               acpi_table + sizeof(*header),
> +                                               0);
> +
> +                       if (ACPI_COMPARE_NAMESEG(header->signature, ACPI_SIG_SRAT)) {
> +                               kernel_add_identity_map(acpi_table,
> +                                                       acpi_table + header->length,
> +                                                       0);
>                                 return acpi_table;
> +                       }
>                 }
>                 entry += size;
>         }
> diff --git a/arch/x86/boot/compressed/efi.c b/arch/x86/boot/compressed/efi.c
> index 6edd034b0b30..ce70103fbbc0 100644
> --- a/arch/x86/boot/compressed/efi.c
> +++ b/arch/x86/boot/compressed/efi.c
> @@ -57,10 +57,14 @@ enum efi_type efi_get_type(struct boot_params *bp)
>   */
>  unsigned long efi_get_system_table(struct boot_params *bp)
>  {
> -       unsigned long sys_tbl_pa;
> +       static unsigned long sys_tbl_pa __section(".data");
>         struct efi_info *ei;
> +       unsigned long sys_tbl_size;
>         enum efi_type et;
>
> +       if (sys_tbl_pa)
> +               return sys_tbl_pa;
> +
>         /* Get systab from boot params. */
>         ei = &bp->efi_info;
>  #ifdef CONFIG_X86_64
> @@ -73,6 +77,13 @@ unsigned long efi_get_system_table(struct boot_params *bp)
>                 return 0;
>         }
>
> +       if (efi_get_type(bp) == EFI_TYPE_64)
> +               sys_tbl_size = sizeof(efi_system_table_64_t);
> +       else
> +               sys_tbl_size = sizeof(efi_system_table_32_t);
> +
> +       kernel_add_identity_map(sys_tbl_pa, sys_tbl_pa + sys_tbl_size, 0);
> +
>         return sys_tbl_pa;
>  }
>
> @@ -92,6 +103,10 @@ static struct efi_setup_data *get_kexec_setup_data(struct boot_params *bp,
>
>         pa_data = bp->hdr.setup_data;
>         while (pa_data) {
> +               unsigned long pa_data_end = pa_data + sizeof(struct setup_data)
> +                                         + sizeof(struct efi_setup_data);
> +               kernel_add_identity_map(pa_data, pa_data_end, 0);
> +
>                 data = (struct setup_data *)pa_data;
>                 if (data->type == SETUP_EFI) {
>                         esd = (struct efi_setup_data *)(pa_data + sizeof(struct setup_data));
> @@ -160,6 +175,8 @@ int efi_get_conf_table(struct boot_params *bp, unsigned long *cfg_tbl_pa,
>                 return -EINVAL;
>         }
>
> +       kernel_add_identity_map(*cfg_tbl_pa, *cfg_tbl_pa + *cfg_tbl_len, 0);
> +
>         return 0;
>  }
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 454757fbdfe5..c0ee116c4fa2 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -688,6 +688,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>         u32 nr_desc;
>         int i;
>
> +       kernel_add_identity_map((unsigned long)e, (unsigned long)(e + 1), 0);
> +
>         signature = (char *)&e->efi_loader_signature;
>         if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>             strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> @@ -704,6 +706,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
>         pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
>  #endif
>
> +       kernel_add_identity_map(pmap, pmap + e->efi_memmap_size, 0);
> +
>         nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>         for (i = 0; i < nr_desc; i++) {
>                 md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);
> --
> 2.37.4
>
Ard Biesheuvel March 8, 2023, 10:28 a.m. UTC | #2 
On Wed, 8 Mar 2023 at 10:38, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 15 Dec 2022 at 13:38, Evgeniy Baskov <baskov@ispras.ru> wrote:
> >
> > Implicit mappings hide possible memory errors, e.g. allocations for
> > ACPI tables were not included in boot page table size.
> >
> > Replace all implicit mappings from page fault handler with
> > explicit mappings.
> >
>
> I agree with the motivation but this patch seems to break the boot
> under SeaBIOS/QEMU, and I imagine other legacy BIOS boot scenarios as
> well.
>
> Naively, I would assume that there is simply a legacy BIOS region that
> we fail to map here, but I am fairly clueless when it comes to non-EFI
> x86 boot so take this with a grain of salt.
>

The below seems to help - not sure why exactly, but apparently legacy
BIOS needs the bootparams struct to be mapped writable?

--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -31,6 +31,7 @@
 #include <linux/ctype.h>
 #include <generated/utsversion.h>
 #include <generated/utsrelease.h>
+#include <asm/shared/pgtable.h>

 #define _SETUP
 #include <asm/setup.h> /* For COMMAND_LINE_SIZE */
@@ -688,7 +689,7 @@ process_efi_entries(unsigned long minimum,
unsigned long image_size)
        u32 nr_desc;
        int i;

-       kernel_add_identity_map((unsigned long)e, (unsigned long)(e + 1), 0);
+       kernel_add_identity_map((unsigned long)e, (unsigned long)(e +
1), MAP_WRITE);

        signature = (char *)&e->efi_loader_signature;
        if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
Evgeniy Baskov March 8, 2023, 4:09 p.m. UTC | #3 
On 2023-03-08 13:28, Ard Biesheuvel wrote:
> On Wed, 8 Mar 2023 at 10:38, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 
>> On Thu, 15 Dec 2022 at 13:38, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> >
>> > Implicit mappings hide possible memory errors, e.g. allocations for
>> > ACPI tables were not included in boot page table size.
>> >
>> > Replace all implicit mappings from page fault handler with
>> > explicit mappings.
>> >
>> 
>> I agree with the motivation but this patch seems to break the boot
>> under SeaBIOS/QEMU, and I imagine other legacy BIOS boot scenarios as
>> well.
>> 
>> Naively, I would assume that there is simply a legacy BIOS region that
>> we fail to map here, but I am fairly clueless when it comes to non-EFI
>> x86 boot so take this with a grain of salt.
>> 
> 
> The below seems to help - not sure why exactly, but apparently legacy
> BIOS needs the bootparams struct to be mapped writable?

I think I got too eager adding mappings to everything.
In the process_efi_entries() bootparams should already be mapped, so
I will just remove the call. And AFAIK bootparams is indeed gets
written to.

> 
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -31,6 +31,7 @@
>  #include <linux/ctype.h>
>  #include <generated/utsversion.h>
>  #include <generated/utsrelease.h>
> +#include <asm/shared/pgtable.h>
> 
>  #define _SETUP
>  #include <asm/setup.h> /* For COMMAND_LINE_SIZE */
> @@ -688,7 +689,7 @@ process_efi_entries(unsigned long minimum,
> unsigned long image_size)
>         u32 nr_desc;
>         int i;
> 
> -       kernel_add_identity_map((unsigned long)e, (unsigned long)(e + 
> 1), 0);
> +       kernel_add_identity_map((unsigned long)e, (unsigned long)(e +
> 1), MAP_WRITE);
> 
>         signature = (char *)&e->efi_loader_signature;
>         if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&

Thanks,
Evgeniy Baskov
diff mbox series

Patch

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 9caf89063e77..c775e01fc7db 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -93,6 +93,8 @@  static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
 
 	end = start + length;
 
+	kernel_add_identity_map((unsigned long)start, (unsigned long)end, 0);
+
 	/* Search from given start address for the requested length */
 	for (address = start; address < end; address += ACPI_RSDP_SCAN_STEP) {
 		/*
@@ -128,6 +130,9 @@  static acpi_physical_address bios_get_rsdp_addr(void)
 	unsigned long address;
 	u8 *rsdp;
 
+	kernel_add_identity_map((unsigned long)ACPI_EBDA_PTR_LOCATION,
+				(unsigned long)ACPI_EBDA_PTR_LOCATION + 2, 0);
+
 	/* Get the location of the Extended BIOS Data Area (EBDA) */
 	address = *(u16 *)ACPI_EBDA_PTR_LOCATION;
 	address <<= 4;
@@ -215,6 +220,9 @@  static unsigned long get_acpi_srat_table(void)
 	if (!rsdp)
 		return 0;
 
+	kernel_add_identity_map((unsigned long)rsdp,
+				(unsigned long)(rsdp + 1), 0);
+
 	/* Get ACPI root table from RSDP.*/
 	if (!(cmdline_find_option("acpi", arg, sizeof(arg)) == 4 &&
 	    !strncmp(arg, "rsdt", 4)) &&
@@ -231,10 +239,17 @@  static unsigned long get_acpi_srat_table(void)
 		return 0;
 
 	header = (struct acpi_table_header *)root_table;
+
+	kernel_add_identity_map((unsigned long)header,
+				(unsigned long)(header + 1), 0);
+
 	len = header->length;
 	if (len < sizeof(struct acpi_table_header) + size)
 		return 0;
 
+	kernel_add_identity_map((unsigned long)header,
+				(unsigned long)header + len, 0);
+
 	num_entries = (len - sizeof(struct acpi_table_header)) / size;
 	entry = (u8 *)(root_table + sizeof(struct acpi_table_header));
 
@@ -247,8 +262,16 @@  static unsigned long get_acpi_srat_table(void)
 		if (acpi_table) {
 			header = (struct acpi_table_header *)acpi_table;
 
-			if (ACPI_COMPARE_NAMESEG(header->signature, ACPI_SIG_SRAT))
+			kernel_add_identity_map(acpi_table,
+						acpi_table + sizeof(*header),
+						0);
+
+			if (ACPI_COMPARE_NAMESEG(header->signature, ACPI_SIG_SRAT)) {
+				kernel_add_identity_map(acpi_table,
+							acpi_table + header->length,
+							0);
 				return acpi_table;
+			}
 		}
 		entry += size;
 	}
diff --git a/arch/x86/boot/compressed/efi.c b/arch/x86/boot/compressed/efi.c
index 6edd034b0b30..ce70103fbbc0 100644
--- a/arch/x86/boot/compressed/efi.c
+++ b/arch/x86/boot/compressed/efi.c
@@ -57,10 +57,14 @@  enum efi_type efi_get_type(struct boot_params *bp)
  */
 unsigned long efi_get_system_table(struct boot_params *bp)
 {
-	unsigned long sys_tbl_pa;
+	static unsigned long sys_tbl_pa __section(".data");
 	struct efi_info *ei;
+	unsigned long sys_tbl_size;
 	enum efi_type et;
 
+	if (sys_tbl_pa)
+		return sys_tbl_pa;
+
 	/* Get systab from boot params. */
 	ei = &bp->efi_info;
 #ifdef CONFIG_X86_64
@@ -73,6 +77,13 @@  unsigned long efi_get_system_table(struct boot_params *bp)
 		return 0;
 	}
 
+	if (efi_get_type(bp) == EFI_TYPE_64)
+		sys_tbl_size = sizeof(efi_system_table_64_t);
+	else
+		sys_tbl_size = sizeof(efi_system_table_32_t);
+
+	kernel_add_identity_map(sys_tbl_pa, sys_tbl_pa + sys_tbl_size, 0);
+
 	return sys_tbl_pa;
 }
 
@@ -92,6 +103,10 @@  static struct efi_setup_data *get_kexec_setup_data(struct boot_params *bp,
 
 	pa_data = bp->hdr.setup_data;
 	while (pa_data) {
+		unsigned long pa_data_end = pa_data + sizeof(struct setup_data)
+					  + sizeof(struct efi_setup_data);
+		kernel_add_identity_map(pa_data, pa_data_end, 0);
+
 		data = (struct setup_data *)pa_data;
 		if (data->type == SETUP_EFI) {
 			esd = (struct efi_setup_data *)(pa_data + sizeof(struct setup_data));
@@ -160,6 +175,8 @@  int efi_get_conf_table(struct boot_params *bp, unsigned long *cfg_tbl_pa,
 		return -EINVAL;
 	}
 
+	kernel_add_identity_map(*cfg_tbl_pa, *cfg_tbl_pa + *cfg_tbl_len, 0);
+
 	return 0;
 }
 
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 454757fbdfe5..c0ee116c4fa2 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -688,6 +688,8 @@  process_efi_entries(unsigned long minimum, unsigned long image_size)
 	u32 nr_desc;
 	int i;
 
+	kernel_add_identity_map((unsigned long)e, (unsigned long)(e + 1), 0);
+
 	signature = (char *)&e->efi_loader_signature;
 	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
 	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
@@ -704,6 +706,8 @@  process_efi_entries(unsigned long minimum, unsigned long image_size)
 	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
 #endif
 
+	kernel_add_identity_map(pmap, pmap + e->efi_memmap_size, 0);
+
 	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
 	for (i = 0; i < nr_desc; i++) {
 		md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i);