From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759891AbdJQJE3 (ORCPT ); Tue, 17 Oct 2017 05:04:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:41769 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752936AbdJQJE0 (ORCPT ); Tue, 17 Oct 2017 05:04:26 -0400 Date: Tue, 17 Oct 2017 11:04:24 +0200 From: Michal Hocko To: Kees Cook Cc: LKML , Linus Torvalds , Jiri Kosina , Al Viro , Oleg Nesterov , Ingo Molnar , Baoquan He Subject: Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment Message-ID: <20171017090424.hfw64zumekcavgug@dhcp22.suse.cz> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171016134446.19910-1-mhocko@kernel.org> <20171016134446.19910-3-mhocko@kernel.org> <20171016184335.hj6osq7su24e75jz@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 16-10-17 12:38:19, Kees Cook wrote: > On Mon, Oct 16, 2017 at 11:43 AM, Michal Hocko wrote: > > On Mon 16-10-17 09:44:31, Kees Cook wrote: > >> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko wrote: > >> > From: Michal Hocko > >> > > >> > eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added > >> > MAP_FIXED flag to the initial ET_DYN segment mapping which defines the > >> > randomized base for the PIE ELF segments. The thing is that MAP_FIXED > >> > shouldn't be really needed because the address is essentially random > >> > anyway. All other segments are mapped relatively to this base. elf_map > >> > makes sure that all segments will fit into the address space by > >> > enforcing total_mapping_size initial map. > >> > > >> > Why do we want to drop MAP_FIXED in the first place? Because it is error > >> > prone. If we happen to have an existing mapping in the requested range > >> > then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap > >> > will simply fallback to another range. In reality there shouldn't be > >> > any conflicting mappings at this early exec stage so the mmap should > >> > succeed even without MAP_FIXED but subtle changes to the randomization > >> > can break this assumption so we should rather be careful here. > >> > > >> > Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") > >> > Signed-off-by: Michal Hocko > >> > --- > >> > fs/binfmt_elf.c | 1 - > >> > 1 file changed, 1 deletion(-) > >> > > >> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > >> > index 09456e2add18..244cc30dfa24 100644 > >> > --- a/fs/binfmt_elf.c > >> > +++ b/fs/binfmt_elf.c > >> > @@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm) > >> > load_bias = ELF_ET_DYN_BASE; > >> > if (current->flags & PF_RANDOMIZE) > >> > load_bias += arch_mmap_rnd(); > >> > - elf_flags |= MAP_FIXED; > >> > >> If MAP_FIXED is being masked out in patch 1 (but used as a check for > >> correct position, I think this MAP_FIXED should _not_ be removed). > >> This provides for checking for the initial mapping. The failure mode > >> here would be to allow an attack to "push" a mapping away from some > >> overlapping region. This should not be allowed either: if the initial > >> mapping is "wrong", we should absolutely fail, otherwise we can be > >> introducing a silent reduction in PIE entropy. > > > > Do we really lose any entropy? We are using standard randomized mmap in > > that case. So we are randomized in either case. Are you worried that > > an attacker could tell the two cases and abuse some sort of offset2lib > > attack? > > Not in the regular case. I'm suggesting that what your changes are > preparing for is an _unknown_ way to collide mappings. In that case, > we should be as defensive as we know how. And if we were to remove > MAP_FIXED here, it would allow an attacker (with some future method) > to potentially collapse a range of ASLR for execution, since missing > MAP_FIXED here would silently move a mapping somewhere else. So we > should keep MAP_FIXED, as any collision would indicate an unknown > method of crashing an exec into something else. I am sorry but I do not follow. I could see how offset2lib would be a concern but you seem to be thinking about a different scenario. Could you be more specific please. I am not insisting on this patch but it seems to me is just makes a recoverable state a failure. -- Michal Hocko SUSE Labs