[PATCH v1 0/3] perf/binfmt/mm: remove in-tree usage of MAP_EXECUTABLE

[PATCH v1 0/3] perf/binfmt/mm: remove in-tree usage of MAP_EXECUTABLE

[David Hildenbrand]

Stumbling over the history of MAP_EXECUTABLE, I noticed that we still
have some in-tree users that we can get rid of.

A good fit for the whole series could be Andrew's tree.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Greg Ungerer <gerg@linux-m68k.org>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kevin Brodsky <Kevin.Brodsky@arm.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: x86@kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org

David Hildenbrand (3):
  perf: MAP_EXECUTABLE does not indicate VM_MAYEXEC
  binfmt: remove in-tree usage of MAP_EXECUTABLE
  mm: ignore MAP_EXECUTABLE in ksys_mmap_pgoff()

 arch/x86/ia32/ia32_aout.c |  4 ++--
 fs/binfmt_aout.c          |  4 ++--
 fs/binfmt_elf.c           |  2 +-
 fs/binfmt_elf_fdpic.c     | 11 ++---------
 fs/binfmt_flat.c          |  2 +-
 include/linux/mman.h      |  2 ++
 kernel/events/core.c      |  2 --
 mm/mmap.c                 |  2 +-
 mm/nommu.c                |  2 +-
 9 files changed, 12 insertions(+), 19 deletions(-)

-- 
2.30.2

[PATCH v1 1/3] perf: MAP_EXECUTABLE does not indicate VM_MAYEXEC

[David Hildenbrand]

Before commit e9714acf8c43 ("mm: kill vma flag VM_EXECUTABLE and
mm->num_exe_file_vmas"), VM_EXECUTABLE indicated MAP_EXECUTABLE.
MAP_EXECUTABLE is nowadays essentially ignored by the kernel and does
not relate to VM_MAYEXEC.

Fixes: f972eb63b100 ("perf: Pass protection and flags bits through mmap2 interface")
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 kernel/events/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 03db40f6cba9..3dfd463f1831 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8186,8 +8186,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 
 	if (vma->vm_flags & VM_DENYWRITE)
 		flags |= MAP_DENYWRITE;
-	if (vma->vm_flags & VM_MAYEXEC)
-		flags |= MAP_EXECUTABLE;
 	if (vma->vm_flags & VM_LOCKED)
 		flags |= MAP_LOCKED;
 	if (is_vm_hugetlb_page(vma))
-- 
2.30.2

[PATCH v1 2/3] binfmt: remove in-tree usage of MAP_EXECUTABLE

[David Hildenbrand]

Ever since commit e9714acf8c43 ("mm: kill vma flag VM_EXECUTABLE and
mm->num_exe_file_vmas"), VM_EXECUTABLE is gone and MAP_EXECUTABLE is
essentially completely ignored. Let's remove all usage of
MAP_EXECUTABLE.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/ia32/ia32_aout.c |  4 ++--
 fs/binfmt_aout.c          |  4 ++--
 fs/binfmt_elf.c           |  2 +-
 fs/binfmt_elf_fdpic.c     | 11 ++---------
 fs/binfmt_flat.c          |  2 +-
 5 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index a09fc37ead9d..5e5b9fc2747f 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -203,7 +203,7 @@ static int load_aout_binary(struct linux_binprm *bprm)
 		error = vm_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
 				PROT_READ | PROT_EXEC,
 				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
-				MAP_EXECUTABLE | MAP_32BIT,
+				MAP_32BIT,
 				fd_offset);
 
 		if (error != N_TXTADDR(ex))
@@ -212,7 +212,7 @@ static int load_aout_binary(struct linux_binprm *bprm)
 		error = vm_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
 				PROT_READ | PROT_WRITE | PROT_EXEC,
 				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
-				MAP_EXECUTABLE | MAP_32BIT,
+				MAP_32BIT,
 				fd_offset + ex.a_text);
 		if (error != N_DATADDR(ex))
 			return error;
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 3e84e9bb9084..12461f3ed04f 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -222,7 +222,7 @@ static int load_aout_binary(struct linux_binprm * bprm)
 
 		error = vm_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
 			PROT_READ | PROT_EXEC,
-			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
+			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE;
 			fd_offset);
 
 		if (error != N_TXTADDR(ex))
@@ -230,7 +230,7 @@ static int load_aout_binary(struct linux_binprm * bprm)
 
 		error = vm_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
 				PROT_READ | PROT_WRITE | PROT_EXEC,
-				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
+				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE;
 				fd_offset + ex.a_text);
 		if (error != N_DATADDR(ex))
 			return error;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b12ba98ae9f5..e0427b817425 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1070,7 +1070,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		elf_prot = make_prot(elf_ppnt->p_flags, &arch_state,
 				     !!interpreter, false);
 
-		elf_flags = MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE;
+		elf_flags = MAP_PRIVATE | MAP_DENYWRITE;
 
 		vaddr = elf_ppnt->p_vaddr;
 		/*
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 3cfd6cd46f26..8723b6686b66 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -928,7 +928,7 @@ static int elf_fdpic_map_file_constdisp_on_uclinux(
 {
 	struct elf32_fdpic_loadseg *seg;
 	struct elf32_phdr *phdr;
-	unsigned long load_addr, base = ULONG_MAX, top = 0, maddr = 0, mflags;
+	unsigned long load_addr, base = ULONG_MAX, top = 0, maddr = 0;
 	int loop, ret;
 
 	load_addr = params->load_addr;
@@ -948,12 +948,8 @@ static int elf_fdpic_map_file_constdisp_on_uclinux(
 	}
 
 	/* allocate one big anon block for everything */
-	mflags = MAP_PRIVATE;
-	if (params->flags & ELF_FDPIC_FLAG_EXECUTABLE)
-		mflags |= MAP_EXECUTABLE;
-
 	maddr = vm_mmap(NULL, load_addr, top - base,
-			PROT_READ | PROT_WRITE | PROT_EXEC, mflags, 0);
+			PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE, 0);
 	if (IS_ERR_VALUE(maddr))
 		return (int) maddr;
 
@@ -1046,9 +1042,6 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 		if (phdr->p_flags & PF_X) prot |= PROT_EXEC;
 
 		flags = MAP_PRIVATE | MAP_DENYWRITE;
-		if (params->flags & ELF_FDPIC_FLAG_EXECUTABLE)
-			flags |= MAP_EXECUTABLE;
-
 		maddr = 0;
 
 		switch (params->flags & ELF_FDPIC_FLAG_ARRANGEMENT) {
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index b9c658e0548e..98c2329d23d7 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -567,7 +567,7 @@ static int load_flat_file(struct linux_binprm *bprm,
 		pr_debug("ROM mapping of file (we hope)\n");
 
 		textpos = vm_mmap(bprm->file, 0, text_len, PROT_READ|PROT_EXEC,
-				  MAP_PRIVATE|MAP_EXECUTABLE, 0);
+				  MAP_PRIVATE, 0);
 		if (!textpos || IS_ERR_VALUE(textpos)) {
 			ret = textpos;
 			if (!textpos)
-- 
2.30.2

Re: [PATCH v1 2/3] binfmt: remove in-tree usage of MAP_EXECUTABLE

[David Hildenbrand]

On 21.04.21 11:34, David Hildenbrand wrote:
> Ever since commit e9714acf8c43 ("mm: kill vma flag VM_EXECUTABLE and
> mm->num_exe_file_vmas"), VM_EXECUTABLE is gone and MAP_EXECUTABLE is
> essentially completely ignored. Let's remove all usage of
> MAP_EXECUTABLE.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

[...]

> +++ b/fs/binfmt_aout.c
> @@ -222,7 +222,7 @@ static int load_aout_binary(struct linux_binprm * bprm)
>   
>   		error = vm_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
>   			PROT_READ | PROT_EXEC,
> -			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
> +			MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE;

As reported by kernel test robot, this line should end with a ","

>   			fd_offset);
>   
>   		if (error != N_TXTADDR(ex))
> @@ -230,7 +230,7 @@ static int load_aout_binary(struct linux_binprm * bprm)
>   
>   		error = vm_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
>   				PROT_READ | PROT_WRITE | PROT_EXEC,
> -				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
> +				MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE;
>   				fd_offset + ex.a_text);

dito


@Andrew, I think this resides your tree. Can you fix that up or shall I 
resend the series, or only this individual patch?

-- 
Thanks,

David / dhildenb

[PATCH v1 3/3] mm: ignore MAP_EXECUTABLE in ksys_mmap_pgoff()

[David Hildenbrand]

Let's also remove masking off MAP_EXECUTABLE from ksys_mmap_pgoff():
the last in-tree occurrence of MAP_EXECUTABLE is now in LEGACY_MAP_MASK,
which accepts the flag e.g., for MAP_SHARED_VALIDATE; however, the flag
is ignored throughout the kernel now.

Add a comment to LEGACY_MAP_MASK stating that MAP_EXECUTABLE is ignored.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/linux/mman.h | 2 ++
 mm/mmap.c            | 2 +-
 mm/nommu.c           | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/mman.h b/include/linux/mman.h
index 629cefc4ecba..ebb09a964272 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -31,6 +31,8 @@
 /*
  * The historical set of flags that all mmap implementations implicitly
  * support when a ->mmap_validate() op is not provided in file_operations.
+ *
+ * MAP_EXECUTABLE is completely ignored throughout the kernel.
  */
 #define LEGACY_MAP_MASK (MAP_SHARED \
 		| MAP_PRIVATE \
diff --git a/mm/mmap.c b/mm/mmap.c
index 3f287599a7a3..882f8ee4af1f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1627,7 +1627,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			return PTR_ERR(file);
 	}
 
-	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
+	flags &= ~MAP_DENYWRITE;
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
 out_fput:
diff --git a/mm/nommu.c b/mm/nommu.c
index 5c9ab799c0e6..1fafe3e9d3df 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1306,7 +1306,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			goto out;
 	}
 
-	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
+	flags &= ~MAP_DENYWRITE;
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
 
-- 
2.30.2

Re: [PATCH v1 0/3] perf/binfmt/mm: remove in-tree usage of MAP_EXECUTABLE

[Eric W. Biederman]

David Hildenbrand <david@redhat.com> writes:

> Stumbling over the history of MAP_EXECUTABLE, I noticed that we still
> have some in-tree users that we can get rid of.
>
> A good fit for the whole series could be Andrew's tree.

In general this looks like a good cleanup.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

As far as I can see we can go after MAP_DENYWRITE the same way.
Today deny_write_access in open_exec is what causes -ETXTBSY
when attempting to write to file that is current executing.

Do you have any plans to look at that?

Eric

> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Greg Ungerer <gerg@linux-m68k.org>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kevin Brodsky <Kevin.Brodsky@arm.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Feng Tang <feng.tang@intel.com>
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: x86@kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-mm@kvack.org
>
> David Hildenbrand (3):
>   perf: MAP_EXECUTABLE does not indicate VM_MAYEXEC
>   binfmt: remove in-tree usage of MAP_EXECUTABLE
>   mm: ignore MAP_EXECUTABLE in ksys_mmap_pgoff()
>
>  arch/x86/ia32/ia32_aout.c |  4 ++--
>  fs/binfmt_aout.c          |  4 ++--
>  fs/binfmt_elf.c           |  2 +-
>  fs/binfmt_elf_fdpic.c     | 11 ++---------
>  fs/binfmt_flat.c          |  2 +-
>  include/linux/mman.h      |  2 ++
>  kernel/events/core.c      |  2 --
>  mm/mmap.c                 |  2 +-
>  mm/nommu.c                |  2 +-
>  9 files changed, 12 insertions(+), 19 deletions(-)

Re: [PATCH v1 0/3] perf/binfmt/mm: remove in-tree usage of MAP_EXECUTABLE

[Kees Cook]

On Wed, Apr 21, 2021 at 02:03:49PM -0500, Eric W. Biederman wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
> > Stumbling over the history of MAP_EXECUTABLE, I noticed that we still
> > have some in-tree users that we can get rid of.
> >
> > A good fit for the whole series could be Andrew's tree.
> 
> In general this looks like a good cleanup.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Yeah, the PROT_EXEC parts are the only piece with meaning in the exec
allocations.

Reviewed-by: Kees Cook <keescook@chromium.org>

> As far as I can see we can go after MAP_DENYWRITE the same way.
> Today deny_write_access in open_exec is what causes -ETXTBSY
> when attempting to write to file that is current executing.

Oh, interesting point. I didn't realize MAP_DENYWRITE was separate from
deny_write_access().

-Kees

-- 
Kees Cook

Re: [PATCH v1 0/3] perf/binfmt/mm: remove in-tree usage of MAP_EXECUTABLE

[David Hildenbrand]

On 21.04.21 21:03, Eric W. Biederman wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> Stumbling over the history of MAP_EXECUTABLE, I noticed that we still
>> have some in-tree users that we can get rid of.
>>
>> A good fit for the whole series could be Andrew's tree.
> 
> In general this looks like a good cleanup.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> As far as I can see we can go after MAP_DENYWRITE the same way.
> Today deny_write_access in open_exec is what causes -ETXTBSY
> when attempting to write to file that is current executing.
> 
> Do you have any plans to look at that?

I did have a look and thought about ripping out MAP_DENYWRITE; I'm still 
in the process of connecting the pieces.

Some VM_DENYWRITE e.g., in mm/khugepaged.c still gives me a headache , 
and I'll have to double-check whether it would be okay to just fallback 
on checking the actual vm->file.

-- 
Thanks,

David / dhildenb