mm: drop the assumption that VM_SHARED always implies writable

[ Upstream commit e8e17ee90eaf650c855adb0a3e5e965fd6692ff1 ]

Patch series "permit write-sealed memfd read-only shared mappings", v4.

The man page for fcntl() describing memfd file seals states the following
about F_SEAL_WRITE:-

    Furthermore, trying to create new shared, writable memory-mappings via
    mmap(2) will also fail with EPERM.

With emphasis on 'writable'.  In turns out in fact that currently the
kernel simply disallows all new shared memory mappings for a memfd with
F_SEAL_WRITE applied, rendering this documentation inaccurate.

This matters because users are therefore unable to obtain a shared mapping
to a memfd after write sealing altogether, which limits their usefulness.
This was reported in the discussion thread [1] originating from a bug
report [2].

This is a product of both using the struct address_space->i_mmap_writable
atomic counter to determine whether writing may be permitted, and the
kernel adjusting this counter when any VM_SHARED mapping is performed and
more generally implicitly assuming VM_SHARED implies writable.

It seems sensible that we should only update this mapping if VM_MAYWRITE
is specified, i.e.  whether it is possible that this mapping could at any
point be written to.

If we do so then all we need to do to permit write seals to function as
documented is to clear VM_MAYWRITE when mapping read-only.  It turns out
this functionality already exists for F_SEAL_FUTURE_WRITE - we can
therefore simply adapt this logic to do the same for F_SEAL_WRITE.

We then hit a chicken and egg situation in mmap_region() where the check
for VM_MAYWRITE occurs before we are able to clear this flag.  To work
around this, perform this check after we invoke call_mmap(), with careful
consideration of error paths.

Thanks to Andy Lutomirski for the suggestion!

[1]:https://lore.kernel.org/all/20230324133646.16101dfa666f253c4715d965@linux-foundation.org/
[2]:https://bugzilla.kernel.org/show_bug.cgi?id=217238

This patch (of 3):

There is a general assumption that VMAs with the VM_SHARED flag set are
writable.  If the VM_MAYWRITE flag is not set, then this is simply not the
case.

Update those checks which affect the struct address_space->i_mmap_writable
field to explicitly test for this by introducing
[vma_]is_shared_maywrite() helper functions.

This remains entirely conservative, as the lack of VM_MAYWRITE guarantees
that the VMA cannot be written to.

Link: https://lkml.kernel.org/r/cover.1697116581.git.lstoakes@gmail.com
Link: https://lkml.kernel.org/r/d978aefefa83ec42d18dfa964ad180dbcde34795.1697116581.git.lstoakes@gmail.com
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Muchun Song <muchun.song@linux.dev>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Cc: stable@vger.kernel.org
[isaacmanjarres: resolved merge conflicts due to
due to refactoring that happened in upstream commit
5de195060b2e ("mm: resolve faulty mmap_region() error path behaviour")]
Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9463dddce6bf7d0572994e0bc638331b8e8b726a..11294a89a53bdfe17be5a4847d962318e61ca2d1 100644 (file)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -436,7 +436,7 @@ int pagecache_write_end(struct file *, struct address_space *mapping,
  * @host: Owner, either the inode or the block_device.
  * @i_pages: Cached pages.
  * @gfp_mask: Memory allocation flags to use for allocating pages.
- * @i_mmap_writable: Number of VM_SHARED mappings.
+ * @i_mmap_writable: Number of VM_SHARED, VM_MAYWRITE mappings.
  * @nr_thps: Number of THPs in the pagecache (non-shmem only).
  * @i_mmap: Tree of private and shared mappings.
  * @i_mmap_rwsem: Protects @i_mmap and @i_mmap_writable.
@@ -535,7 +535,7 @@ static inline int mapping_mapped(struct address_space *mapping)
 
 /*
  * Might pages of this file have been modified in userspace?
- * Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap
+ * Note that i_mmap_writable counts all VM_SHARED, VM_MAYWRITE vmas: do_mmap
  * marks vma as VM_SHARED if it is shared, and the file was opened for
  * writing i.e. vma may be mprotected writable even if now readonly.
  *

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e159a11424f1a7fbc099bf1cdb33f66ba9c14b9c..2bedc9940c472c62ec67ed338f03f820093e7da0 100644 (file)
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -666,6 +666,17 @@ static inline bool vma_is_accessible(struct vm_area_struct *vma)
        return vma->vm_flags & VM_ACCESS_FLAGS;
 }
 
+static inline bool is_shared_maywrite(vm_flags_t vm_flags)
+{
+       return (vm_flags & (VM_SHARED | VM_MAYWRITE)) ==
+               (VM_SHARED | VM_MAYWRITE);
+}
+
+static inline bool vma_is_shared_maywrite(struct vm_area_struct *vma)
+{
+       return is_shared_maywrite(vma->vm_flags);
+}
+
 #ifdef CONFIG_SHMEM
 /*
  * The vma_is_shmem is not inline because it is used only by slow

diff --git a/kernel/fork.c b/kernel/fork.c
index 6ece27056fe9d63468b6ef5f9a07d2f1f74cf59a..cdf28ac4487997535a55311772e68c0602237dd7 100644 (file)
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -561,7 +561,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
                        if (tmp->vm_flags & VM_DENYWRITE)
                                put_write_access(inode);
                        i_mmap_lock_write(mapping);
-                       if (tmp->vm_flags & VM_SHARED)
+                       if (vma_is_shared_maywrite(tmp))
                                mapping_allow_writable(mapping);
                        flush_dcache_mmap_lock(mapping);
                        /* insert tmp into the share list, just after mpnt */

diff --git a/mm/filemap.c b/mm/filemap.c
index 3b0d8c6dd58704823de8485934cba7a6f61eb28b..b98af5680bb9c572489a5faa18d6bb7e4be54485 100644 (file)
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2959,7 +2959,7 @@ int generic_file_mmap(struct file * file, struct vm_area_struct * vma)
  */
 int generic_file_readonly_mmap(struct file *file, struct vm_area_struct *vma)
 {
-       if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE))
+       if (vma_is_shared_maywrite(vma))
                return -EINVAL;
        return generic_file_mmap(file, vma);
 }

diff --git a/mm/madvise.c b/mm/madvise.c
index a63aa04ec7fa311fa1d8dd331c60de1b3a83501c..370d0ef719ebecf2854448250f43a964f70a53af 100644 (file)
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -848,7 +848,7 @@ static long madvise_remove(struct vm_area_struct *vma,
                        return -EINVAL;
        }
 
-       if ((vma->vm_flags & (VM_SHARED|VM_WRITE)) != (VM_SHARED|VM_WRITE))
+       if (!vma_is_shared_maywrite(vma))
                return -EACCES;
 
        offset = (loff_t)(start - vma->vm_start)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8c188ed3738ac8e2088a58cd923fe4c891a9a0c1..3cc0d56e41ad7cb3728b840c412e9f0765dbf472 100644 (file)
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -144,7 +144,7 @@ static void __remove_shared_vm_struct(struct vm_area_struct *vma,
 {
        if (vma->vm_flags & VM_DENYWRITE)
                allow_write_access(file);
-       if (vma->vm_flags & VM_SHARED)
+       if (vma_is_shared_maywrite(vma))
                mapping_unmap_writable(mapping);
 
        flush_dcache_mmap_lock(mapping);
@@ -663,7 +663,7 @@ static void __vma_link_file(struct vm_area_struct *vma)
 
                if (vma->vm_flags & VM_DENYWRITE)
                        put_write_access(file_inode(file));
-               if (vma->vm_flags & VM_SHARED)
+               if (vma_is_shared_maywrite(vma))
                        mapping_allow_writable(mapping);
 
                flush_dcache_mmap_lock(mapping);
@@ -2942,7 +2942,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
                return -EINVAL;
 
        /* Map writable and ensure this isn't a sealed memfd. */
-       if (file && (vm_flags & VM_SHARED)) {
+       if (file && is_shared_maywrite(vm_flags)) {
                int error = mapping_map_writable(file->f_mapping);
 
                if (error)