diff mbox series

[v2,1/7] binfmt: don't use MAP_DENYWRITE when loading shared libraries via uselib()

Message ID 20210816194840.42769-2-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series Remove in-tree usage of MAP_DENYWRITE | expand

Commit Message

David Hildenbrand Aug. 16, 2021, 7:48 p.m. UTC
uselib() is the legacy systemcall for loading shared libraries.
Nowadays, applications use dlopen() to load shared libraries, completely
implemented in user space via mmap().

For example, glibc uses MAP_COPY to mmap shared libraries. While this
maps to MAP_PRIVATE | MAP_DENYWRITE on Linux, Linux ignores any
MAP_DENYWRITE specification from user space in mmap.

With this change, all remaining in-tree users of MAP_DENYWRITE use it
to map an executable. We will be able to open shared libraries loaded
via uselib() writable, just as we already can via dlopen() from user
space.

This is one step into the direction of removing MAP_DENYWRITE from the
kernel. This can be considered a minor user space visible change.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/ia32/ia32_aout.c | 2 +-
 fs/binfmt_aout.c          | 2 +-
 fs/binfmt_elf.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Guenter Roeck Sept. 5, 2021, 3:32 p.m. UTC | #1
On Mon, Aug 16, 2021 at 09:48:34PM +0200, David Hildenbrand wrote:
> uselib() is the legacy systemcall for loading shared libraries.
> Nowadays, applications use dlopen() to load shared libraries, completely
> implemented in user space via mmap().
> 
> For example, glibc uses MAP_COPY to mmap shared libraries. While this
> maps to MAP_PRIVATE | MAP_DENYWRITE on Linux, Linux ignores any
> MAP_DENYWRITE specification from user space in mmap.
> 
> With this change, all remaining in-tree users of MAP_DENYWRITE use it
> to map an executable. We will be able to open shared libraries loaded
> via uselib() writable, just as we already can via dlopen() from user
> space.
> 
> This is one step into the direction of removing MAP_DENYWRITE from the
> kernel. This can be considered a minor user space visible change.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/x86/ia32/ia32_aout.c | 2 +-
>  fs/binfmt_aout.c          | 2 +-
>  fs/binfmt_elf.c           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
> index 5e5b9fc2747f..321d7b22ad2d 100644
> --- a/arch/x86/ia32/ia32_aout.c
> +++ b/arch/x86/ia32/ia32_aout.c
> @@ -293,7 +293,7 @@ static int load_aout_library(struct file *file)
>  	/* Now use mmap to map the library into memory. */
>  	error = vm_mmap(file, start_addr, ex.a_text + ex.a_data,
>  			PROT_READ | PROT_WRITE | PROT_EXEC,
> -			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_32BIT,
> +			MAP_FIXED | MAP_PRIVATE | MAP_32BIT,
>  			N_TXTOFF(ex));
>  	retval = error;
>  	if (error != start_addr)
> diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
> index 145917f734fe..d29de971d3f3 100644
> --- a/fs/binfmt_aout.c
> +++ b/fs/binfmt_aout.c
> @@ -309,7 +309,7 @@ static int load_aout_library(struct file *file)
>  	/* Now use mmap to map the library into memory. */
>  	error = vm_mmap(file, start_addr, ex.a_text + ex.a_data,
>  			PROT_READ | PROT_WRITE | PROT_EXEC,
> -			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
> +			MAP_FIXED | MAP_PRIVATE;
>  			N_TXTOFF(ex));

Guess someone didn't care compile testing their code. This is now in
mainline.

Guenter
Linus Torvalds Sept. 5, 2021, 5:17 p.m. UTC | #2
On Sun, Sep 5, 2021 at 8:32 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Guess someone didn't care compile testing their code. This is now in
> mainline.

To be fair, a.out is disabled pretty much on all relevant platforms these days.

Only alpha and m68k left, I think.

I applied the obvious patch from Geert.

            Linus
David Hildenbrand Sept. 5, 2021, 7:07 p.m. UTC | #3
On 05.09.21 19:17, Linus Torvalds wrote:
> On Sun, Sep 5, 2021 at 8:32 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Guess someone didn't care compile testing their code. This is now in
>> mainline.
> 
> To be fair, a.out is disabled pretty much on all relevant platforms these days.

Yes, and it seems like it was disabled in all configs I used. (I did not 
compile all-yes configs; usually my stuff goes via -mm where it will end 
up in -next for a while ... this one was special)

> 
> Only alpha and m68k left, I think.
> 
> I applied the obvious patch from Geert.

Thanks Linus!
diff mbox series

Patch

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 5e5b9fc2747f..321d7b22ad2d 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -293,7 +293,7 @@  static int load_aout_library(struct file *file)
 	/* Now use mmap to map the library into memory. */
 	error = vm_mmap(file, start_addr, ex.a_text + ex.a_data,
 			PROT_READ | PROT_WRITE | PROT_EXEC,
-			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_32BIT,
+			MAP_FIXED | MAP_PRIVATE | MAP_32BIT,
 			N_TXTOFF(ex));
 	retval = error;
 	if (error != start_addr)
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 145917f734fe..d29de971d3f3 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -309,7 +309,7 @@  static int load_aout_library(struct file *file)
 	/* Now use mmap to map the library into memory. */
 	error = vm_mmap(file, start_addr, ex.a_text + ex.a_data,
 			PROT_READ | PROT_WRITE | PROT_EXEC,
-			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
+			MAP_FIXED | MAP_PRIVATE;
 			N_TXTOFF(ex));
 	retval = error;
 	if (error != start_addr)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 439ed81e755a..6d2c79533631 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1384,7 +1384,7 @@  static int load_elf_library(struct file *file)
 			(eppnt->p_filesz +
 			 ELF_PAGEOFFSET(eppnt->p_vaddr)),
 			PROT_READ | PROT_WRITE | PROT_EXEC,
-			MAP_FIXED_NOREPLACE | MAP_PRIVATE | MAP_DENYWRITE,
+			MAP_FIXED_NOREPLACE | MAP_PRIVATE,
 			(eppnt->p_offset -
 			 ELF_PAGEOFFSET(eppnt->p_vaddr)));
 	if (error != ELF_PAGESTART(eppnt->p_vaddr))