Re: [PATCH] ocfs2: fix use-after-free in ocfs2_fault() when VM_FAULT_RETRY

[tejas bharambe]

Hi Joseph, 

That is nice.  
Did the following:
1. Using inode
2. Removed vma (had to update the ocfs2_trace.h as well)

Thank you for the suggestion

Please find v2 below:

filemap_fault() may drop the mmap_lock before returning VM_FAULT_RETRY,
as documented in mm/filemap.c:

  "If our return value has VM_FAULT_RETRY set, it's because the mmap_lock
  may be dropped before doing I/O or by lock_folio_maybe_drop_mmap()."

When this happens, a concurrent munmap() can call remove_vma() and free
the vm_area_struct via RCU. The saved 'vma' pointer in ocfs2_fault() then
becomes a dangling pointer, and the subsequent trace_ocfs2_fault() call
dereferences it -- a use-after-free.

Fix this by saving the inode reference before calling filemap_fault(),
and removing vma from the trace event. The inode remains valid across
the lock drop since the file is still open, so the trace can fire in
all cases without dereferencing the potentially freed vma.

Reported-by: syzbot+a49010a0e8fcdeea075f@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=a49010a0e8fcdeea075f
Suggested-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
Signed-off-by: Tejas Bharambe <tejas.bharambe@xxxxxxxxxxx>
---
 fs/ocfs2/mmap.c        | 14 +++-----------
 fs/ocfs2/ocfs2_trace.h | 10 ++++------
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/ocfs2/mmap.c b/fs/ocfs2/mmap.c
index adf6423ae9..41c08c5a3d 100644
--- a/fs/ocfs2/mmap.c
+++ b/fs/ocfs2/mmap.c
@@ -30,7 +30,7 @@
 
 static vm_fault_t ocfs2_fault(struct vm_fault *vmf)
 {
-	struct vm_area_struct *vma = vmf->vma;
+	struct inode *inode = file_inode(vmf->vma->vm_file);
 	sigset_t oldset;
 	vm_fault_t ret;
 
@@ -38,16 +38,8 @@ static vm_fault_t ocfs2_fault(struct vm_fault *vmf)
 	ret = filemap_fault(vmf);
 	ocfs2_unblock_signals(&oldset);
 
-	/*
-	 * filemap_fault() may drop the mmap_lock and return VM_FAULT_RETRY.
-	 * In that case the vma may have been freed by a concurrent munmap(),
-	 * so we must not dereference it.
-	 */
-	if (ret & VM_FAULT_RETRY)
-		return ret;
-
-	trace_ocfs2_fault(OCFS2_I(vma->vm_file->f_mapping->host)->ip_blkno,
-			  vma, vmf->page, vmf->pgoff);
+	trace_ocfs2_fault(OCFS2_I(inode)->ip_blkno,
+			  vmf->page, vmf->pgoff);
 	return ret;
 }
 
diff --git a/fs/ocfs2/ocfs2_trace.h b/fs/ocfs2/ocfs2_trace.h
index 4b32fb5658..6c2c97a980 100644
--- a/fs/ocfs2/ocfs2_trace.h
+++ b/fs/ocfs2/ocfs2_trace.h
@@ -1246,22 +1246,20 @@ TRACE_EVENT(ocfs2_write_end_inline,
 
 TRACE_EVENT(ocfs2_fault,
 	TP_PROTO(unsigned long long ino,
-		 void *area, void *page, unsigned long pgoff),
-	TP_ARGS(ino, area, page, pgoff),
+		 void *page, unsigned long pgoff),
+	TP_ARGS(ino, page, pgoff),
 	TP_STRUCT__entry(
 		__field(unsigned long long, ino)
-		__field(void *, area)
 		__field(void *, page)
 		__field(unsigned long, pgoff)
 	),
 	TP_fast_assign(
 		__entry->ino = ino;
-		__entry->area = area;
 		__entry->page = page;
 		__entry->pgoff = pgoff;
 	),
-	TP_printk("%llu %p %p %lu",
-		  __entry->ino, __entry->area, __entry->page, __entry->pgoff)
+	TP_printk("%llu %p %lu",
+		  __entry->ino, __entry->page, __entry->pgoff)
 );
 
 /* End of trace events for fs/ocfs2/mmap.c. */
-- 
2.53.0
________________________________________
From: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
Sent: Tuesday, March 31, 2026 2:55 AM
To: tejas bharambe <tejas.bharambe@xxxxxxxxxxx>
Cc: mark@xxxxxxxxxx <mark@xxxxxxxxxx>; jlbec@xxxxxxxxxxxx <jlbec@xxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; syzbot+a49010a0e8fcdeea075f@xxxxxxxxxxxxxxxxxxxxxxxxx <syzbot+a49010a0e8fcdeea075f@xxxxxxxxxxxxxxxxxxxxxxxxx>; Heming Zhao <heming.zhao@xxxxxxxx>; ocfs2-devel@xxxxxxxxxxxxxxx <ocfs2-devel@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] ocfs2: fix use-after-free in ocfs2_fault() when VM_FAULT_RETRY
 


On 3/31/26 2:20 PM, tejas bharambe wrote:
> filemap_fault() may drop the mmap_lock before returning VM_FAULT_RETRY,
> as documented in mm/filemap.c:
>
>   "If our return value has VM_FAULT_RETRY set, it's because the mmap_lock
>   may be dropped before doing I/O or by lock_folio_maybe_drop_mmap()."
>
> When this happens, a concurrent munmap() can call remove_vma() and free
> the vm_area_struct via RCU. The saved 'vma' pointer in ocfs2_fault() then
> becomes a dangling pointer, and the subsequent trace_ocfs2_fault() call
> dereferences it -- a use-after-free.
>
> Fix this by returning early when VM_FAULT_RETRY is set, skipping the
> trace that dereferences the potentially freed vma.
>
> Reported-by: syzbot+a49010a0e8fcdeea075f@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=a49010a0e8fcdeea075f
> Signed-off-by: Tejas Bharambe <tejas.bharambe@xxxxxxxxxxx>
> ---
>  fs/ocfs2/mmap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/fs/ocfs2/mmap.c b/fs/ocfs2/mmap.c
> index 50e2faf64c..adf6423ae9 100644
> --- a/fs/ocfs2/mmap.c
> +++ b/fs/ocfs2/mmap.c
> @@ -38,6 +38,14 @@ static vm_fault_t ocfs2_fault(struct vm_fault *vmf)
>        ret = filemap_fault(vmf);
>        ocfs2_unblock_signals(&oldset);
> 
> +     /*
> +      * filemap_fault() may drop the mmap_lock and return VM_FAULT_RETRY.
> +      * In that case the vma may have been freed by a concurrent munmap(),
> +      * so we must not dereference it.
> +      */
> +     if (ret & VM_FAULT_RETRY)
> +             return ret;
> +

How about change it like this:

Remove vma from ocfs2_fault trace event. Then

struct inode *inode = file_inode(vmf->vma->vm_file);
...
ret = filemap_fault(vmf);
...
trace_ocfs2_fault(OCFS2_I(inode)->ip_blkno, vmf->page, vmf->pgoff);

This can keep trace event in all cases.

Thanks,
Joseph

>        trace_ocfs2_fault(OCFS2_I(vma->vm_file->f_mapping->host)->ip_blkno,
>                          vma, vmf->page, vmf->pgoff);
>        return ret;