Re: [PATCH v2 08/10] mm/shmem_luo: Allow preserving tmpfs files that are still linked

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/shmem_luo: missing FMODE_READ and MAY_READ check in shmem_luo_may_preserve()
- [Medium] mm/shmem_luo: missing IS_APPEND() check in shmem_luo_may_preserve()
--

commit c078673247a58afcd0e7d24e1253ad5aeb43414c
Author: David Matlack <dmatlack@google.com>

mm/shmem_luo: Allow preserving tmpfs files that are still linked

This commit allows preserving tmpfs files that still have links in the
filesystem by removing the !inode->i_nlink restriction in
shmem_luo_can_preserve(). Because preserving a file freezes its size for all
users, it adds shmem_luo_may_preserve() to require write permission (or
ownership when the file is immutable or on a read-only superblock) and
documents the new behavior.

> diff --git a/mm/shmem_luo.c b/mm/shmem_luo.c
> index ecbeb37d33a8a..aba2bc1f18c52 100644
> --- a/mm/shmem_luo.c
> +++ b/mm/shmem_luo.c

[ ... ]

> @@ -298,6 +315,34 @@ static void shmem_luo_unpreserve_folios(struct kho_vmalloc *kho_vmalloc,
>  	vfree(folios_ser);
>  }
>  
> +/*
> + * Check whether the caller may preserve @file. Preserving the file freezes its
> + * size until it is unpreserved, for all users of the file, so require
> + * permission to write to the file. Like FS_IOC_ENABLE_VERITY, don't require a
> + * file opened for writing, so that e.g. a binary that is being executed can be
> + * preserved, nor a writable mount. If the size of the file can't change
> + * anyway, because it is on a read-only superblock or immutable, its owner may
> + * also preserve it. The size could only change again after the file is made
> + * writable, which is up to the owner or root.
> + *
> + * Preserving the file also fills its holes, but that doesn't need any
> + * permission: Anyone who can read the file can fill its holes by faulting
> + * them in through a read-only mapping.
> + */
> +static int shmem_luo_may_preserve(struct file *file)
> +{
> +	struct mnt_idmap *idmap = file_mnt_idmap(file);
> +	struct inode *inode = file_inode(file);
> +	int err;
> +
> +	err = inode_permission(idmap, inode, MAY_WRITE);

[Severity: High]
Should shmem_luo_may_preserve() in mm/shmem_luo.c also verify that the file
is open for reading (file->f_mode & FMODE_READ) or that the caller has
MAY_READ permission?

The comment above notes that anyone who can read the file can fill its holes
through a read-only mapping, and models the check on FS_IOC_ENABLE_VERITY.
However, fsverity_ioctl_enable() in fs/verity/enable.c explicitly checks
FMODE_READ in addition to MAY_WRITE:

    err = file_permission(filp, MAY_WRITE);
    if (err)
        return err;
    if (!(filp->f_mode & FMODE_READ))
        return -EBADF;

Similarly, do_mmap() in mm/mmap.c rejects mapping a file when FMODE_READ is
not set:

    if (!(file->f_mode & FMODE_READ))
        return -EACCES;

Without a read check in shmem_luo_may_preserve(), a user who opens a
write-only tmpfs file (for example, mode 0622 or 0222 in /dev/shm owned by
another user) with O_WRONLY, or who holds an ioctl-only mode-3 file
descriptor lacking FMODE_READ, can pass it to LIVEUPDATE_SESSION_PRESERVE_FD:

    luo_preserve_file()
      -> shmem_luo_preserve()
        -> shmem_luo_may_preserve()
        -> shmem_luo_preserve_folios()
          -> memfd_pin_folios()

This faults in and pins folios for the entire file (including holes), and on
LIVEUPDATE_SESSION_RETRIEVE_FD, shmem_luo_retrieve() ->
shmem_luo_create_tmpfs_file() in mm/shmem_luo.c places those folios into a
new tmpfs file opened with caller-chosen flags (such as O_RDWR or O_RDONLY)
and mode:

    file = file_open_root(&fd_file(dir)->f_path, ".",
                          O_TMPFILE | O_LARGEFILE | arg->flags, arg->mode);

Wouldn't that allow a caller with only write access to read the file's
contents after retrieval?

> +	if (err && (sb_rdonly(inode->i_sb) || IS_IMMUTABLE(inode)) &&
> +	    inode_owner_or_capable(idmap, inode))
> +		return 0;
> +
> +	return err;
> +}

[Severity: Medium]
What happens if shmem_luo_may_preserve() in mm/shmem_luo.c is called on an
append-only (S_APPEND) tmpfs inode?

Tmpfs supports setting S_APPEND via FS_IOC_SETFLAGS in
shmem_fileattr_set() -> shmem_set_inode_flags() in mm/shmem.c:

    if (fsflags & FS_APPEND_FL)
        i_flags |= S_APPEND;

Unlike IS_IMMUTABLE(inode), inode_permission(idmap, inode, MAY_WRITE) does
not reject S_APPEND inodes. In fs/verity/enable.c, fsverity_ioctl_enable()
explicitly checks IS_APPEND(inode) after checking MAY_WRITE:

    if (IS_APPEND(inode))
        return -EPERM;

When a caller with MAY_WRITE permission on an S_APPEND tmpfs file preserves
it via LIVEUPDATE_SESSION_PRESERVE_FD -> luo_preserve_file() ->
shmem_luo_preserve(), shmem_freeze(inode, true) sets SHMEM_F_MAPPING_FROZEN.
Because all writes to an S_APPEND file extend past i_size,
shmem_write_begin() and shmem_fallocate() in mm/shmem.c will then fail with
-EPERM:

    if (unlikely((info->flags & SHMEM_F_MAPPING_FROZEN) &&
                 pos + len > inode->i_size))
        return -EPERM;

Should shmem_luo_may_preserve() also reject IS_APPEND(inode) with -EPERM so
an append-only writer cannot freeze the file and block future appends by all
users?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022225035.377445-1-dmatlack@google.com?part=8