linux-kernel - Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET

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: <20171017090424.hfw64zumekcavgug@dhcp22.suse.cz>
Date:   Tue, 17 Oct 2017 11:04:24 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Al Viro <viro@...iv.linux.org.uk>,
        Oleg Nesterov <oleg@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment

On Mon 16-10-17 12:38:19, Kees Cook wrote:
> On Mon, Oct 16, 2017 at 11:43 AM, Michal Hocko <mhocko@...nel.org> wrote:
> > On Mon 16-10-17 09:44:31, Kees Cook wrote:
> >> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@...nel.org> wrote:
> >> > From: Michal Hocko <mhocko@...e.com>
> >> >
> >> > 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 <mhocko@...e.com>
> >> > ---
> >> >  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