linux-kernel - Re: [PATCH v3] ipc/shm: kill 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] [day] [month] [year] [list]

Message-ID: <alpine.LSU.2.11.1408261316560.19965@eggly.anvils>
Date:	Tue, 26 Aug 2014 13:28:15 -0700 (PDT)
From:	Hugh Dickins <hughd@...gle.com>
To:	Oleg Nesterov <oleg@...hat.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	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: Re: [PATCH v3] ipc/shm: kill the historical/wrong mm->start_stack
 check

On Tue, 26 Aug 2014, Oleg Nesterov wrote:

> do_shmat() is the only user of ->start_stack (proc just reports its
> value), and this check looks ugly and wrong.
> 
> The reason for this check is not clear at all, and it wrongly assumes
> that the stack can only grow down.
> 
> But the main problem is that in general mm->start_stack has nothing
> to do with stack_vma->vm_start. Not only the application can switch
> to another stack and even unmap this area, setup_arg_pages() expands
> the stack without updating mm->start_stack during exec(). This means
> that in the likely case "addr > start_stack - size - PAGE_SIZE * 5"
> is simply impossible after find_vma_intersection() == F, or the stack
> can't grow anyway because of RLIMIT_STACK.
> 
> Many thanks to Hugh for his explanations.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>

Acked-by: Hugh Dickins <hughd@...gle.com>

But you're much too generous to me: I never even noticed how exec's
expand_stack() comes *after* setting start_stack, so that the shmat()
check is nowadays utterly irrelevant.  Thank you for persisting and
observing that and finally nailing down this coffin!

> ---
>  ipc/shm.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7fc9f9f..0145479 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1172,13 +1172,6 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  
>  		if (find_vma_intersection(current->mm, addr, addr + size))
>  			goto invalid;
> -		/*
> -		 * If shm segment goes below stack, make sure there is some
> -		 * space left for the stack to grow (at least 4 pages).
> -		 */
> -		if (addr < current->mm->start_stack &&
> -		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> -			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/