On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <[email protected]> wrote: > From: Michal Hocko <[email protected]> > > 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 <[email protected]> > --- > 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
