diff mbox series

[v2,1/2] x86: Check return values from early_memremap calls

Message ID 20221110154521.613472-2-ross.philipson@oracle.com
State New
Headers
Series x86: Check return values for early memory/IO remap calls |

Commit Message

Ross Philipson Nov. 10, 2022, 3:45 p.m. UTC 
  There are a number of places where early_memremap is called
but the return pointer is not checked for NULL. The call
can result in a NULL being returned so the checks must
be added.

Note that the maintainers for both the Jailhouse and Xen code
approved of using panic() to handle allocation failures.

In addition to checking the return values, a bit of extra
cleanup of pr_* usages was done since the pr_fmt macro was
introduced in the modules touched by this patch.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 arch/x86/kernel/devicetree.c | 13 +++++++++++
 arch/x86/kernel/e820.c       | 12 +++++++++--
 arch/x86/kernel/jailhouse.c  |  6 ++++++
 arch/x86/kernel/mpparse.c    | 51 +++++++++++++++++++++++++++++++++-----------
 arch/x86/kernel/setup.c      | 19 ++++++++++++++---
 arch/x86/xen/enlighten_hvm.c |  2 ++
 arch/x86/xen/mmu_pv.c        |  8 +++++++
 arch/x86/xen/setup.c         |  2 ++
 8 files changed, 95 insertions(+), 18 deletions(-)

Comments

Juergen Gross Nov. 10, 2022, 4:07 p.m. UTC | #1 
On 10.11.22 16:45, Ross Philipson wrote:
> There are a number of places where early_memremap is called
> but the return pointer is not checked for NULL. The call
> can result in a NULL being returned so the checks must
> be added.
> 
> Note that the maintainers for both the Jailhouse and Xen code
> approved of using panic() to handle allocation failures.
> 
> In addition to checking the return values, a bit of extra
> cleanup of pr_* usages was done since the pr_fmt macro was
> introduced in the modules touched by this patch.
> 
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> ---
>   arch/x86/kernel/devicetree.c | 13 +++++++++++
>   arch/x86/kernel/e820.c       | 12 +++++++++--
>   arch/x86/kernel/jailhouse.c  |  6 ++++++
>   arch/x86/kernel/mpparse.c    | 51 +++++++++++++++++++++++++++++++++-----------
>   arch/x86/kernel/setup.c      | 19 ++++++++++++++---
>   arch/x86/xen/enlighten_hvm.c |  2 ++
>   arch/x86/xen/mmu_pv.c        |  8 +++++++
>   arch/x86/xen/setup.c         |  2 ++
>   8 files changed, 95 insertions(+), 18 deletions(-)

For the Xen parts:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Dave Hansen Nov. 10, 2022, 4:07 p.m. UTC | #2 
On 11/10/22 07:45, Ross Philipson wrote:
>  	dt = early_memremap(initial_dtb, map_len);
> +	if (!dt) {
> +		pr_warn("failed to memremap initial dtb\n");
> +		return;
> +	}

Are all of these new pr_warn/err()'s really adding much value?  They all
look pretty generic.  It makes me wonder if we should just spit out a
generic message in early_memremap() and save all the callers the trouble.

Oh, and don't we try to refer to functions() with parenthesis?
Ross Philipson Nov. 10, 2022, 7:30 p.m. UTC | #3 
On 11/10/22 11:07, Dave Hansen wrote:
> On 11/10/22 07:45, Ross Philipson wrote:
>>   	dt = early_memremap(initial_dtb, map_len);
>> +	if (!dt) {
>> +		pr_warn("failed to memremap initial dtb\n");
>> +		return;
>> +	}
> 
> Are all of these new pr_warn/err()'s really adding much value?  They all
> look pretty generic.  It makes me wonder if we should just spit out a
> generic message in early_memremap() and save all the callers the trouble.

These changes were prompted by some comments on an earlier patch set I 
sent. It was requested that I fix the other missing checks for NULL 
returns from these functions but I thought that was out of scope for 
that patch set. So I agreed to submit this set and add the checks making 
things consistent.

> 
> Oh, and don't we try to refer to functions() with parenthesis?

Yes I can fix that.

Thanks
Ross

>
Borislav Petkov Jan. 4, 2023, 11:58 a.m. UTC | #4 
On Thu, Nov 10, 2022 at 08:07:53AM -0800, Dave Hansen wrote:
> On 11/10/22 07:45, Ross Philipson wrote:
> >  	dt = early_memremap(initial_dtb, map_len);
> > +	if (!dt) {
> > +		pr_warn("failed to memremap initial dtb\n");
> > +		return;
> > +	}
> 
> Are all of these new pr_warn/err()'s really adding much value?  They all
> look pretty generic.  It makes me wonder if we should just spit out a
> generic message in early_memremap() and save all the callers the trouble.

Well, let's see.

early_memremap() calls __early_ioremap() and that one already warns befofe each
NULL return. So I guess we don't need the error messages as we will know where
it starts failing.

I guess we still need the error handling though.

I.e., this above should be:

    dt = early_memremap(initial_dtb, map_len);
+   if (!dt)
+           return;

so that we don't go off into the weeds with a NULL ptr.

Or?
diff mbox series

Patch

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f2..4a5ca9a 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -2,6 +2,9 @@ 
 /*
  * Architecture specific OF callbacks.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
@@ -292,10 +295,20 @@  static void __init x86_flattree_get_config(void)
 	map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
 
 	dt = early_memremap(initial_dtb, map_len);
+	if (!dt) {
+		pr_warn("failed to memremap initial dtb\n");
+		return;
+	}
+
 	size = fdt_totalsize(dt);
 	if (map_len < size) {
 		early_memunmap(dt, map_len);
 		dt = early_memremap(initial_dtb, size);
+		if (!dt) {
+			pr_warn("failed to memremap initial dtb\n");
+			return;
+		}
+
 		map_len = size;
 	}
 
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 9dac246..9cbc724 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -9,6 +9,9 @@ 
  * quirks and other tweaks, and feeds that into the generic Linux memory
  * allocation code routines via a platform independent interface (memblock, etc.).
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/crash_dump.h>
 #include <linux/memblock.h>
 #include <linux/suspend.h>
@@ -728,6 +731,11 @@  void __init e820__memory_setup_extended(u64 phys_addr, u32 data_len)
 	struct setup_data *sdata;
 
 	sdata = early_memremap(phys_addr, data_len);
+	if (!sdata) {
+		pr_warn("failed to memremap extended\n");
+		return;
+	}
+
 	entries = sdata->len / sizeof(*extmap);
 	extmap = (struct boot_e820_entry *)(sdata->data);
 
@@ -1007,7 +1015,7 @@  void __init e820__reserve_setup_data(void)
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
 		if (!data) {
-			pr_warn("e820: failed to memremap setup_data entry\n");
+			pr_warn("failed to memremap setup_data entry\n");
 			return;
 		}
 
@@ -1030,7 +1038,7 @@  void __init e820__reserve_setup_data(void)
 			early_memunmap(data, sizeof(*data));
 			data = early_memremap(pa_data, len);
 			if (!data) {
-				pr_warn("e820: failed to memremap indirect setup_data\n");
+				pr_warn("failed to memremap indirect setup_data\n");
 				return;
 			}
 
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 4eb8f2d..80db0c2 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -221,6 +221,9 @@  static void __init jailhouse_init_platform(void)
 
 	while (pa_data) {
 		mapping = early_memremap(pa_data, sizeof(header));
+		if (!mapping)
+			panic("Jailhouse: failed to memremap setup_data header\n");
+
 		memcpy(&header, mapping, sizeof(header));
 		early_memunmap(mapping, sizeof(header));
 
@@ -241,6 +244,9 @@  static void __init jailhouse_init_platform(void)
 	setup_data_len = min_t(unsigned long, sizeof(setup_data),
 			       (unsigned long)header.len);
 	mapping = early_memremap(pa_data, setup_data_len);
+	if (!mapping)
+		panic("Jailhouse: failed to memremap setup_data\n");
+
 	memcpy(&setup_data, mapping, setup_data_len);
 	early_memunmap(mapping, setup_data_len);
 
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fed721f..4254163 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -8,6 +8,8 @@ 
  *      (c) 2008 Alexey Starikovskiy <astarikovskiy@suse.de>
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/delay.h>
@@ -145,33 +147,33 @@  static int __init smp_check_mpc(struct mpc_table *mpc, char *oem, char *str)
 {
 
 	if (memcmp(mpc->signature, MPC_SIGNATURE, 4)) {
-		pr_err("MPTABLE: bad signature [%c%c%c%c]!\n",
+		pr_err("bad signature [%c%c%c%c]!\n",
 		       mpc->signature[0], mpc->signature[1],
 		       mpc->signature[2], mpc->signature[3]);
 		return 0;
 	}
 	if (mpf_checksum((unsigned char *)mpc, mpc->length)) {
-		pr_err("MPTABLE: checksum error!\n");
+		pr_err("checksum error!\n");
 		return 0;
 	}
 	if (mpc->spec != 0x01 && mpc->spec != 0x04) {
-		pr_err("MPTABLE: bad table version (%d)!!\n", mpc->spec);
+		pr_err("bad table version (%d)!!\n", mpc->spec);
 		return 0;
 	}
 	if (!mpc->lapic) {
-		pr_err("MPTABLE: null local APIC address!\n");
+		pr_err("null local APIC address!\n");
 		return 0;
 	}
 	memcpy(oem, mpc->oem, 8);
 	oem[8] = 0;
-	pr_info("MPTABLE: OEM ID: %s\n", oem);
+	pr_info("OEM ID: %s\n", oem);
 
 	memcpy(str, mpc->productid, 12);
 	str[12] = 0;
 
-	pr_info("MPTABLE: Product ID: %s\n", str);
+	pr_info("Product ID: %s\n", str);
 
-	pr_info("MPTABLE: APIC at: 0x%X\n", mpc->lapic);
+	pr_info("APIC at: 0x%X\n", mpc->lapic);
 
 	return 1;
 }
@@ -242,7 +244,7 @@  static int __init smp_read_mpc(struct mpc_table *mpc, unsigned early)
 	}
 
 	if (!num_processors)
-		pr_err("MPTABLE: no processors registered!\n");
+		pr_err("no processors registered!\n");
 	return num_processors;
 }
 
@@ -424,6 +426,9 @@  static unsigned long __init get_mpc_size(unsigned long physptr)
 	unsigned long size;
 
 	mpc = early_memremap(physptr, PAGE_SIZE);
+	if (!mpc)
+		return 0;
+
 	size = mpc->length;
 	early_memunmap(mpc, PAGE_SIZE);
 	apic_printk(APIC_VERBOSE, "  mpc: %lx-%lx\n", physptr, physptr + size);
@@ -437,7 +442,16 @@  static int __init check_physptr(struct mpf_intel *mpf, unsigned int early)
 	unsigned long size;
 
 	size = get_mpc_size(mpf->physptr);
+	if (!size) {
+		pr_err("error getting MP table size\n");
+		return -1;
+	}
+
 	mpc = early_memremap(mpf->physptr, size);
+	if (!mpc) {
+		pr_err("error mapping MP table physptr\n");
+		return -1;
+	}
 
 	/*
 	 * Read the physical hardware table.  Anything here will
@@ -505,7 +519,7 @@  void __init default_get_smp_config(unsigned int early)
 
 	mpf = early_memremap(mpf_base, sizeof(*mpf));
 	if (!mpf) {
-		pr_err("MPTABLE: error mapping MP table\n");
+		pr_err("error mapping MP table\n");
 		return;
 	}
 
@@ -552,6 +566,7 @@  void __init default_get_smp_config(unsigned int early)
 
 static void __init smp_reserve_memory(struct mpf_intel *mpf)
 {
+	/* If get_mpc_size() is 0, memblock_reserve() will just do nothing */
 	memblock_reserve(mpf->physptr, get_mpc_size(mpf->physptr));
 }
 
@@ -567,6 +582,11 @@  static int __init smp_scan_config(unsigned long base, unsigned long length)
 
 	while (length > 0) {
 		bp = early_memremap(base, length);
+		if (!bp) {
+			pr_err("error mapping SMP config\n");
+			return 0;
+		}
+
 		mpf = (struct mpf_intel *)bp;
 		if ((*bp == SMP_MAGIC_IDENT) &&
 		    (mpf->length == 1) &&
@@ -850,7 +870,7 @@  static int __init update_mp_table(void)
 
 	mpf = early_memremap(mpf_base, sizeof(*mpf));
 	if (!mpf) {
-		pr_err("MPTABLE: mpf early_memremap() failed\n");
+		pr_err("mpf early_memremap() failed\n");
 		return 0;
 	}
 
@@ -864,9 +884,14 @@  static int __init update_mp_table(void)
 		goto do_unmap_mpf;
 
 	size = get_mpc_size(mpf->physptr);
+	if (!size) {
+		pr_err("error getting MP table size\n");
+		goto do_unmap_mpf;
+	}
+
 	mpc = early_memremap(mpf->physptr, size);
 	if (!mpc) {
-		pr_err("MPTABLE: mpc early_memremap() failed\n");
+		pr_err("mpc early_memremap() failed\n");
 		goto do_unmap_mpf;
 	}
 
@@ -897,7 +922,7 @@  static int __init update_mp_table(void)
 	} else {
 		mpc_new = early_memremap(mpc_new_phys, mpc_new_length);
 		if (!mpc_new) {
-			pr_err("MPTABLE: new mpc early_memremap() failed\n");
+			pr_err("new mpc early_memremap() failed\n");
 			goto do_unmap_mpc;
 		}
 		mpf->physptr = mpc_new_phys;
@@ -911,7 +936,7 @@  static int __init update_mp_table(void)
 			/* steal 16 bytes from [0, 1k) */
 			mpf_new = early_memremap(0x400 - 16, sizeof(*mpf_new));
 			if (!mpf_new) {
-				pr_err("MPTABLE: new mpf early_memremap() failed\n");
+				pr_err("new mpf early_memremap() failed\n");
 				goto do_unmap_mpc;
 			}
 			pr_info("mpf new: %x\n", 0x400 - 16);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7..621fc3e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -5,6 +5,9 @@ 
  * This file contains the setup_arch() code, which handles the architecture-dependent
  * parts of early kernel initialization.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/acpi.h>
 #include <linux/console.h>
 #include <linux/crash_dump.h>
@@ -344,7 +347,7 @@  static void __init add_early_ima_buffer(u64 phys_addr)
 
 	data = early_memremap(phys_addr + sizeof(struct setup_data), sizeof(*data));
 	if (!data) {
-		pr_warn("setup: failed to memremap ima_setup_data entry\n");
+		pr_warn("failed to memremap ima_setup_data entry\n");
 		return;
 	}
 
@@ -401,6 +404,11 @@  static void __init parse_setup_data(void)
 		u32 data_len, data_type;
 
 		data = early_memremap(pa_data, sizeof(*data));
+		if (!data) {
+			pr_warn("failed to memremap in parse_setup_data\n");
+			return;
+		}
+
 		data_len = data->len + sizeof(struct setup_data);
 		data_type = data->type;
 		pa_next = data->next;
@@ -421,6 +429,11 @@  static void __init parse_setup_data(void)
 			break;
 		case SETUP_RNG_SEED:
 			data = early_memremap(pa_data, data_len);
+			if (!data) {
+				pr_warn("failed to memremap RNG seed data\n");
+				return;
+			}
+
 			add_bootloader_randomness(data->data, data->len);
 			/* Zero seed for forward secrecy. */
 			memzero_explicit(data->data, data->len);
@@ -446,7 +459,7 @@  static void __init memblock_x86_reserve_range_setup_data(void)
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
 		if (!data) {
-			pr_warn("setup: failed to memremap setup_data entry\n");
+			pr_warn("failed to memremap setup_data entry\n");
 			return;
 		}
 
@@ -460,7 +473,7 @@  static void __init memblock_x86_reserve_range_setup_data(void)
 			early_memunmap(data, sizeof(*data));
 			data = early_memremap(pa_data, len);
 			if (!data) {
-				pr_warn("setup: failed to memremap indirect setup_data\n");
+				pr_warn("failed to memremap indirect setup_data\n");
 				return;
 			}
 
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index c1cd28e..2135bfe 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -70,6 +70,8 @@  static void __init reserve_shared_info(void)
 
 	memblock_reserve(pa, PAGE_SIZE);
 	HYPERVISOR_shared_info = early_memremap(pa, PAGE_SIZE);
+	if (!HYPERVISOR_shared_info)
+		panic("xen: failed to memmap hypervisor shared page: 0x%llx\n", pa);
 }
 
 static void __init xen_hvm_init_mem_mapping(void)
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index ee29fb5..b164d8f 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1824,6 +1824,8 @@  static unsigned long __init xen_read_phys_ulong(phys_addr_t addr)
 	unsigned long val;
 
 	vaddr = early_memremap_ro(addr, sizeof(val));
+	if (!vaddr)
+		panic("xen: failed to memmap physical address: 0x%llx\n", addr);
 	val = *vaddr;
 	early_memunmap(vaddr, sizeof(val));
 	return val;
@@ -1919,14 +1921,20 @@  void __init xen_relocate_p2m(void)
 	new_p2m = (unsigned long *)(2 * PGDIR_SIZE);
 	for (idx_pud = 0; idx_pud < n_pud; idx_pud++) {
 		pud = early_memremap(pud_phys, PAGE_SIZE);
+		if (!pud)
+			panic("xen: failed to memmap PUD physical address: 0x%llx\n", pud_phys);
 		clear_page(pud);
 		for (idx_pmd = 0; idx_pmd < min(n_pmd, PTRS_PER_PUD);
 				idx_pmd++) {
 			pmd = early_memremap(pmd_phys, PAGE_SIZE);
+			if (!pmd)
+				panic("xen: failed to memmap PMD physical address: 0x%llx\n", pmd_phys);
 			clear_page(pmd);
 			for (idx_pt = 0; idx_pt < min(n_pt, PTRS_PER_PMD);
 					idx_pt++) {
 				pt = early_memremap(pt_phys, PAGE_SIZE);
+				if (!pt)
+					panic("xen: failed to memmap PT physical address: 0x%llx\n", pt_phys);
 				clear_page(pt);
 				for (idx_pte = 0;
 				     idx_pte < min(n_pte, PTRS_PER_PTE);
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 4f43095..2f3cf6c 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -685,6 +685,8 @@  static void __init xen_phys_memcpy(phys_addr_t dest, phys_addr_t src,
 		len = min(dest_len, src_len);
 		to = early_memremap(dest - dest_off, dest_len + dest_off);
 		from = early_memremap(src - src_off, src_len + src_off);
+		if (!to || !from)
+			panic("xen: failed to memmap for physical address memcpy\n");
 		memcpy(to, from, len);
 		early_memunmap(to, dest_len + dest_off);
 		early_memunmap(from, src_len + src_off);