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

From: Baoquan He <bhe@redhat.com>
To: Michal Hocko <mhocko@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>, Jiri Kosina <jkosina@suse.cz>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Oleg Nesterov <oleg@redhat.com>, Ingo Molnar <mingo@redhat.com>,
	Michal Hocko <mhocko@suse.com>
Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map
Date: Tue, 17 Oct 2017 20:26:14 +0800	[thread overview]
Message-ID: <20171017122614.GJ11198@x1> (raw)
In-Reply-To: <20171016134446.19910-2-mhocko@kernel.org>

Hi Michal,

Earlier I posted a patchset to clean up these in a different way, but
patches sent to your below mail address are all rejected.

<mhocko@kerne.org>: host mail.kerne.org[104.131.33.237] said: 454 4.1.1
    <mhocko@kerne.org>: Recipient address rejected: User unknown in virtual
    mailbox table (in reply to RCPT TO command)

On 10/16/17 at 03:44pm, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 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);

Here we will still take the map total, then unmap the rest way.

I am wondering why we don't fix those issues we figured out, but add 
another level of wrapper elf_vm_mmap() to hack it. So we will have
elf_map() -> elf_vm_mmap() -> vm_mmap(), not even counting
vm_mmap_pgoff(), then finally enter into do_mmap_pgoff(), to do the
maping for elf program.

Thanks
Baoquan

>  	} 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
> 