* [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map()
@ 2017-10-06 3:37 Baoquan He
2017-10-06 3:37 ` [PATCH 1/3] binfmt_elf: Clean up the variable name of map flags Baoquan He
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Baoquan He @ 2017-10-06 3:37 UTC (permalink / raw)
To: linux-kernel
Cc: viro, linux-fsdevel, oleg, mhocko, keescook, jkosina, mingo,
torvalds, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] binfmt_elf: Clean up the variable name of map flags
2017-10-06 3:37 [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
@ 2017-10-06 3:37 ` Baoquan He
2017-10-06 3:37 ` [PATCH 2/3] binfmt_elf: Get the total_size only for dynamic loader in load_elf_binary() Baoquan He
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Baoquan He @ 2017-10-06 3:37 UTC (permalink / raw)
To: linux-kernel
Cc: viro, linux-fsdevel, oleg, mhocko, keescook, jkosina, mingo,
torvalds, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] binfmt_elf: Get the total_size only for dynamic loader in load_elf_binary()
2017-10-06 3:37 [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
2017-10-06 3:37 ` [PATCH 1/3] binfmt_elf: Clean up the variable name of map flags Baoquan He
@ 2017-10-06 3:37 ` Baoquan He
2017-10-06 3:37 ` [PATCH 3/3] binfmt_elf: Search an unmapped area with total_size but not map the whole image Baoquan He
2017-11-07 12:00 ` [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
3 siblings, 0 replies; 5+ messages in thread
From: Baoquan He @ 2017-10-06 3:37 UTC (permalink / raw)
To: linux-kernel
Cc: viro, linux-fsdevel, oleg, mhocko, keescook, jkosina, mingo,
torvalds, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] binfmt_elf: Search an unmapped area with total_size but not map the whole image
2017-10-06 3:37 [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
2017-10-06 3:37 ` [PATCH 1/3] binfmt_elf: Clean up the variable name of map flags Baoquan He
2017-10-06 3:37 ` [PATCH 2/3] binfmt_elf: Get the total_size only for dynamic loader in load_elf_binary() Baoquan He
@ 2017-10-06 3:37 ` Baoquan He
2017-11-07 12:00 ` [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
3 siblings, 0 replies; 5+ messages in thread
From: Baoquan He @ 2017-10-06 3:37 UTC (permalink / raw)
To: linux-kernel
Cc: viro, linux-fsdevel, oleg, mhocko, keescook, jkosina, mingo,
torvalds, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map()
2017-10-06 3:37 [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
` (2 preceding siblings ...)
2017-10-06 3:37 ` [PATCH 3/3] binfmt_elf: Search an unmapped area with total_size but not map the whole image Baoquan He
@ 2017-11-07 12:00 ` Baoquan He
3 siblings, 0 replies; 5+ messages in thread
From: Baoquan He @ 2017-11-07 12:00 UTC (permalink / raw)
To: linux-kernel
Cc: viro, linux-fsdevel, oleg, mhocko, keescook, jkosina, mingo,
torvalds, akpm
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-07 12:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-06 3:37 [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
2017-10-06 3:37 ` [PATCH 1/3] binfmt_elf: Clean up the variable name of map flags Baoquan He
2017-10-06 3:37 ` [PATCH 2/3] binfmt_elf: Get the total_size only for dynamic loader in load_elf_binary() Baoquan He
2017-10-06 3:37 ` [PATCH 3/3] binfmt_elf: Search an unmapped area with total_size but not map the whole image Baoquan He
2017-11-07 12:00 ` [PATCH 0/3] binfmt_elf: Clean up codes related to total_size passed into elf_map() Baoquan He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).