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

[Kees Cook]

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.

So, yes to patch 1, that makes sense to not allow silent overlap, but
no to patch 2: we do not want silent entropy reduction.

-Kees

>                         } else
>                                 load_bias = 0;
>
> --
> 2.14.2
>



-- 
Kees Cook
Pixel Security