Skip to contentSkip to conversation

arm[64]/memremap: don't abuse pfn_valid() to ensure presence of linear map

PATCH In review Mike Rapoport 3 participants 3 messages Apr 26, 2022 – Apr 29, 2022

Subsystems (3)
Stable branch Arm (32-bit) arm64
lore
Share & tools
lore

Mike Rapoport

author
#0 lore raw

PATCH v2 1/1 arm[64]/memremap: don't abuse pfn_valid() to ensure presence of linear map

From: Mike Rapoport <[email protected]>

The semantics of pfn_valid() is to check presence of the memory map for a PFN and not whether a PFN is covered by the linear map. The memory map may be present for NOMAP memory regions, but they won't be mapped in the linear mapping. Accessing such regions via __va() when they are memremap()'ed will cause a crash.

On v5.4.y the crash happens on qemu-arm with UEFI [1]:

<1>[    0.084476] 8<--- cut here ---
<1>[    0.084595] Unable to handle kernel paging request at virtual address dfb76000
<1>[    0.084938] pgd = (ptrval)
<1>[    0.085038] [dfb76000] *pgd=5f7fe801, *pte=00000000, *ppte=00000000

...

<4>[    0.093923] [<c0ed6ce8>] (memcpy) from [<c16a06f8>] (dmi_setup+0x60/0x418)
<4>[    0.094204] [<c16a06f8>] (dmi_setup) from [<c16a38d4>] (arm_dmi_init+0x8/0x10)
<4>[    0.094408] [<c16a38d4>] (arm_dmi_init) from [<c0302e9c>] (do_one_initcall+0x50/0x228)
<4>[    0.094619] [<c0302e9c>] (do_one_initcall) from [<c16011e4>] (kernel_init_freeable+0x15c/0x1f8)
<4>[    0.094841] [<c16011e4>] (kernel_init_freeable) from [<c0f028cc>] (kernel_init+0x8/0x10c)
<4>[    0.095057] [<c0f028cc>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)

On kernels v5.10.y and newer the same crash won't reproduce on ARM because commit b10d6bca8720 ("arch, drivers: replace for_each_membock() with for_each_mem_range()") changed the way memory regions are registered in the resource tree, but that merely covers up the problem.

On ARM64 memory resources registered in yet another way and there the issue of wrong usage of pfn_valid() to ensure availability of the linear map is also covered.

Implement arch_memremap_can_ram_remap() on ARM and ARM64 to prevent access to NOMAP regions via the linear mapping in memremap().

Link https://lore.kernel.org/all/[email protected] open here
Reported-by "kernelci.org bot" <[email protected]>
Tested-by Mark Brown <[email protected]>
Cc [email protected] # 5.4+
Signed-off-by Mike Rapoport <[email protected]>

---
v2: don't remove pfn_valid() from try_ram_remap(), per Ard

 arch/arm/include/asm/io.h   | 3 +++
 arch/arm/mm/ioremap.c       | 8 ++++++++
 arch/arm64/include/asm/io.h | 4 ++++
 arch/arm64/mm/ioremap.c     | 8 ++++++++
 4 files changed, 23 insertions(+)
arch/arm/include/asm/io.h +3 -0
index 0c70eb688a00..2a0739a2350b 100644
@@ -440,6 +440,9 @@ extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr);
 #define ARCH_HAS_VALID_PHYS_ADDR_RANGE
 extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
 extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
+extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+					unsigned long flags);
+#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap
 #endif
 
 /*
arch/arm/mm/ioremap.c +8 -0
index aa08bcb72db9..290702328a33 100644
@@ -493,3 +493,11 @@ void __init early_ioremap_init(void)
 {
 	early_ioremap_setup();
 }
+
+bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+				 unsigned long flags)
+{
+	unsigned long pfn = PHYS_PFN(offset);
+
+	return memblock_is_map_memory(pfn);
+}
arch/arm64/include/asm/io.h +4 -0
index 7fd836bea7eb..3995652daf81 100644
@@ -192,4 +192,8 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
 extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
 
+extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+					unsigned long flags);
+#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap
+
 #endif	/* __ASM_IO_H */
arch/arm64/mm/ioremap.c +8 -0
index b7c81dacabf0..b21f91cd830d 100644
@@ -99,3 +99,11 @@ void __init early_ioremap_init(void)
 {
 	early_ioremap_setup();
 }
+
+bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+				 unsigned long flags)
+{
+	unsigned long pfn = PHYS_PFN(offset);
+
+	return pfn_is_map_memory(pfn);
+}

base-commit: b2d229d4ddb17db541098b83524d901257e93845

Signature · 1 line
2.28.0
Reviewed-by on v2 1/1
#1 lore raw
Quoted 42 lines · Mike Rapoport

From: Mike Rapoport <[email protected]>

The semantics of pfn_valid() is to check presence of the memory map for a PFN and not whether a PFN is covered by the linear map. The memory map may be present for NOMAP memory regions, but they won't be mapped in the linear mapping. Accessing such regions via __va() when they are memremap()'ed will cause a crash.

On v5.4.y the crash happens on qemu-arm with UEFI [1]:

<1>[    0.084476] 8<--- cut here ---
<1>[    0.084595] Unable to handle kernel paging request at virtual address dfb76000
<1>[    0.084938] pgd = (ptrval)
<1>[    0.085038] [dfb76000] *pgd=5f7fe801, *pte=00000000, *ppte=00000000

...

<4>[    0.093923] [<c0ed6ce8>] (memcpy) from [<c16a06f8>] (dmi_setup+0x60/0x418)
<4>[    0.094204] [<c16a06f8>] (dmi_setup) from [<c16a38d4>] (arm_dmi_init+0x8/0x10)
<4>[    0.094408] [<c16a38d4>] (arm_dmi_init) from [<c0302e9c>] (do_one_initcall+0x50/0x228)
<4>[    0.094619] [<c0302e9c>] (do_one_initcall) from [<c16011e4>] (kernel_init_freeable+0x15c/0x1f8)
<4>[    0.094841] [<c16011e4>] (kernel_init_freeable) from [<c0f028cc>] (kernel_init+0x8/0x10c)
<4>[    0.095057] [<c0f028cc>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)

On kernels v5.10.y and newer the same crash won't reproduce on ARM because commit b10d6bca8720 ("arch, drivers: replace for_each_membock() with for_each_mem_range()") changed the way memory regions are registered in the resource tree, but that merely covers up the problem.

On ARM64 memory resources registered in yet another way and there the issue of wrong usage of pfn_valid() to ensure availability of the linear map is also covered.

Implement arch_memremap_can_ram_remap() on ARM and ARM64 to prevent access to NOMAP regions via the linear mapping in memremap().

Link https://lore.kernel.org/all/[email protected] open here
Reported-by "kernelci.org bot" <[email protected]>
Tested-by Mark Brown <[email protected]>
Cc [email protected] # 5.4+
Signed-off-by Mike Rapoport <[email protected]>
Quoted 9 lines

---
v2: don't remove pfn_valid() from try_ram_remap(), per Ard

 arch/arm/include/asm/io.h   | 3 +++
 arch/arm/mm/ioremap.c       | 8 ++++++++
 arch/arm64/include/asm/io.h | 4 ++++
 arch/arm64/mm/ioremap.c     | 8 ++++++++
 4 files changed, 23 insertions(+)

I think this looks reasonable, but you'll need to split it in two if it is going through the respective arch trees.

Reviewed-by Ard Biesheuvel <[email protected]>
Quoted 64 lines
arch/arm/include/asm/io.h +3 -0
index 0c70eb688a00..2a0739a2350b 100644
@@ -440,6 +440,9 @@ extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr);
 #define ARCH_HAS_VALID_PHYS_ADDR_RANGE
 extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
 extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
+extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+                                       unsigned long flags);
+#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap
 #endif
 
 /*
arch/arm/mm/ioremap.c +8 -0
index aa08bcb72db9..290702328a33 100644
@@ -493,3 +493,11 @@ void __init early_ioremap_init(void)
 {
        early_ioremap_setup();
 }
+
+bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+                                unsigned long flags)
+{
+       unsigned long pfn = PHYS_PFN(offset);
+
+       return memblock_is_map_memory(pfn);
+}
arch/arm64/include/asm/io.h +4 -0
index 7fd836bea7eb..3995652daf81 100644
@@ -192,4 +192,8 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
 extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
 
+extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+                                       unsigned long flags);
+#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap
+
 #endif /* __ASM_IO_H */
arch/arm64/mm/ioremap.c +8 -0
index b7c81dacabf0..b21f91cd830d 100644
@@ -99,3 +99,11 @@ void __init early_ioremap_init(void)
 {
        early_ioremap_setup();
 }
+
+bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size,
+                                unsigned long flags)
+{
+       unsigned long pfn = PHYS_PFN(offset);
+
+       return pfn_is_map_memory(pfn);
+}

base-commit: b2d229d4ddb17db541098b83524d901257e93845 --
2.28.0

maintainer Acked-by
#2 lore raw
Quoted 52 lines · Ard Biesheuvel
Quoted 41 lines · Mike Rapoport

From: Mike Rapoport <[email protected]>

The semantics of pfn_valid() is to check presence of the memory map for a PFN and not whether a PFN is covered by the linear map. The memory map may be present for NOMAP memory regions, but they won't be mapped in the linear mapping. Accessing such regions via __va() when they are memremap()'ed will cause a crash.

On v5.4.y the crash happens on qemu-arm with UEFI [1]:

<1>[    0.084476] 8<--- cut here ---
<1>[    0.084595] Unable to handle kernel paging request at virtual address dfb76000
<1>[    0.084938] pgd = (ptrval)
<1>[    0.085038] [dfb76000] *pgd=5f7fe801, *pte=00000000, *ppte=00000000

...

<4>[    0.093923] [<c0ed6ce8>] (memcpy) from [<c16a06f8>] (dmi_setup+0x60/0x418)
<4>[    0.094204] [<c16a06f8>] (dmi_setup) from [<c16a38d4>] (arm_dmi_init+0x8/0x10)
<4>[    0.094408] [<c16a38d4>] (arm_dmi_init) from [<c0302e9c>] (do_one_initcall+0x50/0x228)
<4>[    0.094619] [<c0302e9c>] (do_one_initcall) from [<c16011e4>] (kernel_init_freeable+0x15c/0x1f8)
<4>[    0.094841] [<c16011e4>] (kernel_init_freeable) from [<c0f028cc>] (kernel_init+0x8/0x10c)
<4>[    0.095057] [<c0f028cc>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)

On kernels v5.10.y and newer the same crash won't reproduce on ARM because commit b10d6bca8720 ("arch, drivers: replace for_each_membock() with for_each_mem_range()") changed the way memory regions are registered in the resource tree, but that merely covers up the problem.

On ARM64 memory resources registered in yet another way and there the issue of wrong usage of pfn_valid() to ensure availability of the linear map is also covered.

Implement arch_memremap_can_ram_remap() on ARM and ARM64 to prevent access to NOMAP regions via the linear mapping in memremap().

Link https://lore.kernel.org/all/[email protected] open here
Reported-by "kernelci.org bot" <[email protected]>
Tested-by Mark Brown <[email protected]>
Cc [email protected] # 5.4+
Signed-off-by Mike Rapoport <[email protected]>
Quoted 9 lines

---
v2: don't remove pfn_valid() from try_ram_remap(), per Ard

 arch/arm/include/asm/io.h   | 3 +++
 arch/arm/mm/ioremap.c       | 8 ++++++++
 arch/arm64/include/asm/io.h | 4 ++++
 arch/arm64/mm/ioremap.c     | 8 ++++++++
 4 files changed, 23 insertions(+)
Quoted 2 lines

I think this looks reasonable, but you'll need to split it in two if it is going through the respective arch trees.

I guess that would be best. Otherwise, if Andrew picks it up:

Acked-by Catalin Marinas <[email protected]>