[PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map()

[PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map()

[Baoquan He]

Currently total_size passed into elf_map() is non-zero only for dynamic
loader, either in load_elf_interp(), or in load_elf_binary() for ET_DYN
without INTERP case. Now PIE programs are loaded offset from ELF_ET_DYN_BASE,
and map flags has been set as MAP_FIXED, no need to use tatal_size strategy.

And in elf_map(), Oleg pointed out that the mmap(total_size) + munmap(extra_size)
way looks very ugly. We can search the unmapped area of total_size big,
then only map the 1st PT_LOAD segment with the searched address.

In this patchset, clean up them all.

Baoquan He (3):
  binfmt_elf: Clean up the variable name of map flags
  binfmt_elf: Get the total_size only for dynamic loader in
    load_elf_binary()
  binfmt_elf: Search an unmapped area with total_size but not map the
    whole image

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

-- 
2.5.5

[PATCH 1/3] binfmt_elf: Clean up the variable name of map flags

[Baoquan He]

In load_elf_interp() and elf_map(), variables with value of map flags
are named as type or elf_type. That may bring confusion since their
values not only contain map type which is MAP_SHARED or MAP_PRIVATE,
also MAP_FIXED or MAP_DENYWRITE, etc.

So change them as flags or elf_flags.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 fs/binfmt_elf.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 73b01e474fdc..72b7ecba7ead 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -342,7 +342,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 #ifndef elf_map
 
 static unsigned long elf_map(struct file *filep, unsigned long addr,
-		struct elf_phdr *eppnt, int prot, int type,
+		struct elf_phdr *eppnt, int prot, int flags,
 		unsigned long total_size)
 {
 	unsigned long map_addr;
@@ -366,11 +366,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 = 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, type, off);
+		map_addr = vm_mmap(filep, addr, size, prot, flags, off);
 
 	return(map_addr);
 }
@@ -556,7 +556,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 	eppnt = interp_elf_phdata;
 	for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) {
 		if (eppnt->p_type == PT_LOAD) {
-			int elf_type = MAP_PRIVATE | MAP_DENYWRITE;
+			int elf_flags = MAP_PRIVATE | MAP_DENYWRITE;
 			int elf_prot = 0;
 			unsigned long vaddr = 0;
 			unsigned long k, map_addr;
@@ -569,12 +569,12 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 				elf_prot |= PROT_EXEC;
 			vaddr = eppnt->p_vaddr;
 			if (interp_elf_ex->e_type == ET_EXEC || load_addr_set)
-				elf_type |= MAP_FIXED;
+				elf_flags |= MAP_FIXED;
 			else if (no_base && interp_elf_ex->e_type == ET_DYN)
 				load_addr = -vaddr;
 
 			map_addr = elf_map(interpreter, load_addr + vaddr,
-					eppnt, elf_prot, elf_type, total_size);
+					eppnt, elf_prot, elf_flags, total_size);
 			total_size = 0;
 			if (!*interp_map_addr)
 				*interp_map_addr = map_addr;
-- 
2.5.5

[PATCH 2/3] binfmt_elf: Get the total_size only for dynamic loader in load_elf_binary()

[Baoquan He]

In commit:

  eab09532d4 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")

... PIE programs are loaded offset from ELF_ET_DYN_BASE, and its map
has been set as MAP_FIXED. Only dynamic loader will be mapped from
below mm->mmap_base (E.g "./ld.so someprog"), and need take account
of the need to allocate sufficient space for the entire loader image
to avoid the case that the first PT_LOAD segment is mapped below
mm->mmap_base, the subsequent PT_LOAD segment(s) end up being mapped
above mm->mmap_base into the area that is supposed to be the "gap"
between the stack and mm->mmap_base.

Whether it's harmless or not, we should not allow program to map above
mm->mmap_base. So here change to get the total_size only for dynamic
loader in load_elf_binary().

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 fs/binfmt_elf.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 72b7ecba7ead..d7a8a53a6f18 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -967,9 +967,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
 				if (current->flags & PF_RANDOMIZE)
 					load_bias += arch_mmap_rnd();
 				elf_flags |= MAP_FIXED;
-			} else
+			} else {
 				load_bias = 0;
 
+				total_size = total_mapping_size(elf_phdata,
+							loc->elf_ex.e_phnum);
+				if (!total_size) {
+					retval = -EINVAL;
+					goto out_free_dentry;
+				}
+			}
+
 			/*
 			 * Since load_bias is used for all subsequent loading
 			 * calculations, we must lower it by the first vaddr
@@ -978,13 +986,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			 * is then page aligned.
 			 */
 			load_bias = ELF_PAGESTART(load_bias - vaddr);
-
-			total_size = total_mapping_size(elf_phdata,
-							loc->elf_ex.e_phnum);
-			if (!total_size) {
-				retval = -EINVAL;
-				goto out_free_dentry;
-			}
 		}
 
 		error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
-- 
2.5.5

[PATCH 3/3] binfmt_elf: Search an unmapped area with total_size but not map the whole image

[Baoquan He]

In elf_map(), we can search an unmapped area for the whole image of dynamic
loader with total_size, then map the first PT_LOAD segment with its own size.
But not map the while image, then unmap the remainder at the end.

And also update the code comment in elf_map() accordingly.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 fs/binfmt_elf.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index d7a8a53a6f18..bebb9f2c934e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -357,20 +357,23 @@ 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 (interpreter) image if non-zero.
+	* It gets value either in load_elf_interp(), or in load_elf_binary()
+	* for "./ld.so someprog" testing. The _first_ mmap needs to know the
+	* full size, otherwise the first PT_LOAD segment is mapped below
+	* mm->mmap_base, the subsequent PT_LOAD segment(s) end up being mapped
+	* above mm->mmap_base into the area that is supposed to be the "gap"
+	* between the stack and the binary. So we search an unmapped area of
+	* total_size big, then use the found address as hint to do mmap.
 	*/
 	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);
+		if (offset_in_page(addr))
+			return addr;
+	}
+
+	map_addr = vm_mmap(filep, addr, size, prot, flags, off);
 
 	return(map_addr);
 }
-- 
2.5.5

Re: [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map()

[Baoquan He]

Hi maintainers and experts,

Any comment about this patchset?

At least for patch 2, I think we can correct it since we do not have to
take the total_size way for PIE programs which are MAP_FIXED and are
loaded offset from ELF_ET_DYN_BASE. That is making code a little
confusing.

About patch 3, it's pointed out in an Redhat internal patch reviewing. I
am fine if no one like it.

If approved, I can update the git log to make it better and repost.

Thanks
Baoquan

On 10/06/17 at 11:37am, Baoquan He wrote:
> Currently total_size passed into elf_map() is non-zero only for dynamic
> loader, either in load_elf_interp(), or in load_elf_binary() for ET_DYN
> without INTERP case. Now PIE programs are loaded offset from ELF_ET_DYN_BASE,
> and map flags has been set as MAP_FIXED, no need to use tatal_size strategy.
> 
> And in elf_map(), Oleg pointed out that the mmap(total_size) + munmap(extra_size)
> way looks very ugly. We can search the unmapped area of total_size big,
> then only map the 1st PT_LOAD segment with the searched address.
> 
> In this patchset, clean up them all.
> 
> Baoquan He (3):
>   binfmt_elf: Clean up the variable name of map flags
>   binfmt_elf: Get the total_size only for dynamic loader in
>     load_elf_binary()
>   binfmt_elf: Search an unmapped area with total_size but not map the
>     whole image
> 
>  fs/binfmt_elf.c | 50 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 27 insertions(+), 23 deletions(-)
> 
> -- 
> 2.5.5
>