While sending an earlier patch set it was discovered that there are a
number of places in early x86 code were the functions early_memremap()
and early_ioremap() are called but the returned pointer is not checked
for NULL. Since NULL can be returned for a couple of reasons, the return
value should be checked for NULL.
This set fixes the places where the checks were missing. It was not always
clear what the best failure mode should be when NULL is detected. In modules
where other places tended to pr_warn or panic e.g., the same was done for
the checks. In other places it was based on how significantly fatal the
failure would end up being. The review process may point out places where
this should be changed.
Ross Philipson (2):
x86: Check return values from early_memremap calls
x86: Check return values from early_ioremap calls
arch/x86/kernel/apic/x2apic_uv_x.c | 2 ++
arch/x86/kernel/devicetree.c | 10 ++++++++++
arch/x86/kernel/e820.c | 5 +++++
arch/x86/kernel/early_printk.c | 2 ++
arch/x86/kernel/jailhouse.c | 6 ++++++
arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
arch/x86/kernel/setup.c | 5 +++++
arch/x86/kernel/vsmp_64.c | 3 +++
arch/x86/xen/enlighten_hvm.c | 2 ++
arch/x86/xen/mmu_pv.c | 8 ++++++++
arch/x86/xen/setup.c | 2 ++
11 files changed, 68 insertions(+)
--
1.8.3.1
There are a number of places where early_ioremap 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.
Signed-off-by: Ross Philipson <[email protected]>
---
arch/x86/kernel/apic/x2apic_uv_x.c | 2 ++
arch/x86/kernel/early_printk.c | 2 ++
arch/x86/kernel/vsmp_64.c | 3 +++
3 files changed, 7 insertions(+)
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index f5a48e6..7a64287 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -75,6 +75,8 @@ static unsigned long __init uv_early_read_mmr(unsigned long addr)
unsigned long val, *mmr;
mmr = early_ioremap(UV_LOCAL_MMR_BASE | addr, sizeof(*mmr));
+ if (!mmr)
+ panic("UV: error: failed to ioremap MMR\n");
val = *mmr;
early_iounmap(mmr, sizeof(*mmr));
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 68b3892..35b228d 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -290,6 +290,8 @@ static __init void early_pci_serial_init(char *s)
/* WARNING! assuming the address is always in the first 4G */
early_serial_base =
(unsigned long)early_ioremap(bar0 & 0xfffffff0, 0x10);
+ if (!early_serial_base)
+ panic("early_serial: failed to ioremap MMIO BAR\n");
write_pci_config(bus, slot, func, PCI_COMMAND,
cmdreg|PCI_COMMAND_MEMORY);
}
diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c
index 796cfaa..39769f4 100644
--- a/arch/x86/kernel/vsmp_64.c
+++ b/arch/x86/kernel/vsmp_64.c
@@ -32,6 +32,9 @@ static void __init set_vsmp_ctl(void)
/* set vSMP magic bits to indicate vSMP capable kernel */
cfg = read_pci_config(0, 0x1f, 0, PCI_BASE_ADDRESS_0);
address = early_ioremap(cfg, 8);
+ if (WARN_ON(!address))
+ return;
+
cap = readl(address);
ctl = readl(address + 4);
printk(KERN_INFO "vSMP CTL: capabilities:0x%08x control:0x%08x\n",
--
1.8.3.1
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.
Signed-off-by: Ross Philipson <[email protected]>
---
arch/x86/kernel/devicetree.c | 10 ++++++++++
arch/x86/kernel/e820.c | 5 +++++
arch/x86/kernel/jailhouse.c | 6 ++++++
arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
arch/x86/kernel/setup.c | 5 +++++
arch/x86/xen/enlighten_hvm.c | 2 ++
arch/x86/xen/mmu_pv.c | 8 ++++++++
arch/x86/xen/setup.c | 2 ++
8 files changed, 61 insertions(+)
diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f2..ca23875 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -292,10 +292,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("devicetree: 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("devicetree: 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 f267205..f90b883 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -728,6 +728,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("e820: failed to memremap extended\n");
+ return;
+ }
+
entries = sdata->len / sizeof(*extmap);
extmap = (struct boot_e820_entry *)(sdata->data);
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..8bac042 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -424,6 +424,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 +440,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("MPTABLE: error getting MP table size\n");
+ return -1;
+ }
+
mpc = early_memremap(mpf->physptr, size);
+ if (!mpc) {
+ pr_err("MPTABLE: error mapping MP table physptr\n");
+ return -1;
+ }
/*
* Read the physical hardware table. Anything here will
@@ -552,6 +564,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 +580,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("MPTABLE: error mapping SMP config\n");
+ return 0;
+ }
+
mpf = (struct mpf_intel *)bp;
if ((*bp == SMP_MAGIC_IDENT) &&
(mpf->length == 1) &&
@@ -864,6 +882,11 @@ static int __init update_mp_table(void)
goto do_unmap_mpf;
size = get_mpc_size(mpf->physptr);
+ if (!size) {
+ pr_err("MPTABLE: 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");
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c95b9ac..824e901 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -345,6 +345,11 @@ static void __init parse_setup_data(void)
u32 data_len, data_type;
data = early_memremap(pa_data, sizeof(*data));
+ if (!data) {
+ pr_warn("setup: 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;
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 517a9d8..6182c5b 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -64,6 +64,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 0035486..2aff5c6 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1823,6 +1823,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;
@@ -1918,14 +1920,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 81aa46f..5e74496 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);
--
1.8.3.1
Ross,
On Fri, Apr 15 2022 at 11:10, Ross Philipson wrote:
can you please ensure that all relevant maintainers are CC'ed on such
patches? XEN and Jailhouse people might have opinions, no?
> 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.
Also please follow:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs
Thanks,
tglx
Adding Xen and Jailhouse people and MLs to Cc.
Folks, thread starts here:
https://lore.kernel.org/r/[email protected]
On Fri, Apr 15, 2022 at 11:10:00AM -0400, 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.
>
> Signed-off-by: Ross Philipson <[email protected]>
> ---
> arch/x86/kernel/devicetree.c | 10 ++++++++++
> arch/x86/kernel/e820.c | 5 +++++
> arch/x86/kernel/jailhouse.c | 6 ++++++
> arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
> arch/x86/kernel/setup.c | 5 +++++
> arch/x86/xen/enlighten_hvm.c | 2 ++
> arch/x86/xen/mmu_pv.c | 8 ++++++++
> arch/x86/xen/setup.c | 2 ++
> 8 files changed, 61 insertions(+)
Ok, a couple of notes:
1. the pr_*("<prefix>:" ... )
thing is done using pr_fmt() - grep the tree for examples.
2. I think you should not panic() the machine but issue a the
warning/error and let the machine die a painful death anyway. But Xen
folks will know better what would be the optimal thing to do.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 08.10.22 17:12, Borislav Petkov wrote:
> Adding Xen and Jailhouse people and MLs to Cc.
>
> Folks, thread starts here:
>
> https://lore.kernel.org/r/[email protected]
>
> On Fri, Apr 15, 2022 at 11:10:00AM -0400, 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.
>>
>> Signed-off-by: Ross Philipson <[email protected]>
>> ---
>> arch/x86/kernel/devicetree.c | 10 ++++++++++
>> arch/x86/kernel/e820.c | 5 +++++
>> arch/x86/kernel/jailhouse.c | 6 ++++++
>> arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
>> arch/x86/kernel/setup.c | 5 +++++
>> arch/x86/xen/enlighten_hvm.c | 2 ++
>> arch/x86/xen/mmu_pv.c | 8 ++++++++
>> arch/x86/xen/setup.c | 2 ++
>> 8 files changed, 61 insertions(+)
>
> Ok, a couple of notes:
>
> 1. the pr_*("<prefix>:" ... )
>
> thing is done using pr_fmt() - grep the tree for examples.
>
> 2. I think you should not panic() the machine but issue a the
> warning/error and let the machine die a painful death anyway. But Xen
> folks will know better what would be the optimal thing to do.
>
> Thx.
>
For the Jailhouse bits:
Reviewed-by: Jan Kiszka <[email protected]>
(IOW, panic'ing is fine for us here)
Thanks,
Jan
--
Siemens AG, Technology
Competence Center Embedded Linux
On 10/8/22 11:12, Borislav Petkov wrote:
> Adding Xen and Jailhouse people and MLs to Cc.
>
> Folks, thread starts here:
>
> https://lore.kernel.org/r/[email protected]
>
> On Fri, Apr 15, 2022 at 11:10:00AM -0400, 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.
>>
>> Signed-off-by: Ross Philipson <[email protected]>
>> ---
>> arch/x86/kernel/devicetree.c | 10 ++++++++++
>> arch/x86/kernel/e820.c | 5 +++++
>> arch/x86/kernel/jailhouse.c | 6 ++++++
>> arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
>> arch/x86/kernel/setup.c | 5 +++++
>> arch/x86/xen/enlighten_hvm.c | 2 ++
>> arch/x86/xen/mmu_pv.c | 8 ++++++++
>> arch/x86/xen/setup.c | 2 ++
>> 8 files changed, 61 insertions(+)
>
> Ok, a couple of notes:
>
> 1. the pr_*("<prefix>:" ... )
>
> thing is done using pr_fmt() - grep the tree for examples.
I am already using the pr_* macros in the patches. Are you asking me to
do something or is this just informational?
>
> 2. I think you should not panic() the machine but issue a the
> warning/error and let the machine die a painful death anyway. But Xen
> folks will know better what would be the optimal thing to do.
When I was working on the patches I asked Andrew Cooper at Citrix what
action I should take if any of the calls in the Xen code failed. I
believe he told me it was basically fatal and that panic() would be fine
there.
Thank you,
Ross Philipson
>
> Thx.
>
On 12.10.22 17:13, Ross Philipson wrote:
> On 10/8/22 11:12, Borislav Petkov wrote:
>> Adding Xen and Jailhouse people and MLs to Cc.
>>
>> Folks, thread starts here:
>>
>> https://lore.kernel.org/r/[email protected]
>>
>> On Fri, Apr 15, 2022 at 11:10:00AM -0400, 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.
>>>
>>> Signed-off-by: Ross Philipson <[email protected]>
>>> ---
>>> arch/x86/kernel/devicetree.c | 10 ++++++++++
>>> arch/x86/kernel/e820.c | 5 +++++
>>> arch/x86/kernel/jailhouse.c | 6 ++++++
>>> arch/x86/kernel/mpparse.c | 23 +++++++++++++++++++++++
>>> arch/x86/kernel/setup.c | 5 +++++
>>> arch/x86/xen/enlighten_hvm.c | 2 ++
>>> arch/x86/xen/mmu_pv.c | 8 ++++++++
>>> arch/x86/xen/setup.c | 2 ++
>>> 8 files changed, 61 insertions(+)
>>
>> Ok, a couple of notes:
>>
>> 1. the pr_*("<prefix>:" ... )
>>
>> thing is done using pr_fmt() - grep the tree for examples.
>
> I am already using the pr_* macros in the patches. Are you asking me to do
> something or is this just informational?
>
>>
>> 2. I think you should not panic() the machine but issue a the
>> warning/error and let the machine die a painful death anyway. But Xen
>> folks will know better what would be the optimal thing to do.
>
> When I was working on the patches I asked Andrew Cooper at Citrix what action I
> should take if any of the calls in the Xen code failed. I believe he told me it
> was basically fatal and that panic() would be fine there.
panic() is the way to go. Everything else would make the error harder
to analyze.
BTW, CC-ing the maintainers of the modified code is good practice.
Juergen
On Wed, Oct 12, 2022 at 11:13:20AM -0400, Ross Philipson wrote:
> I am already using the pr_* macros in the patches. Are you asking me to do
> something
Yes. You do
pr_X("prefix_string: msg"
while you should set the prefix string and do
pr_X("msg".
As said, grep the tree for examples where pr_fmt() is set.
arch/x86/kernel/cpu/bugs.c is a good example.
> When I was working on the patches I asked Andrew Cooper at Citrix what
> action I should take if any of the calls in the Xen code failed. I
> believe he told me it was basically fatal and that panic() would be
> fine there.
Yes, that should be confirmed by people who know the code and you should
mention in the commit message at least that panicking is the only viable
thing to do in the respective case. If, as Jürgen says, it is actually
better to panic() in those cases, especially if it otherwise would make
debugging harder, then sure, by all means.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette