Attachment 727764 Details for Bug 1039348

[patch] fix candidate v4

0001-mm-enlarge-stack-guard-gap.patch (text/plain), 16.42 KB, created by Michal Hocko on 2017-06-06 08:05:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Michal Hocko

Created: 2017-06-06 08:05:58 UTC

Size: 16.42 KB

patch

obsolete

>From a4e6c0875b904ea49f825d7b32f52fd7286fb1f8 Mon Sep 17 00:00:00 2001
>From: Michal Hocko <mhocko@suse.com>
>Date: Fri, 12 May 2017 09:09:30 +0200
>Subject: [PATCH 1/2] mm: enlarge stack guard gap
>
>Stack guard page is a useful feature to reduce a risk of stack smashing
>into a different mapping. We have been using a single page gap which
>is sufficient to prevent having stack adjacent to a different mapping.
>But this seems to be insufficient in the light of the stack usage in
>the userspace. E.g. glibc uses as large as 64kB alloca() in many
>commonly used functions. This will become especially dangerous for suid
>binaries and the default no limit for the stack size limit because those
>applications can be tricked to consume a large portion of the stack and
>a single glibc call could jump over the guard page. These attacks are
>not theoretical, unfortunatelly.
>
>Make those attacks less probable by increasing the stack guard gap
>to 1MB (on systems with 4k pages but make it depend on the page size
>because systems with larger base pages might cap stack allocations in
>the PAGE_SIZE units) which should cover larger alloca() and VLA stack
>allocations. It is obviously not a full fix because the problem is
>somehow inherent but it should reduce attack space a lot. One could
>argue that the gap size should be configurable from the userspace but
>that can be done later on top when somebody finds that the new 1MB is
>not suitable or even wrong for some special case applications.
>
>Implementation wise, get rid of check_stack_guard_page and move all the
>guard page specific code to expandable_stack_area which always tries to
>guarantee the gap. do_anonymous_page then just calls expand_stack. Also
>get rid of stack_guard_page_{start,end} and replace them with
>stack_guard_area to handle stack population and /proc/<pid>/[s]maps.
>
>This should clean up the code which is quite scattered currently
>and therefore justify the change.
>
>TODO: ia64 page fault handling calls expand_upwards explicitly for
>register store. Do we need a gap there as well?
>
>Signed-off-by: Michal Hocko <mhocko@suse.com>
>---
> arch/ia64/mm/fault.c |   2 +-
> fs/exec.c            |   8 ++-
> fs/proc/task_mmu.c   |  11 ++--
> include/linux/mm.h   |  40 +++-----------
> mm/gup.c             |   4 +-
> mm/memory.c          |  35 +-----------
> mm/mmap.c            | 152 +++++++++++++++++++++++++++++++++++++++++----------
> 7 files changed, 150 insertions(+), 102 deletions(-)
>
>diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
>index 15f09cfff335..4b25036775b7 100644
>--- a/arch/ia64/mm/fault.c
>+++ b/arch/ia64/mm/fault.c
>@@ -226,7 +226,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
> 		 */
> 		if (address > vma->vm_end + PAGE_SIZE - sizeof(long))
> 			goto bad_area;
>-		if (expand_upwards(vma, address))
>+		if (expand_upwards(vma, address, 0))
> 			goto bad_area;
> 	}
> 	goto good_area;
>diff --git a/fs/exec.c b/fs/exec.c
>index 72934df68471..ad1f50c0c6bf 100644
>--- a/fs/exec.c
>+++ b/fs/exec.c
>@@ -200,7 +200,7 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> 
> #ifdef CONFIG_STACK_GROWSUP
> 	if (write) {
>-		ret = expand_downwards(bprm->vma, pos);
>+		ret = expand_downwards(bprm->vma, pos, 0);
> 		if (ret < 0)
> 			return NULL;
> 	}
>@@ -222,6 +222,12 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> 		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
> 		struct rlimit *rlim;
> 
>+		/*
>+		 * GRWOSUP doesn't really have any gap at this stage because we grow
>+		 * the stack down now. See the expand_downwards above.
>+		 */
>+		if (!IS_ENABLED(CONFIG_STACK_GROWSUP))
>+			size -= stack_guard_gap;
> 		acct_arg_size(bprm, size / PAGE_SIZE);
> 
> 		/*
>diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>index f0c8b33d99b1..ddba869e94b5 100644
>--- a/fs/proc/task_mmu.c
>+++ b/fs/proc/task_mmu.c
>@@ -300,11 +300,14 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
> 
> 	/* We don't show the stack guard page in /proc/maps */
> 	start = vma->vm_start;
>-	if (stack_guard_page_start(vma, start))
>-		start += PAGE_SIZE;
> 	end = vma->vm_end;
>-	if (stack_guard_page_end(vma, end))
>-		end -= PAGE_SIZE;
>+	if (vma->vm_flags & VM_GROWSDOWN) {
>+		if (stack_guard_area(vma, start))
>+			start += stack_guard_gap;
>+	} else if (vma->vm_flags & VM_GROWSUP) {
>+		if (stack_guard_area(vma, end))
>+			end -= stack_guard_gap;
>+	}
> 
> 	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
> 	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
>diff --git a/include/linux/mm.h b/include/linux/mm.h
>index 7cb17c6b97de..ed642c7fa251 100644
>--- a/include/linux/mm.h
>+++ b/include/linux/mm.h
>@@ -1393,12 +1393,6 @@ int clear_page_dirty_for_io(struct page *page);
> 
> int get_cmdline(struct task_struct *task, char *buffer, int buflen);
> 
>-/* Is the vma a continuation of the stack vma above it? */
>-static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long addr)
>-{
>-	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
>-}
>-
> static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> {
> 	return !vma->vm_ops;
>@@ -1414,28 +1408,6 @@ bool vma_is_shmem(struct vm_area_struct *vma);
> static inline bool vma_is_shmem(struct vm_area_struct *vma) { return false; }
> #endif
> 
>-static inline int stack_guard_page_start(struct vm_area_struct *vma,
>-					     unsigned long addr)
>-{
>-	return (vma->vm_flags & VM_GROWSDOWN) &&
>-		(vma->vm_start == addr) &&
>-		!vma_growsdown(vma->vm_prev, addr);
>-}
>-
>-/* Is the vma a continuation of the stack vma below it? */
>-static inline int vma_growsup(struct vm_area_struct *vma, unsigned long addr)
>-{
>-	return vma && (vma->vm_start == addr) && (vma->vm_flags & VM_GROWSUP);
>-}
>-
>-static inline int stack_guard_page_end(struct vm_area_struct *vma,
>-					   unsigned long addr)
>-{
>-	return (vma->vm_flags & VM_GROWSUP) &&
>-		(vma->vm_end == addr) &&
>-		!vma_growsup(vma->vm_next, addr);
>-}
>-
> int vma_is_stack_for_current(struct vm_area_struct *vma);
> 
> extern unsigned long move_page_tables(struct vm_area_struct *vma,
>@@ -2222,16 +2194,22 @@ void page_cache_async_readahead(struct address_space *mapping,
> 				pgoff_t offset,
> 				unsigned long size);
> 
>+extern unsigned long stack_guard_gap;
> /* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */
> extern int expand_stack(struct vm_area_struct *vma, unsigned long address);
>+extern int stack_guard_area(struct vm_area_struct *vma, unsigned long address);
> 
> /* CONFIG_STACK_GROWSUP still needs to to grow downwards at some places */
> extern int expand_downwards(struct vm_area_struct *vma,
>-		unsigned long address);
>+		unsigned long address, unsigned long gap);
>+unsigned long expandable_stack_area(struct vm_area_struct *vma,
>+		unsigned long address, unsigned long *gap);
>+
> #if VM_GROWSUP
>-extern int expand_upwards(struct vm_area_struct *vma, unsigned long address);
>+extern int expand_upwards(struct vm_area_struct *vma,
>+		unsigned long address, unsigned long gap);
> #else
>-  #define expand_upwards(vma, address) (0)
>+  #define expand_upwards(vma, address, gap) (0)
> #endif
> 
> /* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */
>diff --git a/mm/gup.c b/mm/gup.c
>index d9e6fddcc51f..175569638bdd 100644
>--- a/mm/gup.c
>+++ b/mm/gup.c
>@@ -388,9 +388,7 @@ static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma,
> 	if ((*flags & (FOLL_POPULATE | FOLL_MLOCK)) == FOLL_MLOCK)
> 		return -ENOENT;
> 	/* For mm_populate(), just skip the stack guard page. */
>-	if ((*flags & FOLL_POPULATE) &&
>-			(stack_guard_page_start(vma, address) ||
>-			 stack_guard_page_end(vma, address + PAGE_SIZE)))
>+	if ((*flags & FOLL_POPULATE) && stack_guard_area(vma, address))
> 		return -ENOENT;
> 	if (*flags & FOLL_WRITE)
> 		fault_flags |= FAULT_FLAG_WRITE;
>diff --git a/mm/memory.c b/mm/memory.c
>index 6ff5d729ded0..9e1489b0536b 100644
>--- a/mm/memory.c
>+++ b/mm/memory.c
>@@ -2854,39 +2854,7 @@ int do_swap_page(struct vm_fault *vmf)
> 	return ret;
> }
> 
>-/*
>- * This is like a special single-page "expand_{down|up}wards()",
>- * except we must first make sure that 'address{-|+}PAGE_SIZE'
>- * doesn't hit another vma.
>- */
>-static inline int check_stack_guard_page(struct vm_area_struct *vma, unsigned long address)
>-{
>-	address &= PAGE_MASK;
>-	if ((vma->vm_flags & VM_GROWSDOWN) && address == vma->vm_start) {
>-		struct vm_area_struct *prev = vma->vm_prev;
>-
>-		/*
>-		 * Is there a mapping abutting this one below?
>-		 *
>-		 * That's only ok if it's the same stack mapping
>-		 * that has gotten split..
>-		 */
>-		if (prev && prev->vm_end == address)
>-			return prev->vm_flags & VM_GROWSDOWN ? 0 : -ENOMEM;
> 
>-		return expand_downwards(vma, address - PAGE_SIZE);
>-	}
>-	if ((vma->vm_flags & VM_GROWSUP) && address + PAGE_SIZE == vma->vm_end) {
>-		struct vm_area_struct *next = vma->vm_next;
>-
>-		/* As VM_GROWSDOWN but s/below/above/ */
>-		if (next && next->vm_start == address + PAGE_SIZE)
>-			return next->vm_flags & VM_GROWSUP ? 0 : -ENOMEM;
>-
>-		return expand_upwards(vma, address + PAGE_SIZE);
>-	}
>-	return 0;
>-}
> 
> /*
>  * We enter with non-exclusive mmap_sem (to exclude vma changes,
>@@ -2905,7 +2873,8 @@ static int do_anonymous_page(struct vm_fault *vmf)
> 		return VM_FAULT_SIGBUS;
> 
> 	/* Check if we need to add a guard page to the stack */
>-	if (check_stack_guard_page(vma, vmf->address) < 0)
>+	if ((vma->vm_flags & (VM_GROWSDOWN|VM_GROWSUP)) &&
>+			expand_stack(vma, vmf->address) < 0)
> 		return VM_FAULT_SIGSEGV;
> 
> 	/*
>diff --git a/mm/mmap.c b/mm/mmap.c
>index f82741e199c0..d1582ed482ac 100644
>--- a/mm/mmap.c
>+++ b/mm/mmap.c
>@@ -2155,7 +2155,8 @@ find_vma_prev(struct mm_struct *mm, unsigned long addr,
>  * update accounting. This is shared with both the
>  * grow-up and grow-down cases.
>  */
>-static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, unsigned long grow)
>+static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, unsigned long grow,
>+		unsigned long gap)
> {
> 	struct mm_struct *mm = vma->vm_mm;
> 	struct rlimit *rlim = current->signal->rlim;
>@@ -2168,7 +2169,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
> 	/* Stack limit test */
> 	actual_size = size;
> 	if (size && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN)))
>-		actual_size -= PAGE_SIZE;
>+		actual_size -= gap;
> 	if (actual_size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur))
> 		return -ENOMEM;
> 
>@@ -2204,7 +2205,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
>  * PA-RISC uses this for its stack; IA64 for its Register Backing Store.
>  * vma is the last one with address > vma->vm_end.  Have to extend vma.
>  */
>-int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>+int expand_upwards(struct vm_area_struct *vma, unsigned long address, unsigned long gap)
> {
> 	struct mm_struct *mm = vma->vm_mm;
> 	int error = 0;
>@@ -2212,12 +2213,6 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> 	if (!(vma->vm_flags & VM_GROWSUP))
> 		return -EFAULT;
> 
>-	/* Guard against wrapping around to address 0. */
>-	if (address < PAGE_ALIGN(address+4))
>-		address = PAGE_ALIGN(address+4);
>-	else
>-		return -ENOMEM;
>-
> 	/* We must make sure the anon_vma is allocated. */
> 	if (unlikely(anon_vma_prepare(vma)))
> 		return -ENOMEM;
>@@ -2238,7 +2233,7 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> 
> 		error = -ENOMEM;
> 		if (vma->vm_pgoff + (size >> PAGE_SHIFT) >= vma->vm_pgoff) {
>-			error = acct_stack_growth(vma, size, grow);
>+			error = acct_stack_growth(vma, size, grow, gap);
> 			if (!error) {
> 				/*
> 				 * vma_gap_update() doesn't support concurrent
>@@ -2279,7 +2274,7 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>  * vma is the first one with address < vma->vm_start.  Have to extend vma.
>  */
> int expand_downwards(struct vm_area_struct *vma,
>-				   unsigned long address)
>+				   unsigned long address, unsigned long gap)
> {
> 	struct mm_struct *mm = vma->vm_mm;
> 	int error;
>@@ -2309,7 +2304,7 @@ int expand_downwards(struct vm_area_struct *vma,
> 
> 		error = -ENOMEM;
> 		if (grow <= vma->vm_pgoff) {
>-			error = acct_stack_growth(vma, size, grow);
>+			error = acct_stack_growth(vma, size, grow, gap);
> 			if (!error) {
> 				/*
> 				 * vma_gap_update() doesn't support concurrent
>@@ -2343,29 +2338,72 @@ int expand_downwards(struct vm_area_struct *vma,
> 	return error;
> }
> 
>+/* enforced gap between the expanding stack and other mappings. */
>+unsigned long stack_guard_gap = 256UL<<PAGE_SHIFT;
>+
> /*
>  * Note how expand_stack() refuses to expand the stack all the way to
>  * abut the next virtual mapping, *unless* that mapping itself is also
>- * a stack mapping. We want to leave room for a guard page, after all
>+ * a stack mapping. We want to leave room for a guard area, after all
>  * (the guard page itself is not added here, that is done by the
>  * actual page faulting logic)
>- *
>- * This matches the behavior of the guard page logic (see mm/memory.c:
>- * check_stack_guard_page()), which only allows the guard page to be
>- * removed under these circumstances.
>  */
> #ifdef CONFIG_STACK_GROWSUP
>+unsigned long expandable_stack_area(struct vm_area_struct *vma,
>+		unsigned long address, unsigned long *gap)
>+{
>+	struct vm_area_struct *next = vma->vm_next;
>+	unsigned long guard_gap = stack_guard_gap;
>+	unsigned long guard_addr;
>+
>+	address = ALIGN(address, PAGE_SIZE);;
>+	if (!next)
>+		goto out;
>+
>+	if (next->vm_flags & VM_GROWSUP) {
>+		guard_gap = min(guard_gap, next->vm_start - address);
>+		goto out;
>+	}
>+
>+	if (next->vm_start - address < guard_gap)
>+		return -ENOMEM;
>+out:
>+	if (TASK_SIZE - address < guard_gap)
>+		guard_gap = TASK_SIZE - address;
>+	guard_addr = address + guard_gap;
>+	*gap = guard_gap;
>+
>+	return guard_addr;
>+}
>+
> int expand_stack(struct vm_area_struct *vma, unsigned long address)
> {
>+	unsigned long gap;
>+
>+	address = expandable_stack_area(vma, address, &gap);
>+	if (IS_ERR_VALUE(address))
>+		return -ENOMEM;
>+	return expand_upwards(vma, address, gap);
>+}
>+
>+int stack_guard_area(struct vm_area_struct *vma, unsigned long address)
>+{
> 	struct vm_area_struct *next;
> 
>-	address &= PAGE_MASK;
>+	if (!(vma->vm_flags & VM_GROWSUP))
>+		return 0;
>+
>+	/*
>+	 * strictly speaking there is a guard gap between disjoint stacks
>+	 * but the gap is not canonical (it might be smaller) and it is
>+	 * reasonably safe to assume that we can ignore that gap for stack
>+	 * POPULATE or /proc/<pid>[s]maps purposes
>+	 */
> 	next = vma->vm_next;
>-	if (next && next->vm_start == address + PAGE_SIZE) {
>-		if (!(next->vm_flags & VM_GROWSUP))
>-			return -ENOMEM;
>-	}
>-	return expand_upwards(vma, address);
>+	if (next && next->vm_flags & VM_GROWSUP)
>+		return 0;
>+
>+	return vma->vm_end - address <= stack_guard_gap;
> }
> 
> struct vm_area_struct *
>@@ -2384,17 +2422,73 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
> 	return prev;
> }
> #else
>+unsigned long expandable_stack_area(struct vm_area_struct *vma,
>+		unsigned long address, unsigned long *gap)
>+{
>+	struct vm_area_struct *prev = vma->vm_prev;
>+	unsigned long guard_gap = stack_guard_gap;
>+	unsigned long guard_addr;
>+
>+	address &= PAGE_MASK;
>+	if (!prev)
>+		goto out;
>+
>+	/*
>+	 * Is there a mapping abutting this one below?
>+	 *
>+	 * That's only ok if it's the same stack mapping
>+	 * that has gotten split or there is sufficient gap
>+	 * between mappings
>+	 */
>+	if (prev->vm_flags & VM_GROWSDOWN) {
>+		guard_gap = min(guard_gap, address - prev->vm_end);
>+		goto out;
>+	}
>+
>+	if (address - prev->vm_end < guard_gap)
>+		return -ENOMEM;
>+
>+out:
>+	/* make sure we won't underflow */
>+	if (address < mmap_min_addr)
>+		return -ENOMEM;
>+	if (address - mmap_min_addr < guard_gap)
>+		guard_gap = address - mmap_min_addr;
>+
>+	guard_addr = address - guard_gap;
>+	*gap = guard_gap;
>+
>+	return guard_addr;
>+}
>+
> int expand_stack(struct vm_area_struct *vma, unsigned long address)
> {
>+	unsigned long gap;
>+
>+	address = expandable_stack_area(vma, address, &gap);
>+	if (IS_ERR_VALUE(address))
>+		return -ENOMEM;
>+	return expand_downwards(vma, address, gap);
>+}
>+
>+int stack_guard_area(struct vm_area_struct *vma, unsigned long address)
>+{
> 	struct vm_area_struct *prev;
> 
>-	address &= PAGE_MASK;
>+	if (!(vma->vm_flags & VM_GROWSDOWN))
>+		return 0;
>+
>+	/*
>+	 * strictly speaking there is a guard gap between disjoint stacks
>+	 * but the gap is not canonical (it might be smaller) and it is
>+	 * reasonably safe to assume that we can ignore that gap for stack
>+	 * POPULATE or /proc/<pid>[s]maps purposes
>+	 */
> 	prev = vma->vm_prev;
>-	if (prev && prev->vm_end == address) {
>-		if (!(prev->vm_flags & VM_GROWSDOWN))
>-			return -ENOMEM;
>-	}
>-	return expand_downwards(vma, address);
>+	if (prev && prev->vm_flags & VM_GROWSDOWN)
>+		return 0;
>+
>+	return address - vma->vm_start < stack_guard_gap;
> }
> 
> struct vm_area_struct *
>-- 
>2.11.0
>

Actions: View | Diff

Attachments on bug 1039348: 725338 | 727600 | 727601 | 727627 | 727764 | 727765 | 729853