Regression: mmap rejects shared, read-only mappings of write-sealed memfds

Regression: mmap rejects shared, read-only mappings of write-sealed memfds

[Julian Orth]

Since around

    5de19506 mm: resolve faulty mmap_region() error path behaviour

mmap rejects shared, read-only mapping of memfds that have a write-seal applied.

Before the commit, the code in mmap_region was

    if (file) {
        vma->vm_file = get_file(file);
        error = mmap_file(file, vma);
        if (error)
            goto unmap_and_free_vma;

        if (vma_is_shared_maywrite(vma)) {
            error = mapping_map_writable(file->f_mapping);

where mmap_file would clear the VM_MAYWRITE flag for write-sealed memfds.

After the commit, the code in mmap_region is simply

    if (file && is_shared_maywrite(vm_flags)) {
        int error = mapping_map_writable(file->f_mapping);

with mmap_file not being called until much later.

This regression seems to have been first released in 6.12 and is still
present on master.

Re: Regression: mmap rejects shared, read-only mappings of write-sealed memfds

[Lorenzo Stoakes]

+ VMA people, Linus

On Wed, Nov 27, 2024 at 09:49:29PM +0100, Julian Orth wrote:
> Since around
>
>     5de19506 mm: resolve faulty mmap_region() error path behaviour
>
> mmap rejects shared, read-only mapping of memfds that have a write-seal applied.
>
> Before the commit, the code in mmap_region was
>
>     if (file) {
>         vma->vm_file = get_file(file);
>         error = mmap_file(file, vma);
>         if (error)
>             goto unmap_and_free_vma;
>
>         if (vma_is_shared_maywrite(vma)) {
>             error = mapping_map_writable(file->f_mapping);
>
> where mmap_file would clear the VM_MAYWRITE flag for write-sealed memfds.
>
> After the commit, the code in mmap_region is simply
>
>     if (file && is_shared_maywrite(vm_flags)) {
>         int error = mapping_map_writable(file->f_mapping);
>
> with mmap_file not being called until much later.
>
> This regression seems to have been first released in 6.12 and is still
> present on master.

Thanks, this is ironic as I was the one who made this behaviour possible in
commit e8e17ee90eaf ("mm: drop the assumption that VM_SHARED always implies
writable") :)

This means this functionality was only available from 6.6, and is pretty
corner-case niche stuff (code written for any prior kernel could not rely
on this being possible), so the number of people impacted by this will be
minimal.

I will look into this and see if it is feasible to resolve it.

However it is of critical importance for security and stability purposes
that we do not abort the mmap operation midway through, and therefore we
cannot have a case where we abort _after_ the mmap_file() call (which calls
the f_op->mmap() hook), so the behaviour as originally implemented simply
cannot be restored.

A workaround might be an icky special case for memfd's or even a
refactoring of this code in general...

Thanks, Lorenzo