Re: general protection fault in _vm_normal_page

[Kirill A. Shutemov]

On Wed, Jul 11, 2018 at 02:04:49PM -0700, Andrew Morton wrote:
> On Wed, 11 Jul 2018 09:49:01 -0700 syzbot <syzbot+120abb1c3f7bfdc523f7@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    98be45067040 Add linux-next specific files for 20180711
> > git tree:       linux-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=12496ac2400000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=3f3b3673fec35d01
> > dashboard link: https://syzkaller.appspot.com/bug?extid=120abb1c3f7bfdc523f7
> > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=12a46568400000
> 
> Handy.  /dev/ion from drivers/staging/android/ion/ion.c
> 
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+120abb1c3f7bfdc523f7@xxxxxxxxxxxxxxxxxxxxxxxxx
> > 
> > R10: 0000000004000812 R11: 0000000000000246 R12: 0000000000000005
> > R13: 00000000004c0565 R14: 00000000004cffb0 R15: 0000000000000005
> > ion_mmap: failure mapping buffer to userspace
> > kasan: CONFIG_KASAN_INLINE enabled
> > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > general protection fault: 0000 [#1] SMP KASAN
> > CPU: 0 PID: 4785 Comm: syz-executor0 Not tainted 4.18.0-rc4-next-20180711+  
> > #4
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
> > Google 01/01/2011
> > RIP: 0010:_vm_normal_page+0x1e5/0x330 mm/memory.c:828
> 
> Presumably has a NULL vma->vm_ops.  Probably one of the now-removed
> checks in mm-drop-unneeded-vm_ops-checks.patch would have avoided
> this.
> 
> Something for Kirill to think about ;)
> 

Okay. Looks like we need vm_ops in the error path too :P

Here's the fixup which should help.

I'll post the new version of the patchset once figure out nommu issues.

diff --git a/mm/mmap.c b/mm/mmap.c
index 74d4d2a8fe08..eedac20735c1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1776,6 +1776,11 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		 */
 		vma->vm_file = get_file(file);
 		error = call_mmap(file, vma);
+
+		/* All mappings must have ->vm_ops set */
+		if (!vma->vm_ops)
+			vma->vm_ops = &dummy_vm_ops;
+
 		if (error)
 			goto unmap_and_free_vma;
 
@@ -1788,10 +1793,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		 */
 		WARN_ON_ONCE(addr != vma->vm_start);
 
-		/* All mappings must have ->vm_ops set */
-		if (!vma->vm_ops)
-			vma->vm_ops = &dummy_vm_ops;
-
 		addr = vma->vm_start;
 		vm_flags = vma->vm_flags;
 	} else if (vm_flags & VM_SHARED) {
-- 
 Kirill A. Shutemov