[PATCH] Remove MAP_DENYWRITE FROM MAP_COPY definition

[PATCH] Remove MAP_DENYWRITE FROM MAP_COPY definition

[Dmitrii Shcherbakov]

Hello,

What I found out is that MAP_DENYWRITE is not used in any significant
place in the Linux kernel. Only compatibility flag handling is present
but it has no effect on the mmap system call.

The mmap manpage contains a note about this flag being deprecated long
ago:

.B MAP_DENYWRITE
This flag is ignored.
.\" Introduced in 1.1.36, removed in 1.3.24.
(Long ago, it signaled that attempts to write to the underlying file
should fail with
.BR ETXTBUSY .
But this was a source of denial-of-service attacks.)

cscope output for the kernel code:

git rev-parse HEAD
07be1337b9e8bfcd855c6e9175b5066a30ac609b

cscope -d

C symbol: MAP_DENYWRITE

  File               Function                          Line
0 mman.h             <global>                            25 #define MAP_DENYWRITE 0x02000
1 mman.h             <global>                            44 #define MAP_DENYWRITE 0x2000
2 mman.h             <global>                            18 #define MAP_DENYWRITE 0x0800
3 mman.h             <global>                            20 #define MAP_DENYWRITE 0x0800
4 mman.h             <global>                            15 #define MAP_DENYWRITE 0x0800
5 mman.h             <global>                            29 #define MAP_DENYWRITE 0x0800
6 mman.h             <global>                            51 #define MAP_DENYWRITE 0x2000
7 mman.h             <global>                             7 #define MAP_DENYWRITE 0x0800
8 ia32_aout.c        load_aout_binary                   360 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
9 ia32_aout.c        load_aout_binary                   369 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
a ia32_aout.c        load_aout_library                  447 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_32BIT,
b binfmt_aout.c      load_aout_binary                   308 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
c binfmt_aout.c      load_aout_binary                   316 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
d binfmt_aout.c      load_aout_library                  391 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
e binfmt_elf.c       load_elf_interp                    551 int elf_type = MAP_PRIVATE | MAP_DENYWRITE;
f binfmt_elf.c       load_elf_binary                    909 elf_flags = MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE;
g binfmt_elf.c       load_elf_library                  1164 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
h binfmt_elf_fdpic.c elf_fdpic_map_file_by_direct_mmap 1063 flags = MAP_PRIVATE | MAP_DENYWRITE;
i mman.h             calc_vm_flag_bits                   88 _calc_vm_trans(flags, MAP_DENYWRITE, VM_DENYWRITE ) |
j core.c             perf_event_mmap_event             6397 flags |= MAP_DENYWRITE;
k mmap.c             SYSCALL_DEFINE6                   1333 flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
l nommu.c            SYSCALL_DEFINE6                   1447 flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);

I found this looking at an strace output for some binary - turns out
shared libraries are mmap-ed using this flag because of the MAP_COPY definition.

I suggest we remove it so that it makes more sense.

Dmitrii Shcherbakov (1):
  dl-load.h: Remove MAP_DENYWRITE from MAP_COPY definition

 elf/dl-load.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH] dl-load.h: Remove MAP_DENYWRITE from MAP_COPY definition

[Dmitrii Shcherbakov]

The Linux kernel no longer uses this flag in mmap
other than for compatibility purposes. In fact,
its support was removed many years ago according to
the mmap man page.

Let's remove it to avoid confusing output in strace
and fix a comment about this flag doing what it does
not anymore.

Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii at gmail.com>

ChangeLog:

	* elf/dl-load.h: Remove MAP_DENYWRITE from MAP_COPY d
---
 elf/dl-load.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/elf/dl-load.h b/elf/dl-load.h
index 9fe7118..622c0bc 100644
--- a/elf/dl-load.h
+++ b/elf/dl-load.h
@@ -37,14 +37,13 @@
    from the new version after starting with pages from the old version.
 
    To make up for the lack and avoid the overwriting problem,
-   what Linux does have is MAP_DENYWRITE.  This prevents anyone
-   from modifying the file while we have it mapped.  */
+   what Linux used to have was MAP_DENYWRITE.
+   This prevented anyone from modifying the file while it was mapped,
+   creating a denial-of-service attack possibility by using mmap given
+   read permissions to a file.
+   */
 #ifndef MAP_COPY
-# ifdef MAP_DENYWRITE
-#  define MAP_COPY      (MAP_PRIVATE | MAP_DENYWRITE)
-# else
-#  define MAP_COPY      MAP_PRIVATE
-# endif
+# define MAP_COPY      MAP_PRIVATE
 #endif
 
 /* Some systems link their relocatable objects for another base address
-- 
2.7.4

Re: [PATCH] Remove MAP_DENYWRITE FROM MAP_COPY definition

[Florian Weimer]

* Dmitrii Shcherbakov:

> Dmitrii Shcherbakov (1):
>   dl-load.h: Remove MAP_DENYWRITE from MAP_COPY definition

The patch seems to be missing, but I can guess what it looks like.

I wonder if the kernel can do a better job here.  Truncation of DSOs
is a common source of application crashes because you either get
SIGBUS immediatelly, or a crash because private writeable mappings are
cleared.  And it's not just about executable mappings, OpenJDK faces
the same issue with mapped JARs.

Re: [PATCH] Remove MAP_DENYWRITE FROM MAP_COPY definition

[Dmitrii Shcherbakov]

Florian,

This one is a cover letter. The patch and its cover are in a single
thread though.

That's the patch:
https://sourceware.org/ml/libc-alpha/2016-09/msg00057.html

I agree about the DSO truncation but MAP_DENYWRITE functionality has
been removed
as it effectively allowed anyone with read permissions to block
writing to a file by using mmap
with this flag.

Right now it just pollutes the strace output and confuses people about how DSOs
work (e.g. why cp of a new library over an old one will most likely
result in a segfault
while mv from the same file system will not).

It is hard for me to tell what kind of mechanism would make a better
job in the Linux kernel.

On Sat, Sep 3, 2016 at 9:58 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Dmitrii Shcherbakov:
>
> > Dmitrii Shcherbakov (1):
> >   dl-load.h: Remove MAP_DENYWRITE from MAP_COPY definition
>
> The patch seems to be missing, but I can guess what it looks like.
>
> I wonder if the kernel can do a better job here.  Truncation of DSOs
> is a common source of application crashes because you either get
> SIGBUS immediatelly, or a crash because private writeable mappings are
> cleared.  And it's not just about executable mappings, OpenJDK faces
> the same issue with mapped JARs.