Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[Kees Cook]

On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
>
> Both load_elf_interp and load_elf_binary rely on elf_map to map segments
> on a controlled address and they use MAP_FIXED to enforce that. This is
> however dangerous thing prone to silent data corruption which can be
> even exploitable. Let's take CVE-2017-1000253 as an example. At the time
> (before eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE"))
> ELF_ET_DYN_BASE was at TASK_SIZE / 3 * 2 which is not that far away from
> the stack top on 32b (legacy) memory layout (only 1GB away). Therefore
> we could end up mapping over the existing stack with some luck.
>
> The issue has been fixed since then (a87938b2e246 ("fs/binfmt_elf.c:
> fix bug in loading of PIE binaries")), ELF_ET_DYN_BASE moved moved much
> further from the stack (eab09532d400 and later by c715b72c1ba4 ("mm:
> revert x86_64 and arm64 ELF_ET_DYN_BASE base changes")) and excessive
> stack consumption early during execve fully stopped by da029c11e6b1
> ("exec: Limit arg stack to at most 75% of _STK_LIM"). So we should be
> safe and any attack should be impractical. On the other hand this is
> just too subtle assumption so it can break quite easily and hard to
> spot.
>
> I believe that the MAP_FIXED usage in load_elf_binary (et. al) is still
> fundamentally dangerous. Moreover it shouldn't be even needed. We are
> at the early process stage and so there shouldn't be unrelated mappings
> (except for stack and loader) existing so mmap for a given address
> should succeed even without MAP_FIXED. Something is terribly wrong if
> this is not the case and we should rather fail than silently corrupt the
> underlying mapping.
>
> Address this issue by adding a helper elf_vm_mmap used by elf_map which
> drops MAP_FIXED when asking for the mapping and check whether the
> returned address really matches what the caller asked for and complain
> loudly if this is not the case and fail. Such a failure would be a
> kernel bug and it should alarm us to look what has gone wrong.
>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
>  fs/binfmt_elf.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 6466153f2bf0..09456e2add18 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -341,6 +341,29 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
>
>  #ifndef elf_map
>
> +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> +               unsigned long size, int prot, int type, unsigned long off)
> +{
> +       unsigned long map_addr;
> +
> +       /*
> +        * If caller requests the mapping at a specific place, make sure we fail
> +        * rather than potentially clobber an existing mapping which can have
> +        * security consequences (e.g. smash over the stack area).
> +        */
> +       map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> +       if (BAD_ADDR(map_addr))
> +               return map_addr;
> +
> +       if ((type & MAP_FIXED) && map_addr != addr) {
> +               pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> +                               (void*)addr);

Is "info" loud enough? I actually think this should be a WARN_ONCE().

> +               return -EAGAIN;
> +       }
> +
> +       return map_addr;
> +}
> +
>  static unsigned long elf_map(struct file *filep, unsigned long addr,
>                 struct elf_phdr *eppnt, int prot, int type,
>                 unsigned long total_size)
> @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,

elf_map is redirected on metag -- it should probably have its vm_mmap
calls adjust too.

>         */
>         if (total_size) {
>                 total_size = ELF_PAGEALIGN(total_size);
> -               map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> +               map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
>                 if (!BAD_ADDR(map_addr))
>                         vm_munmap(map_addr+size, total_size-size);
>         } else
> -               map_addr = vm_mmap(filep, addr, size, prot, type, off);
> +               map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
>
>         return(map_addr);
>  }
> @@ -1215,7 +1238,7 @@ static int load_elf_library(struct file *file)
>                 eppnt++;
>
>         /* Now use mmap to map the library into memory. */
> -       error = vm_mmap(file,
> +       error = elf_vm_mmap(file,
>                         ELF_PAGESTART(eppnt->p_vaddr),
>                         (eppnt->p_filesz +
>                          ELF_PAGEOFFSET(eppnt->p_vaddr)),
> --
> 2.14.2
>

Otherwise, yeah, this should be good.

-Kees

-- 
Kees Cook
Pixel Security