Re: [PATCH] futex: Fix fault_in_user_writeable()

[Peter Zijlstra]

On Tue, Aug 17, 2021 at 09:53:14AM +0800, Huacai Chen wrote:
> Hi, Davidlohr and Thomas,
> 
> On Tue, Aug 17, 2021 at 3:03 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Aug 16 2021 at 11:27, Davidlohr Bueso wrote:
> > > On Mon, 16 Aug 2021, Huacai Chen wrote:
> > >
> > >>fault_in_user_writeable() should verify R/W access but only verify W. In
> > >>most archs W implies R, but not true in MIPS and LoongArch, so fix it.
> > >
> > > Yuck for a find_vma() in futex.c. If this is a problem in MIPS, shouldn't
> > > the fix be there? Furthermore it's stated that fault_in_user_writeable():
> > >
> > > "Fault in user address and verify RW access"
> >
> > That seems to be wishful thinking given the fact that some architectures
> > do not imply R for FLAG_FAULT_WRITE.
> >
> > > And you guys seem to have proposed it already:
> > >
> > > https://lore.kernel.org/linux-mips/20200630005845.1239974-1-liulichao@xxxxxxxxxxx/
> This works, but I don't think this is a MIPS problem, so does Thomas
> Bogendoerfer. Because write-only page is valid in MIPS (so Thomas
> rejected this patch).
> 
> >
> > That's surely one way to fix that. If that does not work for whatever
> > reason, then we really don't want this find_vma() hack there, but rather
> > something like:
> I don't know why find_vma() is unacceptable here, there is also
> find_vma() in fixup_user_fault().
> 
> >
> >     if (IS_ENABLED(CONFIG_ARCH_USER_FAULT_VOODOO) && get_user(&tmp, uaddr))
> >         return -EFAULT;
> get_user() may be better than find_vma(), but can we drop
> CONFIG_ARCH_USER_FAULT_VOODOO here? On those "W implies R" archs,
> get_user() always success, this can simplify the logic.

AFAICT that whole W implies R thing goes much deeper,
mm/gup.c:vma_permits_fault() has:

	vm_flags_t vm_flags = write ? VM_WRITE : VM_READ;

So unless someone wants to go fix the core MM and eradicate all such
assumptions, I'd suggest going with the 'easy' route and fix the arch.

This patch is probably broken and will likely break lots of things...

---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7ca22e6e694a..fc587dbb90b4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -478,6 +478,7 @@ enum fault_flag {
 	FAULT_FLAG_REMOTE =		1 << 7,
 	FAULT_FLAG_INSTRUCTION =	1 << 8,
 	FAULT_FLAG_INTERRUPTIBLE =	1 << 9,
+	FAULT_FLAG_READ =		1 << 10,
 };
 
 /*
diff --git a/kernel/futex.c b/kernel/futex.c
index fcc0570868b7..2c0970759919 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -735,7 +735,7 @@ static int fault_in_user_writeable(u32 __user *uaddr)
 
 	mmap_read_lock(mm);
 	ret = fixup_user_fault(mm, (unsigned long)uaddr,
-			       FAULT_FLAG_WRITE, NULL);
+			       FAULT_FLAG_READ|FAULT_FLAG_WRITE, NULL);
 	mmap_read_unlock(mm);
 
 	return ret < 0 ? ret : 0;
diff --git a/mm/gup.c b/mm/gup.c
index e805c1748bbf..37c8bfbe5196 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1222,11 +1222,17 @@ static long __get_user_pages(struct mm_struct *mm,
 static bool vma_permits_fault(struct vm_area_struct *vma,
 			      unsigned int fault_flags)
 {
+	bool read    = !!(fault_flags & FAULT_FLAG_READ);
 	bool write   = !!(fault_flags & FAULT_FLAG_WRITE);
 	bool foreign = !!(fault_flags & FAULT_FLAG_REMOTE);
-	vm_flags_t vm_flags = write ? VM_WRITE : VM_READ;
+	vm_flags_t vm_flags = 0;
 
-	if (!(vm_flags & vma->vm_flags))
+	if (read)
+		vm_flags |= VM_READ;
+	if (write)
+		vm_flags |= VM_WRITE;
+
+	if ((vma->vm_flags & vm_flags) != vm_flags)
 		return false;
 
 	/*