From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754520AbdGJP7t (ORCPT ); Mon, 10 Jul 2017 11:59:49 -0400 Received: from mx2.suse.de ([195.135.220.15]:35259 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754322AbdGJP7s (ORCPT ); Mon, 10 Jul 2017 11:59:48 -0400 Date: Mon, 10 Jul 2017 17:59:44 +0200 From: Michal Hocko To: Kees Cook Cc: Linus Torvalds , Andy Lutomirski , "Eric W. Biederman" , Ben Hutchings , Hugh Dickins , Oleg Nesterov , "Jason A. Donenfeld" , Rik van Riel , LKML Subject: Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Message-ID: <20170710155943.GA7071@dhcp22.suse.cz> References: <20170707185729.GA70967@beast> <20170710131342.GI19185@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 10-07-17 08:39:43, Kees Cook wrote: > On Mon, Jul 10, 2017 at 6:13 AM, Michal Hocko wrote: > > I am not sure whether this is still actual because there are just too > > many pathes flying around these days. I am still trying to catch up... > > Linus applied this one, yes. Hmm, this is rather rushed... > > On Fri 07-07-17 11:57:29, Kees Cook wrote: > >> To avoid pathological stack usage or the need to special-case setuid > >> execs, just limit all arg stack usage to at most _STK_LIM / 4 * 3 (6MB). > > > > I am worried that we've grown users which rely on a large argument > > lists and now we are pulling more magic constants into the game. This > > just calls for another breakage. > > I think it would be best to only apply this to setuid processes, but > Linus asked that this change be universal. After my secureexec > refactoring, I think it should be possible to add a "how much stack > has already been used?" check in setup_new_exec() and abort the > privileged exec if it exceeds the secureexec stack limit. > > > I think we should simply step back and think about what we want to fix > > here actually. If this is the pathological case when the attacker can > > grow the stack too large and too close to a regular mappings then we > > already have means to address that (stack gap). > > I think Linus's intention is to back off from the stack gap, but maybe > I misunderstood. We will always need some gap inforcement. 256 pages enforced currently can be loosen after the stack probing is generally spread. But let's be realistic there are people using other (non-distribution) compilers and it would be good to have them covered as well, to some extent at least. Also we might remove the expand_stack enforcement but we will still need to keep a gap for new mmaps. With all that in place I am not really sure what this patch actually prevents from. > > If we are worried that mmaps can get way too close to the stack then > > I would question why this is possible at all. Bottom-up layout will > > require consuming mmap space and top-down layout seems just broken > > because we do not try to offset the mmap_base relative to the stack and > > rather calculate both from TASK_SIZE. Or at least this is my current > > undestanding. Am I missing something? Aren't we just trying to fix a bug > > at a wrong place? > > With a variable stack limit, we'll continue to run risks of > gap-jumping if the compiler isn't doing stack probing, so while we > might be able to further improve the layout logic, I think we still > need to impose limits on setuid programs. So how exactly this patch helps if we really enforce the gap between the stack and the mmap base? -- Michal Hocko SUSE Labs