From: Mike Rapoport rppt@linux.ibm.com
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/Yl65zxGgFzF1Okac@sirena.org.uk Reported-by: "kernelci.org bot" bot@kernelci.org Tested-by: Mark Brown broonie@kernel.org Cc: stable@vger.kernel.org # 5.4+ Signed-off-by: Mike Rapoport rppt@linux.ibm.com --- arch/arm/include/asm/io.h | 4 ++++ arch/arm/mm/ioremap.c | 9 ++++++++- arch/arm64/include/asm/io.h | 4 ++++ arch/arm64/mm/ioremap.c | 8 ++++++++ kernel/iomem.c | 2 +- 5 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 0c70eb688a00..fbb2eeea7285 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -145,6 +145,10 @@ extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t, unsigned int, void *); extern void (*arch_iounmap)(volatile void __iomem *);
+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 + /* * Bad read/write accesses... */ diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c index aa08bcb72db9..6eb1ad24544d 100644 --- a/arch/arm/mm/ioremap.c +++ b/arch/arm/mm/ioremap.c @@ -43,7 +43,6 @@ #include <asm/mach/pci.h> #include "mm.h"
- LIST_HEAD(static_vmlist);
static struct static_vm *find_static_vm_paddr(phys_addr_t paddr, @@ -493,3 +492,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); +} diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 7fd836bea7eb..3995652daf81 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -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 */ diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index b7c81dacabf0..b21f91cd830d 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -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); +} diff --git a/kernel/iomem.c b/kernel/iomem.c index 62c92e43aa0d..e85bed24c0a9 100644 --- a/kernel/iomem.c +++ b/kernel/iomem.c @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, unsigned long pfn = PHYS_PFN(offset);
/* In the simple case just return the existing linear address */ - if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) && + if (!PageHighMem(pfn_to_page(pfn)) && arch_memremap_can_ram_remap(offset, size, flags)) return __va(offset);
base-commit: b2d229d4ddb17db541098b83524d901257e93845
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on b2d229d4ddb17db541098b83524d901257e93845]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Rapoport/arm-64-memremap... base: b2d229d4ddb17db541098b83524d901257e93845 config: arm-randconfig-r015-20220424 (https://download.01.org/0day-ci/archive/20220425/202204250429.nq0alVBK-lkp@i...) compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/635763878be30ab45f350cdcffba3d... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242 git checkout 635763878be30ab45f350cdcffba3d8e71089942 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: kernel/iomem.o: in function `memremap':
iomem.c:(.text+0x36): undefined reference to `arch_memremap_can_ram_remap'
arm-linux-gnueabi-ld: drivers/gpu/drm/drm_gem_shmem_helper.o: in function `drm_gem_shmem_fault': drm_gem_shmem_helper.c:(.text+0x54): undefined reference to `vmf_insert_pfn'
Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for DRM_GEM_SHMEM_HELPER Depends on HAS_IOMEM && DRM && MMU Selected by - DRM_SSD130X && HAS_IOMEM && DRM
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on b2d229d4ddb17db541098b83524d901257e93845]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Rapoport/arm-64-memremap... base: b2d229d4ddb17db541098b83524d901257e93845 config: arm-randconfig-c002-20220424 (https://download.01.org/0day-ci/archive/20220425/202204250425.OjOiS0ZK-lkp@i...) compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/635763878be30ab45f350cdcffba3d... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242 git checkout 635763878be30ab45f350cdcffba3d8e71089942 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: kernel/iomem.o: in function `try_ram_remap':
kernel/iomem.c:37: undefined reference to `arch_memremap_can_ram_remap'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_i2c_remove': drivers/gpu/drm/bridge/ite-it6505.c:3317: undefined reference to `drm_dp_aux_unregister' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_get_dpcd': drivers/gpu/drm/bridge/ite-it6505.c:602: undefined reference to `drm_dp_dpcd_read' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_bridge_attach': drivers/gpu/drm/bridge/ite-it6505.c:2858: undefined reference to `drm_dp_aux_register' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_writeb': include/drm/dp/drm_dp_helper.h:2088: undefined reference to `drm_dp_dpcd_write' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_readb': include/drm/dp/drm_dp_helper.h:2073: undefined reference to `drm_dp_dpcd_read' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_hdcp_work': drivers/gpu/drm/bridge/ite-it6505.c:2084: undefined reference to `drm_dp_dpcd_read_link_status' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:2088: undefined reference to `drm_dp_channel_eq_ok' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_step_cr_train': drivers/gpu/drm/bridge/ite-it6505.c:1688: undefined reference to `drm_dp_link_train_clock_recovery_delay' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1689: undefined reference to `drm_dp_dpcd_read_link_status' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1691: undefined reference to `drm_dp_clock_recovery_ok' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1704: undefined reference to `drm_dp_get_adjust_request_voltage' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1708: undefined reference to `drm_dp_get_adjust_request_pre_emphasis' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_step_eq_train': drivers/gpu/drm/bridge/ite-it6505.c:1760: undefined reference to `drm_dp_link_train_channel_eq_delay' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1761: undefined reference to `drm_dp_dpcd_read_link_status' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1763: undefined reference to `drm_dp_clock_recovery_ok' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1766: undefined reference to `drm_dp_channel_eq_ok' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1777: undefined reference to `drm_dp_get_adjust_request_voltage' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1781: undefined reference to `drm_dp_get_adjust_request_pre_emphasis' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_drm_dp_link_configure': drivers/gpu/drm/bridge/ite-it6505.c:1603: undefined reference to `drm_dp_dpcd_write' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_parse_link_capabilities': drivers/gpu/drm/bridge/ite-it6505.c:1457: undefined reference to `drm_dp_link_rate_to_bw_code' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_drm_dp_link_probe': drivers/gpu/drm/bridge/ite-it6505.c:726: undefined reference to `drm_dp_dpcd_read' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:731: undefined reference to `drm_dp_bw_code_to_link_rate' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_readb': include/drm/dp/drm_dp_helper.h:2073: undefined reference to `drm_dp_dpcd_read' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_writeb': include/drm/dp/drm_dp_helper.h:2088: undefined reference to `drm_dp_dpcd_write' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_process_hpd_irq': drivers/gpu/drm/bridge/ite-it6505.c:2293: undefined reference to `drm_dp_dpcd_read_link_status' arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:2302: undefined reference to `drm_dp_channel_eq_ok'
vim +37 kernel/iomem.c
5981690ddb8f72 Dan Williams 2018-03-29 29 5981690ddb8f72 Dan Williams 2018-03-29 30 static void *try_ram_remap(resource_size_t offset, size_t size, 5981690ddb8f72 Dan Williams 2018-03-29 31 unsigned long flags) 5981690ddb8f72 Dan Williams 2018-03-29 32 { 5981690ddb8f72 Dan Williams 2018-03-29 33 unsigned long pfn = PHYS_PFN(offset); 5981690ddb8f72 Dan Williams 2018-03-29 34 5981690ddb8f72 Dan Williams 2018-03-29 35 /* In the simple case just return the existing linear address */ 635763878be30a Mike Rapoport 2022-04-24 36 if (!PageHighMem(pfn_to_page(pfn)) && 5981690ddb8f72 Dan Williams 2018-03-29 @37 arch_memremap_can_ram_remap(offset, size, flags)) 5981690ddb8f72 Dan Williams 2018-03-29 38 return __va(offset); 5981690ddb8f72 Dan Williams 2018-03-29 39 5981690ddb8f72 Dan Williams 2018-03-29 40 return NULL; /* fallback to arch_memremap_wb */ 5981690ddb8f72 Dan Williams 2018-03-29 41 } 5981690ddb8f72 Dan Williams 2018-03-29 42
On Sun, 24 Apr 2022 at 19:22, Mike Rapoport rppt@kernel.org wrote:
From: Mike Rapoport rppt@linux.ibm.com
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/Yl65zxGgFzF1Okac@sirena.org.uk Reported-by: "kernelci.org bot" bot@kernelci.org Tested-by: Mark Brown broonie@kernel.org Cc: stable@vger.kernel.org # 5.4+ Signed-off-by: Mike Rapoport rppt@linux.ibm.com
arch/arm/include/asm/io.h | 4 ++++ arch/arm/mm/ioremap.c | 9 ++++++++- arch/arm64/include/asm/io.h | 4 ++++ arch/arm64/mm/ioremap.c | 8 ++++++++ kernel/iomem.c | 2 +- 5 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 0c70eb688a00..fbb2eeea7285 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -145,6 +145,10 @@ extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t, unsigned int, void *); extern void (*arch_iounmap)(volatile void __iomem *);
+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
/*
- Bad read/write accesses...
*/ diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c index aa08bcb72db9..6eb1ad24544d 100644 --- a/arch/arm/mm/ioremap.c +++ b/arch/arm/mm/ioremap.c @@ -43,7 +43,6 @@ #include <asm/mach/pci.h> #include "mm.h"
LIST_HEAD(static_vmlist);
static struct static_vm *find_static_vm_paddr(phys_addr_t paddr, @@ -493,3 +492,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);+} diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 7fd836bea7eb..3995652daf81 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -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 */ diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index b7c81dacabf0..b21f91cd830d 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -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);+} diff --git a/kernel/iomem.c b/kernel/iomem.c index 62c92e43aa0d..e85bed24c0a9 100644 --- a/kernel/iomem.c +++ b/kernel/iomem.c @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, unsigned long pfn = PHYS_PFN(offset);
/* In the simple case just return the existing linear address */
if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) &&
if (!PageHighMem(pfn_to_page(pfn)) &&
This looks wrong to me. Calling any of the PageXxx() accessors is only safe if the PFN is valid, since otherwise, we don't know if the associated struct page exists.
arch_memremap_can_ram_remap(offset, size, flags)) return __va(offset);base-commit: b2d229d4ddb17db541098b83524d901257e93845
2.28.0
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Apr 24, 2022 at 11:19:05PM +0200, Ard Biesheuvel wrote:
On Sun, 24 Apr 2022 at 19:22, Mike Rapoport rppt@kernel.org wrote:
From: Mike Rapoport rppt@linux.ibm.com
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.
...
diff --git a/kernel/iomem.c b/kernel/iomem.c index 62c92e43aa0d..e85bed24c0a9 100644 --- a/kernel/iomem.c +++ b/kernel/iomem.c @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, unsigned long pfn = PHYS_PFN(offset);
/* In the simple case just return the existing linear address */
if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) &&
if (!PageHighMem(pfn_to_page(pfn)) &&This looks wrong to me. Calling any of the PageXxx() accessors is only safe if the PFN is valid, since otherwise, we don't know if the associated struct page exists.
Yeah, you are right, was over-enthusiastic here...
arch_memremap_can_ram_remap(offset, size, flags)) return __va(offset);base-commit: b2d229d4ddb17db541098b83524d901257e93845
2.28.0
linux-stable-mirror@lists.linaro.org
-
Ard Biesheuvel -
kernel test robot -
Mike Rapoport