From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756272AbdJPQjT (ORCPT ); Mon, 16 Oct 2017 12:39:19 -0400 Received: from mail-io0-f179.google.com ([209.85.223.179]:46065 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755578AbdJPQjP (ORCPT ); Mon, 16 Oct 2017 12:39:15 -0400 X-Google-Smtp-Source: AOwi7QDIT6rAhMbIP6Wz44ZxVoG9JrBOa0IhixL9SVw01BDcnmjooXApf27Kr+rXjA65F0o55Kj9OD3kuNQc42dC6is= MIME-Version: 1.0 In-Reply-To: <20171016134446.19910-2-mhocko@kernel.org> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171016134446.19910-1-mhocko@kernel.org> <20171016134446.19910-2-mhocko@kernel.org> From: Kees Cook Date: Mon, 16 Oct 2017 09:39:14 -0700 X-Google-Sender-Auth: dwBmQiTL5hWFM3TuHllBxMRm9mc Message-ID: Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map To: Michal Hocko Cc: LKML , Linus Torvalds , Jiri Kosina , Al Viro , Oleg Nesterov , Ingo Molnar , Baoquan He , Michal Hocko Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko wrote: > From: Michal Hocko > > 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 > --- > 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