Messages in this thread | | | From | Kees Cook <> | Date | Mon, 16 Oct 2017 12:38:19 -0700 | Subject | Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment |
| |
On Mon, Oct 16, 2017 at 11:43 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Mon 16-10-17 09:44:31, Kees Cook wrote: >> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@kernel.org> wrote: >> > From: Michal Hocko <mhocko@suse.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@suse.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. > > 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
| |