linux-kernel - [PATCH v2] ipc/shm: fix the historical/wrong mm->start

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: <20140825191207.GA26106@redhat.com>
Date:	Mon, 25 Aug 2014 21:12:07 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>
Cc:	Manfred Spraul <manfred@...orfullife.com>,
	Davidlohr Bueso <davidlohr.bueso@...com>,
	Kees Cook <keescook@...omium.org>, Tejun Heo <tj@...nel.org>,
	Andrew Vagin <avagin@...nvz.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Julien Tinnes <jln@...gle.com>, linux-kernel@...r.kernel.org
Subject: [PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

The ->start_stack check in do_shmat() looks ugly and simply wrong.

1. ->start_stack is only valid right after exec(), the application
   can switch to another stack and even unmap this area. Or a stack
   can simply grow, ->start_stack won't even notice this.

2. The reason for this check is not clear at all. The application
   should know what it does. And why 4 pages? And why in fact it
   requires 5 pages? Plus "start_stack - size - PAGE_SIZE * 5" can
   underflow although this is minor.

   As Hugh pointed out, we actually need to require the additional
   guard page, but this code was written before linux had it.

3. This wrongly assumes that the stack can only grown down.

Personally I think we should simply kill this check, but I did not
dare to do this. So the patch only fixes the 1st problem (mostly to
avoid the usage of mm->start_stack) and ignores the VM_GROWSUP case.

Signed-off-by: Oleg Nesterov <oleg@...hat.com>
---
 ipc/shm.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 7fc9f9f..9a322f5 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1166,19 +1166,25 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 
 	down_write(&current->mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
-		err = -EINVAL;
-		if (addr + size < addr)
-			goto invalid;
+		struct vm_area_struct *vma;
 
-		if (find_vma_intersection(current->mm, addr, addr + size))
-			goto invalid;
+		err = -EINVAL;
 		/*
-		 * If shm segment goes below stack, make sure there is some
-		 * space left for the stack to grow (at least 4 pages).
+		 * Ensure this segment doesn't overlap with the next vma.
+		 * If it is stack, make sure there is some space left for
+		 * the stack to grow, at least 4 pages plus a guard page
+		 * enforced by check_stack_guard_page(). (Why?)
 		 */
-		if (addr < current->mm->start_stack &&
-		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
-			goto invalid;
+		vma = find_vma(current->mm, addr);
+		if (vma) {
+			unsigned long end = addr + size;
+
+			if (vma->vm_flags & VM_GROWSDOWN)
+				end += PAGE_SIZE * 5;
+
+			if (end < addr || end > vma->vm_start)
+				goto invalid;
+		}
 	}
 
 	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
-- 
1.5.5.1


--
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/