linux-kernel - Re: 2.6.23 regression: accessing invalid mmap'ed memory from gdb causes unkillable spinning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20071101154705.GA745@wotan.suse.de>
Date:	Thu, 1 Nov 2007 16:47:05 +0100
From:	Nick Piggin <npiggin@...e.de>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Duane Griffin <duaneg@...da.com>,
	linux-kernel Mailing List <linux-kernel@...r.kernel.org>,
	stable@...nel.org, Hugh Dickins <hugh@...itas.com>
Subject: Re: 2.6.23 regression: accessing invalid mmap'ed memory from gdb causes unkillable spinning

On Thu, Nov 01, 2007 at 08:14:47AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 1 Nov 2007, Nick Piggin wrote:
> 
> > On Wed, Oct 31, 2007 at 04:08:21PM -0700, Linus Torvalds wrote:
> > > 
> > > We made much bigger changes to ptrace support when we disallowed writing 
> > > to read-only shared memory areas (we used to do the magic per-page COW 
> > > thing).
> > 
> > Really? No, we still do that magic COW thing which creates anonymous
> > pages in MAP_SHARED vmas, don't we?
> 
> No, we don't. I'm pretty sure. It didn't work with the VM cleanups, since 
> the MAP_SHARED vma's won't be on the anonymous list any more, and cannot 
> be swapped out.
> 
> So now, if you try to write to a read-only shared page through ptrace, 
> you'll get "Unable to access".
 
No, it COWs it (the file is RW).

I believe do_wp_page will still attach an anon_vma to the vma, which
will make the pte discoverable via rmap.


> Of course, I didn't really look closely, so maybe I just don't remember 
> things right..
> 
> > > access_vm_pages() (things like core-dumping comes to mind - although I 
> > > think we don't dump pure file mappings at all, do we?) it would certainly 
> > > be good to run any such tests on the current -git tree...
> > 
> > We do for MAP_SHARED|MAP_ANONYMOUS, by the looks.
> 
> Well, as we should. There's no way for a debugger to get those pages back. 
> So that all looks sane.
> 
> > -			vm_flags |= VM_SHARED | VM_MAYSHARE;
> > -			if (!(file->f_mode & FMODE_WRITE))
> > -				vm_flags &= ~(VM_MAYWRITE | VM_SHARED);
> > +			vm_flags |= VM_MAYSHARE;
> > +			if (file->f_mode & FMODE_WRITE)
> > +				vm_flags |= VM_SHARED;
> > +			if (!(vm_flags & VM_WRITE))
> > +				vm_flags &= ~VM_MAYWRITE;
> 
> This looks totally bogus. What was the intent of this patch?
> 
> The VM_MAYWRITE flag is *not* supposed to track the VM_WRITE flag: that 
> would defeat the whole purpose of it! The whole point of that flag is to 
> say whether mprotect() could turn it into a VM_WRITE mapping, and it 
> depends on the file mode, not VM_WRITE!

Yeah of course that won't work, stupid...

The intent is to stop get_user_pages from proceeding with a write fault (and
subsequent COW) to readonly shared mappings, when force is set. I think it
can be done simply via get_user_pages(), which is what I should have done
to begin with.

Untested patch follows
---
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c
+++ linux-2.6/mm/memory.c
@@ -1031,7 +1031,9 @@ int get_user_pages(struct task_struct *t
 		}
 
 		if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP))
-				|| !(vm_flags & vma->vm_flags))
+				|| !(vm_flags & vma->vm_flags)
+				|| (write && ((vma->vm_flags &
+				      (VM_SHARED|VM_MAYSHARE)) == VM_MAYSHARE)))
 			return i ? : -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
@@ -1563,13 +1565,11 @@ static int do_wp_page(struct mm_struct *
 			reuse = can_share_swap_page(old_page);
 			unlock_page(old_page);
 		}
-	} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
-					(VM_WRITE|VM_SHARED))) {
+	} else if (unlikely((vma->vm_flags & VM_SHARED))) {
 		/*
-		 * Only catch write-faults on shared writable pages,
-		 * read-only shared pages can get COWed by
-		 * get_user_pages(.write=1, .force=1).
+		 * Only catch write-faults on shared writable pages.
 		 */
+		BUG_ON(!(vma->vm_flags & VM_WRITE));
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/