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

fs/binfmt_elf.c +26 -3

index 6466153f2bf0..09456e2add18 100644
@@ -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)),

Signature · 2 lines

Michal Hocko
SUSE Labs

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

fs/binfmt_elf.c +26 -3

index 6466153f2bf0..09456e2add18 100644
@@ -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)),

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 15 lines · Michal Hocko

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).

Quoted 6 lines

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.

Sorry to interrupt. I tried below example.c and example2.c files and compile and link them with fpie and pie. Seems in the final PIE executable, the local global is pc relative. Means the data segment has to be put after the code segment though PIE program. Then MAP_FIXED is a good to have flag, especially we have counted in the total_size to search an area to cover the whole dynamic program and get the load_bias. It's no way to fail to get map agrea in case load_addr_set == 1. So with MAP_FIXED set, we won't take time to search the mm vma rb tree when load_addr_set == 1, but just return the specified addr directly, looks more efficient.

From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001

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.

Quoted 1 line

From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001

From: Baoquan He <[email protected]>
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 <[email protected]>

---
 fs/binfmt_elf.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

fs/binfmt_elf.c +7 -6

index 72b7ecba7ead..43a47b2aa3f6 100644
@@ -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,

fs/binfmt_elf.c +6 -1

+		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 */

Signature · 1 line

2.5.5

Quoted 30 lines · Baoquan He

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.

Quoted 1 line

From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001

From: Baoquan He <[email protected]>
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 <[email protected]>

---
 fs/binfmt_elf.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

fs/binfmt_elf.c +0 -0

index 72b7ecba7ead..43a47b2aa3f6 100644
@@ -357,22 +357,25 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
 		return addr;
 

Quoted 5 lines

fs/binfmt_elf.c +0 -4

 	/*
-	* 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)

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 367

Quoted 10 lines

fs/binfmt_elf.c +8 -2

-	* 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.

Quoted 4 lines

fs/binfmt_elf.c +0 -1

 	*/
 	if (total_size) {
 		total_size = ELF_PAGEALIGN(total_size);
-		map_addr = vm_mmap(filep, addr, total_size, prot, flags, off);

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 371

Quoted 5 lines

fs/binfmt_elf.c +1 -4

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

Quoted 14 lines

fs/binfmt_elf.c +6 -1

+		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 */

Signature · 1 line

2.5.5

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 31 lines · Michal Hocko

Quoted 30 lines · Baoquan He

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.

Quoted 1 line

From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001

From: Baoquan He <[email protected]>
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 <[email protected]>

---
 fs/binfmt_elf.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

fs/binfmt_elf.c +0 -0

index 72b7ecba7ead..43a47b2aa3f6 100644
@@ -357,22 +357,25 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
 		return addr;
 

Quoted 15 lines

Quoted 5 lines

fs/binfmt_elf.c +0 -4

 	/*
-	* 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)

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 367

Quoted 10 lines

fs/binfmt_elf.c +8 -2

-	* 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.

Quoted 4 lines

Quoted 4 lines

fs/binfmt_elf.c +0 -1

 	*/
 	if (total_size) {
 		total_size = ELF_PAGEALIGN(total_size);
-		map_addr = vm_mmap(filep, addr, total_size, prot, flags, off);

Quoted 8 lines

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 371

Quoted 5 lines

fs/binfmt_elf.c +1 -4

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

Yes, but we already have a new stack mapped and that was the point of the referenced CVE where the binary segments got mapped over the stack AFAIU.

Quoted 4 lines · Michal Hocko

Yes, but we already have a new stack mapped and that was the point of the referenced CVE where the binary segments got mapped over the stack AFAIU.

Well, if you control the binary to the point where you just make the ELF section map on top of the stack, what's the problem?

I mean, it's not a security issue. You could just have written the code to do mmap() instead.

So I think this is a "crazy users can do crazy things, we're not arbiters of taste" thing.

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 371

Quoted 8 lines · Baoquan He

fs/binfmt_elf.c +1 -5

 	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?

Quoted 13 lines · Michal Hocko

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 371

Quoted 8 lines · Baoquan He

fs/binfmt_elf.c +1 -5

 	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?

I got lost...

this is just cleanup, it should not change the behaviour, with or without MAP_FIXED. It just avoids the mmap(total_size) + munmap(extra_size).

From: Michal Hocko <[email protected]>

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 <[email protected]>

---
 fs/binfmt_elf.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

fs/binfmt_elf.c +7 -0

index 6466153f2bf0..09456e2add18 100644
@@ -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

fs/binfmt_elf.c +3 -4

        */
        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

On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <[email protected]> wrote:

Quoted 11 lines

fs/binfmt_elf.c +11 -0

+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);

fs/binfmt_elf.c create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, · line 360

Quoted 6 lines

fs/binfmt_elf.c +6 -0

+       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().

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+               return -EAGAIN;
+       }
+
+       return map_addr;
+}

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr,

Quoted 5 lines

fs/binfmt_elf.c +1 -0

+
 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.

Quoted 16 lines

fs/binfmt_elf.c +2 -2

        */
        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++;
 

Quoted 9 lines

fs/binfmt_elf.c +1 -2

        /* 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.

[CCing metag people for the metag elf_map implementation specific. The thread starts here http://lkml.kernel.org/r/[email protected]]

Quoted 14 lines · Kees Cook

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr,

Quoted 10 lines · Michal Hocko

fs/binfmt_elf.c +6 -0

+               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?

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.

Quoted 21 lines · Michal Hocko

[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]

Quoted 14 lines · Kees Cook

Review static unsigned long elf_map(struct file *filep, unsigned long addr,

Quoted 10 lines · Michal Hocko

(quoted diff) +6 -0

+               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

Quoted 7 lines

	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:

Quoted 4 lines

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.

Quoted 41 lines · James Hogan

Quoted 21 lines · Michal Hocko

[CCing metag people for the metag elf_map implementation specific. The thread starts here http://lkml.kernel.org/r/[email protected]]

Quoted 14 lines · Kees Cook

arch/metag/kernel/process.c static unsigned long elf_map(struct file *filep, unsigned long addr,

Quoted 10 lines · Michal Hocko

arch/metag/kernel/process.c +6 -0

+               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

Quoted 7 lines

	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:

Quoted 4 lines

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.

Quoted 6 lines

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/[email protected],
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.
---

arch/metag/kernel/process.c +23 -0

index c4606ce743d2..b20596b4c4c2 100644
@@ -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);

On Tue, Oct 17, 2017 at 09:37:48AM +0200, Michal Hocko wrote:

Quoted 50 lines

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

arch/metag/kernel/process.c +25 -2

index c4606ce743d2..b20596b4c4c2 100644
@@ -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);
 

arch/metag/kernel/process.c unsigned long __metag_elf_map(struct file *filep, unsigned long addr, · line 443

Quoted 2 lines

arch/metag/kernel/process.c +0 -0

 	if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) {
 		struct tcm_allocation *tcm;

Yeh that looks reasonable to me.

From: Michal Hocko <[email protected]>

fs/binfmt_elf.c +23 -0

index 6466153f2bf0..09456e2add18 100644
@@ -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)

fs/binfmt_elf.c +2 -2

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

Signature · 1 line

2.14.2

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.

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

Quoted 36 lines

From: Michal Hocko <[email protected]>

fs/binfmt_elf.c +23 -0

index 6466153f2bf0..09456e2add18 100644
@@ -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)

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 394

Quoted 7 lines

fs/binfmt_elf.c +1 -1

@@ -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.

Quoted 5 lines · Baoquan He

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.

Quoted 3 lines

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

yes, there was a typo in the email address which I've fixed up in the reply... Your cleanup was mostly about replacing vm_mmap by get_unmaped_area which is an independent issue to what I am trying to achieve here.

Quoted 48 lines · Michal Hocko

Quoted 36 lines

From: Michal Hocko <[email protected]>

fs/binfmt_elf.c +23 -0

index 6466153f2bf0..09456e2add18 100644
@@ -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)

fs/binfmt_elf.c static unsigned long elf_map(struct file *filep, unsigned long addr, · line 394

Quoted 7 lines

fs/binfmt_elf.c +1 -1

@@ -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.

Quoted 2 lines

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.

Could you be more specific please?

Quoted 5 lines

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.

I've added another level of helper to keep the code in elf_map saner. It is quite complex already.

Signature · 2 lines

Michal Hocko
SUSE Labs

And as I said before, then we will still have the ungraceful 'mapping total'|'unmapping the rest' method.

If from a new code reader's point of view, the newly added elf_vm_mmap() plus the existing elf_map() may confuse people more.

On Tue 17-10-17 21:22:46, Baoquan He wrote:
[...]

Quoted 2 lines

And as I said before, then we will still have the ungraceful 'mapping total'|'unmapping the rest' method.

yes I haven't touched this part and I guess you are right it can be removed now, but this was not my primary concern to adddress so there is still some room for improvements on top. If you want to refactor that part, then feel free to do so but I think that we should address the MAP_FIXED part first.

From: Michal Hocko <[email protected]>

eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added MAP_FIXED flag to the initial ET_DYN segment mapping which defines the randomized base for the PIE ELF segments. The thing is that MAP_FIXED shouldn't be really needed because the address is essentially random anyway. All other segments are mapped relatively to this base. elf_map makes sure that all segments will fit into the address space by enforcing total_mapping_size initial map.

Why do we want to drop MAP_FIXED in the first place? Because it is error prone. If we happen to have an existing mapping in the requested range then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap will simply fallback to another range. In reality there shouldn't be any conflicting mappings at this early exec stage so the mmap should succeed even without MAP_FIXED but subtle changes to the randomization can break this assumption so we should rather be careful here.

Fixes eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

Signed-off-by Michal Hocko <[email protected]>

---
 fs/binfmt_elf.c | 1 -
 1 file changed, 1 deletion(-)

fs/binfmt_elf.c +0 -0

index 09456e2add18..244cc30dfa24 100644

fs/binfmt_elf.c +0 -1

                        } else
                                load_bias = 0;
 
--

2.14.2

Quoted 29 lines · Michal Hocko

From: Michal Hocko <[email protected]>

eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added MAP_FIXED flag to the initial ET_DYN segment mapping which defines the randomized base for the PIE ELF segments. The thing is that MAP_FIXED shouldn't be really needed because the address is essentially random anyway. All other segments are mapped relatively to this base. elf_map makes sure that all segments will fit into the address space by enforcing total_mapping_size initial map.

Why do we want to drop MAP_FIXED in the first place? Because it is error prone. If we happen to have an existing mapping in the requested range then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap will simply fallback to another range. In reality there shouldn't be any conflicting mappings at this early exec stage so the mmap should succeed even without MAP_FIXED but subtle changes to the randomization can break this assumption so we should rather be careful here.

Fixes eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

Signed-off-by Michal Hocko <[email protected]>

---
 fs/binfmt_elf.c | 1 -
 1 file changed, 1 deletion(-)

fs/binfmt_elf.c +0 -0

index 09456e2add18..244cc30dfa24 100644

fs/binfmt_elf.c static int load_elf_binary(struct linux_binprm *bprm) · line 991

Quoted 4 lines

fs/binfmt_elf.c +0 -1

@@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
                                load_bias = ELF_ET_DYN_BASE;
                                if (current->flags & PF_RANDOMIZE)
                                        load_bias += arch_mmap_rnd();
-                               elf_flags |= MAP_FIXED;

If MAP_FIXED is being masked out in patch 1 (but used as a check for correct position, I think this MAP_FIXED should _not_ be removed). This provides for checking for the initial mapping. The failure mode here would be to allow an attack to "push" a mapping away from some overlapping region. This should not be allowed either: if the initial mapping is "wrong", we should absolutely fail, otherwise we can be introducing a silent reduction in PIE entropy.

Quoted 35 lines · Kees Cook

Quoted 29 lines · Michal Hocko

From: Michal Hocko <[email protected]>

eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added MAP_FIXED flag to the initial ET_DYN segment mapping which defines the randomized base for the PIE ELF segments. The thing is that MAP_FIXED shouldn't be really needed because the address is essentially random anyway. All other segments are mapped relatively to this base. elf_map makes sure that all segments will fit into the address space by enforcing total_mapping_size initial map.

Why do we want to drop MAP_FIXED in the first place? Because it is error prone. If we happen to have an existing mapping in the requested range then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap will simply fallback to another range. In reality there shouldn't be any conflicting mappings at this early exec stage so the mmap should succeed even without MAP_FIXED but subtle changes to the randomization can break this assumption so we should rather be careful here.

Fixes eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

Signed-off-by Michal Hocko <[email protected]>

---
 fs/binfmt_elf.c | 1 -
 1 file changed, 1 deletion(-)

fs/binfmt_elf.c +0 -0

index 09456e2add18..244cc30dfa24 100644

fs/binfmt_elf.c static int load_elf_binary(struct linux_binprm *bprm) · line 991

Quoted 4 lines

fs/binfmt_elf.c +0 -1

@@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
                                load_bias = ELF_ET_DYN_BASE;
                                if (current->flags & PF_RANDOMIZE)
                                        load_bias += arch_mmap_rnd();
-                               elf_flags |= MAP_FIXED;

Quoted 7 lines

If MAP_FIXED is being masked out in patch 1 (but used as a check for correct position, I think this MAP_FIXED should _not_ be removed). This provides for checking for the initial mapping. The failure mode here would be to allow an attack to "push" a mapping away from some overlapping region. This should not be allowed either: if the initial mapping is "wrong", we should absolutely fail, otherwise we can be introducing a silent reduction in PIE entropy.

Do we really lose any entropy? We are using standard randomized mmap in that case. So we are randomized in either case. Are you worried that an attacker could tell the two cases and abuse some sort of offset2lib attack?

Quoted 44 lines · Michal Hocko

Quoted 35 lines · Kees Cook

Quoted 29 lines · Michal Hocko

From: Michal Hocko <[email protected]>

eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added MAP_FIXED flag to the initial ET_DYN segment mapping which defines the randomized base for the PIE ELF segments. The thing is that MAP_FIXED shouldn't be really needed because the address is essentially random anyway. All other segments are mapped relatively to this base. elf_map makes sure that all segments will fit into the address space by enforcing total_mapping_size initial map.

Why do we want to drop MAP_FIXED in the first place? Because it is error prone. If we happen to have an existing mapping in the requested range then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap will simply fallback to another range. In reality there shouldn't be any conflicting mappings at this early exec stage so the mmap should succeed even without MAP_FIXED but subtle changes to the randomization can break this assumption so we should rather be careful here.

Fixes eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

Signed-off-by Michal Hocko <[email protected]>

---
 fs/binfmt_elf.c | 1 -
 1 file changed, 1 deletion(-)

fs/binfmt_elf.c +0 -0

index 09456e2add18..244cc30dfa24 100644

fs/binfmt_elf.c static int load_elf_binary(struct linux_binprm *bprm) · line 991

Quoted 4 lines

fs/binfmt_elf.c +0 -1

@@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
                                load_bias = ELF_ET_DYN_BASE;
                                if (current->flags & PF_RANDOMIZE)
                                        load_bias += arch_mmap_rnd();
-                               elf_flags |= MAP_FIXED;

Quoted 7 lines

If MAP_FIXED is being masked out in patch 1 (but used as a check for correct position, I think this MAP_FIXED should _not_ be removed). This provides for checking for the initial mapping. The failure mode here would be to allow an attack to "push" a mapping away from some overlapping region. This should not be allowed either: if the initial mapping is "wrong", we should absolutely fail, otherwise we can be introducing a silent reduction in PIE entropy.

Quoted 4 lines

Do we really lose any entropy? We are using standard randomized mmap in that case. So we are randomized in either case. Are you worried that an attacker could tell the two cases and abuse some sort of offset2lib attack?

Not in the regular case. I'm suggesting that what your changes are preparing for is an _unknown_ way to collide mappings. In that case, we should be as defensive as we know how. And if we were to remove MAP_FIXED here, it would allow an attacker (with some future method) to potentially collapse a range of ASLR for execution, since missing MAP_FIXED here would silently move a mapping somewhere else. So we should keep MAP_FIXED, as any collision would indicate an unknown method of crashing an exec into something else.

Quoted 50 lines · Kees Cook

Quoted 44 lines · Michal Hocko

Quoted 35 lines · Kees Cook

Quoted 29 lines · Michal Hocko

From: Michal Hocko <[email protected]>

eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added MAP_FIXED flag to the initial ET_DYN segment mapping which defines the randomized base for the PIE ELF segments. The thing is that MAP_FIXED shouldn't be really needed because the address is essentially random anyway. All other segments are mapped relatively to this base. elf_map makes sure that all segments will fit into the address space by enforcing total_mapping_size initial map.

Why do we want to drop MAP_FIXED in the first place? Because it is error prone. If we happen to have an existing mapping in the requested range then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap will simply fallback to another range. In reality there shouldn't be any conflicting mappings at this early exec stage so the mmap should succeed even without MAP_FIXED but subtle changes to the randomization can break this assumption so we should rather be careful here.

Fixes eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

Signed-off-by Michal Hocko <[email protected]>

---
 fs/binfmt_elf.c | 1 -
 1 file changed, 1 deletion(-)

fs/binfmt_elf.c +0 -0

index 09456e2add18..244cc30dfa24 100644

fs/binfmt_elf.c static int load_elf_binary(struct linux_binprm *bprm) · line 991

Quoted 4 lines

fs/binfmt_elf.c +0 -1

@@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
                                load_bias = ELF_ET_DYN_BASE;
                                if (current->flags & PF_RANDOMIZE)
                                        load_bias += arch_mmap_rnd();
-                               elf_flags |= MAP_FIXED;

Quoted 7 lines

If MAP_FIXED is being masked out in patch 1 (but used as a check for correct position, I think this MAP_FIXED should _not_ be removed). This provides for checking for the initial mapping. The failure mode here would be to allow an attack to "push" a mapping away from some overlapping region. This should not be allowed either: if the initial mapping is "wrong", we should absolutely fail, otherwise we can be introducing a silent reduction in PIE entropy.

Quoted 4 lines

Do we really lose any entropy? We are using standard randomized mmap in that case. So we are randomized in either case. Are you worried that an attacker could tell the two cases and abuse some sort of offset2lib attack?

Quoted 8 lines

Not in the regular case. I'm suggesting that what your changes are preparing for is an _unknown_ way to collide mappings. In that case, we should be as defensive as we know how. And if we were to remove MAP_FIXED here, it would allow an attacker (with some future method) to potentially collapse a range of ASLR for execution, since missing MAP_FIXED here would silently move a mapping somewhere else. So we should keep MAP_FIXED, as any collision would indicate an unknown method of crashing an exec into something else.

I am sorry but I do not follow. I could see how offset2lib would be a concern but you seem to be thinking about a different scenario. Could you be more specific please.

I am not insisting on this patch but it seems to me is just makes a recoverable state a failure.

On Tue, Oct 17, 2017 at 2:04 AM, Michal Hocko <[email protected]> wrote:

Quoted 2 lines

I am not insisting on this patch but it seems to me is just makes a recoverable state a failure.

Right, I understand you're trying to make it recoverable. I'm suggesting that making it recoverable provides a way for an attack to abuse it, and that what we'd be recovering from is a case we should never ever see.

Consider the case where through some future bug/feature, it's possible to put the stack at an arbitrary location during an exec. (We've worked to fix that already, but who knows what the future holds either through misfeatures or bugs.) If an attacker maps the stack over a large portion of the PIE exec range, patch 2 will result in vmmap searching out a location that isn't already allocated. This means that instead of the PIE ASLR choosing from the entire possible range, it will get limited to only the area where something isn't already overlapping. This would give an attacker the ability to control the PIE ASLR, possibly forcing it into a fixed location.

Quoted 1 line · Kees Cook

On Tue, Oct 17, 2017 at 2:04 AM, Michal Hocko <[email protected]> wrote:

[...]

Quoted 8 lines

Quoted 2 lines

I am not insisting on this patch but it seems to me is just makes a recoverable state a failure.

Right, I understand you're trying to make it recoverable. I'm suggesting that making it recoverable provides a way for an attack to abuse it, and that what we'd be recovering from is a case we should never ever see.

Quoted 10 lines

Consider the case where through some future bug/feature, it's possible to put the stack at an arbitrary location during an exec. (We've worked to fix that already, but who knows what the future holds either through misfeatures or bugs.) If an attacker maps the stack over a large portion of the PIE exec range, patch 2 will result in vmmap searching out a location that isn't already allocated. This means that instead of the PIE ASLR choosing from the entire possible range, it will get limited to only the area where something isn't already overlapping. This would give an attacker the ability to control the PIE ASLR, possibly forcing it into a fixed location.

Yes, I guess I understand that part. What is not clear to me exactly is why this matters as we have the mmap_base randomized and not under the control of the attacker.

Quoted 22 lines · Michal Hocko

Quoted 1 line · Kees Cook

On Tue, Oct 17, 2017 at 2:04 AM, Michal Hocko <[email protected]> wrote:

[...]

Quoted 18 lines

Quoted 2 lines

I am not insisting on this patch but it seems to me is just makes a recoverable state a failure.

Right, I understand you're trying to make it recoverable. I'm suggesting that making it recoverable provides a way for an attack to abuse it, and that what we'd be recovering from is a case we should never ever see.

Consider the case where through some future bug/feature, it's possible to put the stack at an arbitrary location during an exec. (We've worked to fix that already, but who knows what the future holds either through misfeatures or bugs.) If an attacker maps the stack over a large portion of the PIE exec range, patch 2 will result in vmmap searching out a location that isn't already allocated. This means that instead of the PIE ASLR choosing from the entire possible range, it will get limited to only the area where something isn't already overlapping. This would give an attacker the ability to control the PIE ASLR, possibly forcing it into a fixed location.

Quoted 3 lines

Yes, I guess I understand that part. What is not clear to me exactly is why this matters as we have the mmap_base randomized and not under the control of the attacker.

mmap_base is separate from the PIE base, so patch 2 would allow for a reduction of the PIE ASLR entropy in the case of a novel overlap attack.

Quoted 27 lines · Kees Cook

Quoted 25 lines · Michal Hocko

Quoted 1 line · Kees Cook

On Tue, Oct 17, 2017 at 2:04 AM, Michal Hocko <[email protected]> wrote:

[...]

Quoted 8 lines

Quoted 2 lines

I am not insisting on this patch but it seems to me is just makes a recoverable state a failure.

Right, I understand you're trying to make it recoverable. I'm suggesting that making it recoverable provides a way for an attack to abuse it, and that what we'd be recovering from is a case we should never ever see.

Quoted 10 lines

Consider the case where through some future bug/feature, it's possible to put the stack at an arbitrary location during an exec. (We've worked to fix that already, but who knows what the future holds either through misfeatures or bugs.) If an attacker maps the stack over a large portion of the PIE exec range, patch 2 will result in vmmap searching out a location that isn't already allocated. This means that instead of the PIE ASLR choosing from the entire possible range, it will get limited to only the area where something isn't already overlapping. This would give an attacker the ability to control the PIE ASLR, possibly forcing it into a fixed location.

Yes, I guess I understand that part. What is not clear to me exactly is why this matters as we have the mmap_base randomized and not under the control of the attacker.

Quoted 3 lines

mmap_base is separate from the PIE base, so patch 2 would allow for a reduction of the PIE ASLR entropy in the case of a novel overlap attack.

OK, it seems that I am just too dull see through your concerns here.

Anyway, are you willing to ack the patch 1 (when metag fix is included)? I would resubmit in that case and ask for merging without patch 2.