mm: disallow ioremap'ing ordinary RAM in generic_access_phys()
Subsystems (3)
David Hildenbrand (Arm)
maintainerPATCH 1/1 mm: disallow ioremap'ing ordinary RAM in generic_access_phys()
generic_access_phys() can end up getting called on CoW PFNMAP mappings, resulting in it trying to ioremap anonymous folios, which especially the x86 code doesn't enjoy, and which is in general just odd/wrong.
E.g., with a CoW mapping of a PCI BAR:
ioremap on RAM at 0x0000000045623000 - 0x0000000045623fff
WARNING: arch/x86/mm/ioremap.c:216 at __ioremap_caller.isra.0+0x4c2/0x5f0 Call Trace: generic_access_phys+0x130/0x4d0 mm/memory.c:7178 kernfs_vma_access+0x1ce/0x280 fs/kernfs/file.c:437 __access_remote_vm+0x58f/0x890 mm/memory.c:7256 mem_rw+0x2a1/0x670 fs/proc/base.c:912
But more generically, we should disallow access to anything that looks like ordinary RAM in the system: including allocating memory (e.g., alloc_pages) and mapping them in a VM_PFNMAP.
For pages with an online memmap, allow only pages marked as reserved (PageReserved). This also includes memblock allocations, however, distinguishing them from other MMIO is tricky, and nobody should really be mapping them in a PFNMAP (except someone abusing /dev/mem, which we do not care about).
So only allow access if there is no online memmap, or if the pages are marked reserved. Reject other pages with -EINVAL.
Note that, just like other pfn_to_online_page() users, we won't care about concurrent memory offlining; mapping memory that can concurrently get offlined is highly dubious.
---
I went back and forth 10 times and concluded that using PageReserved should be the right thing to check for now.
--- mm/memory.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
mm/memory.c +14 -0
@@ -7226,6 +7226,7 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,resource_size_t phys_addr;pgprot_t prot =__pgprot (0 );void __iomem *maddr; +struct page *page;int offset =offset_in_page (addr);int ret = -EINVAL ;bool writable;
@@ -7240,6 +7241,19 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, retry:if (follow_pfnmap_start (&args))return -EINVAL ; + +/* +* Reject anything that looks like ordinary RAM, including anonymous +* folios in CoW mappings or allocated kernel memory. Note that we +* cannot distinguish memblock allocations from non-RAM reliably, so +* we cannot easily reject them here. +*/ + page =pfn_to_online_page (args.pfn); +if (page && !PageReserved (page)) { +follow_pfnmap_end (&args); +return -EINVAL ; + } + prot = args.pgprot; phys_addr = (resource_size_t )args.pfn <<PAGE_SHIFT ; writable = args.writable;
---
base-commit: 634cbf5b794b734c78631b92e381cfc85f357bbe
change-id: 20261009-generic_access_phys-0f3e0806305d
--
Cheers,
David
Quoted 3 lines · David Hildenbrand (Arm)
generic_access_phys()can end up getting called on CoW PFNMAP mappings, resulting in it trying to ioremap anonymous folios, which especially the x86 code doesn't enjoy, and which is in general just odd/wrong.
Ugh god.
Quoted 11 lines
E.g., with a CoW mapping of a PCI BAR:
ioremap on RAM at
0x0000000045623000-0x0000000045623fffWARNING: arch/x86/mm/ioremap.c:216 at __ioremap_caller.isra.0+0x4c2/0x5f0 Call Trace: generic_access_phys+0x130/0x4d0 mm/memory.c:7178 kernfs_vma_access+0x1ce/0x280 fs/kernfs/file.c:437 __access_remote_vm+0x58f/0x890 mm/memory.c:7256 mem_rw+0x2a1/0x670 fs/proc/base.c:912
Quoted 3 lines
But more generically, we should disallow access to anything that looks like ordinary RAM in the system: including allocating memory (e.g.,
alloc_pages) and mapping them in aVM_PFNMAP.
Yes.
Quoted 6 lines
For pages with an online memmap, allow only pages marked as reserved (PageReserved). This also includes memblock allocations, however, distinguishing them from other MMIO is tricky, and nobody should really be mapping them in a PFNMAP (except someone abusing
/dev/mem, which we do not care about).
Yeah I guess we shouldn't try too hard for weird edge cases, or at least can revisit it later potentially.
Quoted 3 lines
So only allow access if there is no online memmap, or if the pages are marked reserved. Reject other pages with
-EINVAL.
Seems reasonable overall.
Quoted 4 lines
Note that, just like other
pfn_to_online_page()users, we won't care about concurrent memory offlining; mapping memory that can concurrently get offlined is highly dubious.
Yeah sounds like the user would be doing something crazy in this case, therefore their problem!
Quoted 8 lines
Reported-by [email protected]Reported-by Nguyen Ngoc Thang <[email protected]>Fixes 28b2ee20c7cb ("access_process_vm device memory infrastructure")Signed-off-by David Hildenbrand (Arm) <[email protected]>
LGTM so:
Quoted 3 lines
---
I went back and forth 10 times and concluded that using PageReserved should be the right thing to check for now.
It seems the sensible choice, yes. And locally, AI review agrees with you, FWIW!
--
Cheers, Lorenzo
Quoted 14 lines · David Hildenbrand (Arm)
generic_access_phys()can end up getting called on CoW PFNMAP mappings, resulting in it trying to ioremap anonymous folios, which especially the x86 code doesn't enjoy, and which is in general just odd/wrong.E.g., with a CoW mapping of a PCI BAR:
ioremap on RAM at
0x0000000045623000-0x0000000045623fffWARNING: arch/x86/mm/ioremap.c:216 at __ioremap_caller.isra.0+0x4c2/0x5f0 Call Trace: generic_access_phys+0x130/0x4d0 mm/memory.c:7178 kernfs_vma_access+0x1ce/0x280 fs/kernfs/file.c:437 __access_remote_vm+0x58f/0x890 mm/memory.c:7256 mem_rw+0x2a1/0x670 fs/proc/base.c:912
Quoted 7 lines
But more generically, we should disallow access to anything that looks like ordinary RAM in the system: including allocating memory (e.g.,
alloc_pages) and mapping them in aVM_PFNMAP.For pages with an online memmap, allow only pages marked as reserved (PageReserved). This also includes memblock allocations, however, distinguishing them from other MMIO is tricky, and nobody should really
With ARCH_KEEP_MEMBLOCK it's actually easy ...
Quoted 2 lines
be mapping them in a PFNMAP (except someone abusing
/dev/mem, which we do not care about).
... but yeah, it's not worth it.
Quoted 7 lines
So only allow access if there is no online memmap, or if the pages are marked reserved. Reject other pages with
-EINVAL.Note that, just like other
pfn_to_online_page()users, we won't care about concurrent memory offlining; mapping memory that can concurrently get offlined is highly dubious.
Quoted 7 lines
Reported-by [email protected]Reported-by Nguyen Ngoc Thang <[email protected]>Fixes 28b2ee20c7cb ("access_process_vm device memory infrastructure")Signed-off-by David Hildenbrand (Arm) <[email protected]>
Quoted 3 lines
--- mm/memory.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Signature · 2 lines
Sincerely yours, Mike.
Quoted 31 lines · David Hildenbrand (Arm)
generic_access_phys()can end up getting called on CoW PFNMAP mappings, resulting in it trying to ioremap anonymous folios, which especially the x86 code doesn't enjoy, and which is in general just odd/wrong.E.g., with a CoW mapping of a PCI BAR:
ioremap on RAM at
0x0000000045623000-0x0000000045623fffWARNING: arch/x86/mm/ioremap.c:216 at __ioremap_caller.isra.0+0x4c2/0x5f0 Call Trace: generic_access_phys+0x130/0x4d0 mm/memory.c:7178 kernfs_vma_access+0x1ce/0x280 fs/kernfs/file.c:437 __access_remote_vm+0x58f/0x890 mm/memory.c:7256 mem_rw+0x2a1/0x670 fs/proc/base.c:912But more generically, we should disallow access to anything that looks like ordinary RAM in the system: including allocating memory (e.g.,
alloc_pages) and mapping them in aVM_PFNMAP.For pages with an online memmap, allow only pages marked as reserved (PageReserved). This also includes memblock allocations, however, distinguishing them from other MMIO is tricky, and nobody should really be mapping them in a PFNMAP (except someone abusing
/dev/mem, which we do not care about).So only allow access if there is no online memmap, or if the pages are marked reserved. Reject other pages with
-EINVAL.Note that, just like other
pfn_to_online_page()users, we won't care about concurrent memory offlining; mapping memory that can concurrently get offlined is highly dubious.
Quoted 8 lines
Reported-by [email protected]Reported-by Nguyen Ngoc Thang <[email protected]>Fixes 28b2ee20c7cb ("access_process_vm device memory infrastructure")Signed-off-by David Hildenbrand (Arm) <[email protected]>---
Nice work!