Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment

[Michal Hocko]

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