Re: concurrent msync triggers NULL pointer dereference

[Ben H. <be...@de...>]

On Thu, 2015-09-03 at 01:21 +0900, sf...@us... wrote:
> sf...@us...:
> >
> > Xavier Chantry:
> > > I can reproduce the bug on all my machines, so it shouldn't be too 
> > > difficult to reproduce. Applying the following hunk is enough to trigger 
> > > the bug.
> >
> > Unfortunately still I don't have enough time.
> > I understand this bug has a big impact for you and any other user who
> > have applied aufs3-mmap.patch. As soon as I get the time, I'm sure I
> > will dive into this problem.
> 
> I've thought about the problem a little (still I'm busy).
> It looks like a race condition around mm->mmap_sem.
> 
> -	get_file(file);
> +	vma_get_file(vma);
> 	up_read(&mm->mmap_sem);
> 	error = vfs_fsync(file, 0);
> -	fput(file);
> +	vma_fput(vma);
> 	if (error || start >= end)
> 		goto out;
> 	down_read(&mm->mmap_sem);
> 
> Since this fput/vma_fput is between up(mmap_sem) and down(mmap_sem),
> vma_fput() is not protected, and got vm_file NULL unexpectedly. It
> means vma->vm_file is changed (or being changed) by someone else (other
> thread in your case) after up_read(&mm->mmap_sem). This is surely a bug
> in aufs[34]-mmap.patch. It shoule be done such like this essentially.
> 	get_file(file);
> 	pr = vma->vm_prfile;
> 		;;;
> 	fput(file);
> 	if (pr)
> 		fput(pr);
> 
> The fix will look a little smarter than above.
> I hope I can post the fix in a few weeks.

I don't see any need for holding a reference to vma->vm_prfile here.

There is also a similar bug in madvise_remove() which I can trigger by
calling madvise(..., MADV_REMOVE) in parallel with another thread that
does mmap() and munmap() of the same address range.

This patch against 3.16 fixes both for me:

From: Ben Hutchings <be...@de...>
Date: Thu, 10 Sep 2015 02:19:59 +0100
Subject: aufs3: mmap: Fix races in madvise_remove() and sys_msync()
Bug-Debian: https://bugs.debian.org/796036

In madvise_remove() and sys_msync() we drop the mmap_sem before
dropping references to the mapped file(s).  As soon as we drop the
mmap_sem, the vma we got them from might be destroyed by another
thread, so calling vma_do_fput() is a possible use-after-free.

In these cases we don't actually need a reference to the aufs file, so
revert to using get_file() and fput() directly.

Signed-off-by: Ben Hutchings <be...@de...>
---
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -327,12 +327,12 @@ static long madvise_remove(struct vm_are
 	 * vma's reference to the file) can go away as soon as we drop
 	 * mmap_sem.
 	 */
-	vma_get_file(vma);
+	get_file(f);
 	up_read(&current->mm->mmap_sem);
 	error = do_fallocate(f,
 				FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
 				offset, end - start);
-	vma_fput(vma);
+	fput(f);
 	down_read(&current->mm->mmap_sem);
 	return error;
 }
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -84,13 +84,13 @@ SYSCALL_DEFINE3(msync, unsigned long, st
 		start = vma->vm_end;
 		if ((flags & MS_SYNC) && file &&
 				(vma->vm_flags & VM_SHARED)) {
-			vma_get_file(vma);
+			get_file(file);
 			up_read(&mm->mmap_sem);
 			if (vma->vm_flags & VM_NONLINEAR)
 				error = vfs_fsync(file, 1);
 			else
 				error = vfs_fsync_range(file, fstart, fend, 1);
-			vma_fput(vma);
+			fput(file);
 			if (error || start >= end)
 				goto out;
 			down_read(&mm->mmap_sem);
--- END ---

This bug has some security impact (at least a minor DoS, but possible
privilege escalation) so I'm going to request a CVE ID for it.

Ben.