Dohh, screwed up From. Sorry for spamming.
On Wed 04-10-17 09:50:59, Michal Hocko wrote:
> Hi,
> while studying CVE-2017-1000253 and the MAP_FIXED usage in load_elf*
> code paths I have stumbled over MAP_FIXED usage for elf segments
> mapping. I am not really familiar with this area much so I might draw
> completely incorrect conclusions here but I am really wondering why we
> are doing MAP_FIXED there at all.
>
> I can see why some segments really have to be mapped at a specific
> address but I wonder whether MAP_FIXED is the right tool to achieve
> that. It seems to me that MAP_FIXED is fundamentally dangerous because
> it unmaps any existing mapping. I assume that nothing should be really
> mapped in the requested range that early so we can only stumble over
> something when the address space randomization place things unexpectedly
> (which was the case of the above mentioned CVE AFAIU).
>
> So my primary question is whether we can/should simply drop MAP_FIXED
> from elf_map at all. Instead we should test whether the mapping was
> successful for the requested address and fail otherwise. I realize that
> failing due to something that a user has no idea about sucks a lot but
> it seems to me safer to simply complain into the log and fail is a safer
> option.
>
> Something like the following completely untested diff. Or am I
> completely missing the point of the MAP_FIXED purpose?
> ---
> 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);
> + 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,
> */
> 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)),
> --
> Michal Hocko
> SUSE Labs
--
Michal Hocko
SUSE Labs
