linux-kernel - MAP_FIXED for ELF mappings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz>
Date:   Wed, 4 Oct 2017 09:50:59 +0200
From:   Michal Hocko <mhocko@...ne.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Oleg Nesterov <oleg@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
        Al Viro <viro@...iv.linux.org.uk>, Ingo Molnar <mingo@...e.hu>,
        Baoquan He <bhe@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: MAP_FIXED for ELF mappings

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