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: <CAGXu5jJSK-jh3ZNC1Y8h8LYh5wrGqw2WnUbw0F1xGxF32uKx8g@mail.gmail.com>
Date:   Mon, 16 Oct 2017 09:44:31 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Michal Hocko <mhocko@...nel.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>,
        Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment

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.

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