From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752512AbdJDPhu (ORCPT ); Wed, 4 Oct 2017 11:37:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48961 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752364AbdJDPht (ORCPT ); Wed, 4 Oct 2017 11:37:49 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4C3787F3F3 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=bhe@redhat.com Date: Wed, 4 Oct 2017 23:37:41 +0800 From: Baoquan He To: Michal Hocko Cc: Linus Torvalds , Kees Cook , Oleg Nesterov , Jiri Kosina , Al Viro , Ingo Molnar , LKML Subject: Re: MAP_FIXED for ELF mappings Message-ID: <20171004153741.GH24886@x1> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171004150334.GB31992@x1> <20171004151238.GG24886@x1> <20171004151700.k4fvivvmi7pm5sl7@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171004151700.k4fvivvmi7pm5sl7@dhcp22.suse.cz> User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 04 Oct 2017 15:37:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/04/17 at 05:17pm, Michal Hocko wrote: > On Wed 04-10-17 23:12:38, Baoquan He wrote: > > I made a clean up patch according to Oleg's suggestion. It's trying to > > get an map area to cover total_size, then do mmap for for the 1st > > program segment only. Not sure if this way is correct. > > > > >From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001 > > From: Baoquan He > > Date: Fri, 29 Sep 2017 21:35:30 +0800 > > Subject: [PATCH] binfmt_elf: Clean up the elf_map > > > > Oleg pointed out that it's really ugly to do mmap of the total_size, then > > unmap the region excluding the 1st segment. The right way should be search > > an unmapped area which can cover region of total_size, then map the 1st > > segment only. > > > > And also update the code comment accordingly. In below commit, the relevant > > code comment is not changed to cover the ELF binary image case. > > commit a87938b2e2 ("fs/binfmt_elf.c: fix bug in loading of PIE binaries") > > > > Signed-off-by: Baoquan He > > --- > > fs/binfmt_elf.c | 27 +++++++++++++++------------ > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > index 72b7ecba7ead..43a47b2aa3f6 100644 > > --- a/fs/binfmt_elf.c > > +++ b/fs/binfmt_elf.c > > @@ -357,22 +357,25 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, > > return addr; > > > > /* > > - * total_size is the size of the ELF (interpreter) image. > > - * The _first_ mmap needs to know the full size, otherwise > > - * randomization might put this image into an overlapping > > - * position with the ELF binary image. (since size < total_size) > > - * So we first map the 'big' image - and unmap the remainder at > > - * the end. (which unmap is needed for ELF images with holes.) > > + * total_size is the size of the ELF binary image or the ELF loader > > + * image. For loader image, the _first_ mmap needs to know the full > > + * size, otherwise randomization might put image into an overlapping > > + * position with the ELF binary image.(since size < total_size) > > + * So we use total_size to get an area to cover the whole loader image, > > + * then map the 1st progment segment only with its own size. For binary > > + * image, similarly, the _first_ mmap also needs to know the full size, > > + * otherwise randomization might put image above mm->mmap_base. Oh, no, here, it won't include the PIE binary case. Since it must be from ELF_ET_DYN_BASE. Here should be the "ld.so program" case. > > */ > > if (total_size) { > > total_size = ELF_PAGEALIGN(total_size); > > - map_addr = vm_mmap(filep, addr, total_size, prot, flags, off); > > - if (!BAD_ADDR(map_addr)) > > - vm_munmap(map_addr+size, total_size-size); > > - } else > > - map_addr = vm_mmap(filep, addr, size, prot, flags, off); > > + addr = get_unmapped_area(file, addr, total_size, off, flags); > > So how does this prevent clobbering an existing VMA when flags contains > MAP_FIXED? Earlier flush_old_exec() is called to clean all old VMAs. Here if total_size is non-zero only if it's the 1st prgoram segment of dynamic loader, either from load_elf_interp() or the "ld.so program" case. With my understanding, it can't be meeting an existing VMA. Not sure if it's correct. Currently, it haven't passed to ld.so to link other .so. > > > + if (offset_in_page(addr)) > > + return addr; > > + } > > + > > + map_addr = vm_mmap(filep, addr, size, prot, flags, off); > > > > - return(map_addr); > > + return (map_addr); > > } > > > > #endif /* !elf_map */ > > -- > > 2.5.5 > > -- > Michal Hocko > SUSE Labs