Re: mm/mremap: F_SEAL_FUTURE_WRITE can be bypassed via mremap() VMA expansion/duplication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] mm/mremap: check_prep_vma() checks memfd seals before VMA bounds
- [Low] mm/mremap: check_prep_vma() open-codes vma_is_shared_maywrite()
- [Low] mm/mremap: commit message is a raw bug report and lacks trailers
--

commit cc4a27e7580c449c0e2b8a3507c3d641a688a645
Author: Junyoung Jang <graypanda.inzag@gmail.com>

> mm/mremap: F_SEAL_FUTURE_WRITE can be bypassed via mremap() VMA expansion/duplication
>
> Hello,
>
> I found a reproducible F_SEAL_FUTURE_WRITE seal-policy bypass. A process
> holding a one-page shared writable memfd mapping created before the seal can
> use mremap() after the seal to gain writable access to additional file pages.
>
> In the accompanying two-UID reproducer, the second page belongs to a memfd
> created by a different unprivileged user. While a new writable mmap() and
> pwrite() to that second page both fail with -EPERM, the mremap() path
> succeeds and modifies the page's contents.
>
> Affected tree tested:
>
> - Linux 7.4.0-rc1-gd48723711bae, built directly from upstream commit
>   d48723711bae9b8cdc56719c448ba42fef28cbee using x86_64_defconfig.
> - A second image built from the same commit with the proposed mm/mremap.c
>   diff, reported by uname as 7.4.0-rc1-gd48723711bae-dirty.
> - Each before/after pair was booted in QEMU with the same statically
>   linked C PoC.
>
> Root cause:
>
> The missing seal check is in mm/mremap.c:check_prep_vma() (lines 1808-1872
> of the tested commit). mmap() calls memfd_check_seals_mmap() at
> mm/mmap.c:499; for F_SEAL_FUTURE_WRITE, mm/memfd.c:384-385 rejects a
> new shared writable mapping with EPERM. check_prep_vma() has no equivalent
> check before it permits a larger mapping.
>
> For the PoC's old_len=0 call, this condition at mm/mremap.c:1819 only
> rejects duplication of a non-shared VMA:
>
>     if (!old_len && !vma_test_any(vma, VMA_SHARED_BIT, VMA_MAYSHARE_BIT)) {
>
> It is false for the PoC's MAP_SHARED VMA. check_prep_vma() then checks
> VMA bounds and expansion limits and returns 0 at mm/mremap.c:1872 without
> checking vma->vm_file for write seals. For this zero-length call,
> expand_vma() moves the VMA and reaches copy_vma_and_data(), which calls
> copy_vma() with the larger new_len (mm/mremap.c:1293-1295):
>
>     new_vma = copy_vma(&vma, vrm->new_addr, vrm->new_len, new_pgoff,
>                        new_anon_pgoff, &pmc.need_rmap_locks,
>                        vrm->flags & MREMAP_DONTUNMAP);
>
> copy_vma() can duplicate the original VMA and extend its range
> (mm/vma.c:2052-2055), retaining VM_WRITE and VM_MAYWRITE. The newly
> covered second memfd page is writable despite F_SEAL_FUTURE_WRITE.
> Ordinary expansion with old_len=one page also succeeded on the unpatched
> kernel. A read-only shared mapping with VM_MAYWRITE can likewise be
> expanded and then changed to PROT_WRITE with mprotect().
>
> Impact:
>
> This allows a process to write to file pages outside its pre-seal mapping
> across a UID boundary, affecting scenarios where a memfd owner grants
> limited writable access to a specific range before sealing and relies on
> F_SEAL_FUTURE_WRITE to prevent future writable mappings of other pages.
>
> Reproducer summary:
>
> 1. Start as root only to set up two distinct unprivileged UIDs (10002 for
>    the owner, 10001 for the writer); both drop all capabilities.
> 2. UID 10002 creates a two-page memfd and initializes page two to 'A'.
> 3. UID 10002 passes the fd to UID 10001 via SCM_RIGHTS.
> 4. UID 10001 maps only page one with MAP_SHARED and PROT_WRITE.
> 5. UID 10002 adds F_SEAL_FUTURE_WRITE.
> 6. UID 10001 confirms a fresh writable mmap() of page two and pwrite() to it
>    both fail with -EPERM.
> 7. UID 10001 calls mremap(old_addr, 0, 2 * page_size, MREMAP_MAYMOVE),
>    writes 'Z' through the expanded mapping, and UID 10002 reads 'Z' back
>    from the second file page.
>
> Build and run:
>   $ cc -O0 -g -Wall -Wextra -Werror -static memfd_future_write_mremap_poc.c -o poc
>   $ sudo ./poc
>   writer uid=10001; mmap=EPERM; pwrite=EPERM; mremap write=Z; result=63
>   owner uid=10002; file page 2=Z; size=8192; result=255
>
> Suggested fix direction:
>
> I believe mremap() needs to check the memfd write seal before duplicating
> (old_len=0) or expanding (new_len > old_len) a shared VMA that has
> VM_MAYWRITE.
>
> I tested the rough diff below in check_prep_vma() on the same commit and
> confirmed that it blocks the reproducer (mremap fails with -EPERM). However,
> I am sending this as a bug report rather than a formal [PATCH] because I am
> unsure about potential side effects:
>
> 1. Because a PROT_READ shared mapping with VM_MAYWRITE could be expanded and
>    later made writable via mprotect(), the diff temporarily sets VMA_WRITE_BIT
>    on a local flags copy to force memfd_check_seals_mmap() to return -EPERM.
>    However, this rejects legitimate read-only mremap() expansions outright,
>    whereas mmap() would normally allow a read-only mapping and simply clear
>    VM_MAYWRITE.
> 2. I am not certain whether check_prep_vma() is the right place regarding
>    locking/synchronization against a concurrent F_ADD_SEALS call.
>
> I would appreciate your guidance on the proper way to handle this, or feel
> free to take it from here:

[Severity: Low]
The commit message is formatted as a conversational bug report rather than
a kernel changelog and is missing a Signed-off-by: trailer.  It should also
include:

  Fixes: ab3948f58ff8 ("mm/memfd: add an F_SEAL_FUTURE_WRITE seal to memfd")

> diff --git a/mm/mremap.c b/mm/mremap.c
> index 19398d5fca585..cc24716120d20 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
[ ... ]
> @@ -1829,6 +1830,17 @@ static int check_prep_vma(struct vma_remap_struct *vrm)
>  
>  	old_len = vrm->old_len;
>  	new_len = vrm->new_len;
> +	if (new_len > old_len && vma->vm_file &&
> +	    vma_test_all(vma, VMA_SHARED_BIT, VMA_MAYWRITE_BIT)) {
> +		vma_flags_t flags = vma->flags;
> +		int err;
> +
> +		/* A larger VMA could become writable via mprotect(). */
> +		vma_flags_set(&flags, VMA_WRITE_BIT);
> +		err = memfd_check_seals_mmap(vma->vm_file, &flags);
> +		if (err)
> +			return err;
> +	}

[Severity: Low]
Should this check be placed in the expansion section of check_prep_vma()
after the VMA bounds check?

Running it here causes mremap() with an out-of-bounds old_len
(old_len > vma->vm_end - addr) and new_len > old_len on a write-sealed
shared memfd mapping to return -EPERM instead of -EFAULT.  Moving it past
the old_len == new_len early return also avoids duplicating the
new_len > old_len check.

[Severity: Low]
This isn't a bug, but vma_test_all(vma, VMA_SHARED_BIT, VMA_MAYWRITE_BIT)
open-codes the existing vma_is_shared_maywrite(vma) helper.  In addition,
memfd_check_seals_mmap() is meant to mutate a new VMA's flags during
mmap(), so copying vma->flags and setting a dummy VMA_WRITE_BIT just to
trigger its -EPERM branch is awkward compared to a dedicated memfd helper.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107030708.1031977-1-graypanda.inzag@gmail.com?part=1