From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759050AbdJQHhw (ORCPT ); Tue, 17 Oct 2017 03:37:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:34134 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753573AbdJQHhu (ORCPT ); Tue, 17 Oct 2017 03:37:50 -0400 Date: Tue, 17 Oct 2017 09:37:48 +0200 From: Michal Hocko To: James Hogan Cc: Kees Cook , LKML , Linus Torvalds , Jiri Kosina , Al Viro , Oleg Nesterov , Ingo Molnar , Baoquan He , linux-metag@vger.kernel.org Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map Message-ID: <20171017073748.gdzsfkh3mnfp455x@dhcp22.suse.cz> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171016134446.19910-1-mhocko@kernel.org> <20171016134446.19910-2-mhocko@kernel.org> <20171016190047.76ry2inllsacd3pv@dhcp22.suse.cz> <20171016200208.GL15235@jhogan-linux> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171016200208.GL15235@jhogan-linux> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 16-10-17 21:02:09, James Hogan wrote: > On Mon, Oct 16, 2017 at 09:00:47PM +0200, Michal Hocko wrote: > > [CCing metag people for the metag elf_map implementation specific. The thread > > starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko@kernel.org] > > > > On Mon 16-10-17 09:39:14, Kees Cook wrote: > > > On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko wrote: > > > > + 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. > > > > Thanks for spotting this. I am not really familiar with metag. It seems > > to clear MAP_FIXED already > > tcm_tag = tcm_lookup_tag(addr); > > > > if (tcm_tag != TCM_INVALID_TAG) > > type &= ~MAP_FIXED; > > > > So if there is a tag the flag is cleared. I do not understand this code > > (and git log doesn't help) but why is this MAP_FIXED code really needed? > > This function was added to the metag port in mid-2010 to support ELFs > with tightly coupled memory (TCM) segments, for example metag "core" > memories are at fixed virtual addresses and aren't MMU mappable (i.e. > globally accessible), and are outside of the usual userland address > range, but are as fast as cache. The commit message says this: > > > Override the definition of the elf_map() function to special case > > sections that are loaded at the address of the internal memories. > > If we have such a section, map it at a different address and copy > > the contents of the section into the appropriate memory. > > So yeh, it looks like if the section is meant to use TCM based on the > virtual address, it drops MAP_FIXED so that the vm_mmap can succeed > (because its outside the normally valid range), and then copies it > directly to the desired TCM so the program can use it. > > Hope that helps add some context to understand whats needed. Hmm, so IIUC then we need the same fix as http://lkml.kernel.org/r/20171016134446.19910-2-mhocko@kernel.org, right? This would be something like. I wanted to share elf_vm_mmap but didn't find a proper place to not cause include dependency hell so I balied out to c&p. --- diff --git a/arch/metag/kernel/process.c b/arch/metag/kernel/process.c index c4606ce743d2..b20596b4c4c2 100644 --- a/arch/metag/kernel/process.c +++ b/arch/metag/kernel/process.c @@ -378,6 +378,29 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu) #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE) +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; +} + unsigned long __metag_elf_map(struct file *filep, unsigned long addr, struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -410,11 +433,11 @@ unsigned long __metag_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); if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) { struct tcm_allocation *tcm; -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map Date: Tue, 17 Oct 2017 09:37:48 +0200 Message-ID: <20171017073748.gdzsfkh3mnfp455x@dhcp22.suse.cz> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171016134446.19910-1-mhocko@kernel.org> <20171016134446.19910-2-mhocko@kernel.org> <20171016190047.76ry2inllsacd3pv@dhcp22.suse.cz> <20171016200208.GL15235@jhogan-linux> Mime-Version: 1.0 Return-path: Content-Disposition: inline In-Reply-To: <20171016200208.GL15235@jhogan-linux> Sender: linux-metag-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: James Hogan Cc: Kees Cook , LKML , Linus Torvalds , Jiri Kosina , Al Viro , Oleg Nesterov , Ingo Molnar , Baoquan He , linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Mon 16-10-17 21:02:09, James Hogan wrote: > On Mon, Oct 16, 2017 at 09:00:47PM +0200, Michal Hocko wrote: > > [CCing metag people for the metag elf_map implementation specific. The thread > > starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org] > > > > On Mon 16-10-17 09:39:14, Kees Cook wrote: > > > On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko wrote: > > > > + 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. > > > > Thanks for spotting this. I am not really familiar with metag. It seems > > to clear MAP_FIXED already > > tcm_tag = tcm_lookup_tag(addr); > > > > if (tcm_tag != TCM_INVALID_TAG) > > type &= ~MAP_FIXED; > > > > So if there is a tag the flag is cleared. I do not understand this code > > (and git log doesn't help) but why is this MAP_FIXED code really needed? > > This function was added to the metag port in mid-2010 to support ELFs > with tightly coupled memory (TCM) segments, for example metag "core" > memories are at fixed virtual addresses and aren't MMU mappable (i.e. > globally accessible), and are outside of the usual userland address > range, but are as fast as cache. The commit message says this: > > > Override the definition of the elf_map() function to special case > > sections that are loaded at the address of the internal memories. > > If we have such a section, map it at a different address and copy > > the contents of the section into the appropriate memory. > > So yeh, it looks like if the section is meant to use TCM based on the > virtual address, it drops MAP_FIXED so that the vm_mmap can succeed > (because its outside the normally valid range), and then copies it > directly to the desired TCM so the program can use it. > > Hope that helps add some context to understand whats needed. Hmm, so IIUC then we need the same fix as http://lkml.kernel.org/r/20171016134446.19910-2-mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, right? This would be something like. I wanted to share elf_vm_mmap but didn't find a proper place to not cause include dependency hell so I balied out to c&p. --- diff --git a/arch/metag/kernel/process.c b/arch/metag/kernel/process.c index c4606ce743d2..b20596b4c4c2 100644 --- a/arch/metag/kernel/process.c +++ b/arch/metag/kernel/process.c @@ -378,6 +378,29 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu) #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE) +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; +} + unsigned long __metag_elf_map(struct file *filep, unsigned long addr, struct elf_phdr *eppnt, int prot, int type, unsigned long total_size) @@ -410,11 +433,11 @@ unsigned long __metag_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); if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) { struct tcm_allocation *tcm; -- Michal Hocko SUSE Labs