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

[Kees Cook]

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.

-Kees

-- 
Kees Cook
Pixel Security