On 15/10/2024 19:26, Jann Horn wrote:
If some remap_pfn_range() calls succeeded before one failed, we still have
buffer pages mapped into the userspace page tables when we drop the buffer
reference with comedi_buf_map_put(bm). The userspace mappings are only
cleaned up later in the mmap error path.
Fix it by explicitly flushing all mappings in our VMA on the error path.
See commit 79a61cc3fc04 ("mm: avoid leaving partial pfn mappings around in
error case").
Cc: stable@xxxxxxxxxxxxxxx
Fixes: ed9eccbe8970 ("Staging: add comedi core")
Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
---
Note: compile-tested only; I don't actually have comedi hardware, and I
don't know anything about comedi.
It is possible to test it by loading the comedi_test driver module,
which creates a comedi device in software. But then you need to induce
an error somehow. I did it by hacking the comedi_mmap() function to
break the loop that calls remap_pfn_range() early with an error.
---
Changes in v2:
- only do the zapping in the pfnmap path (Ian Abbott)
- use zap_vma_ptes() instead of zap_page_range_single() (Ian Abbott)
- Link to v1: https://lore.kernel.org/r/20241014-comedi-tlb-v1-1-4b699144b438@xxxxxxxxxx
---
drivers/comedi/comedi_fops.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 1b481731df96..68e5301e6281 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -2407,6 +2407,16 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
start += PAGE_SIZE;
}
+
+ /*
+ * Leaving behind a partial mapping of a buffer we're about to
+ * drop is unsafe, see remap_pfn_range_notrack().
+ * We need to zap the range here ourselves instead of relying
+ * on the automatic zapping in remap_pfn_range() because we call
+ * remap_pfn_range() in a loop.
+ */
+ if (retval)
+ zap_vma_ptes(vma, vma->vm_start, size);
}
if (retval == 0) {
---
base-commit: 6485cf5ea253d40d507cd71253c9568c5470cd27
change-id: 20241014-comedi-tlb-400246505961
I have tested it with the device created by the comedi_test.ko module
using the comedi_test application from comedilib, by artificially
breaking the partial mapping loop with an error half way to completion.
I haven't noticed any problems from the zap_vma_ptes() call, so it is OK
as far as I can tell. (I don't really have any experience of calling
functions like zap_vma_ptes().)
I note that there are a bunch of drivers in drivers/video/fbdev that
also call remap_pfn_range() or io_remap_pfn_range() in a loop that
probably require your attention!
Tested-by: Ian Abbott <abbotti@xxxxxxxxx>
Reviewed-by: Ian Abbott <abbotti@xxxxxxxxx>
--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
