Re: [PATCH RFC v2 33/42] fs: add file_check_backing()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] fs: unverified overlayfs O_PATH d_real_inode() call in
  file_check_backing()
--

commit 12b4c886c3329cc29a995a96a698923b0df5e8e2
Author: Christian Brauner <brauner@kernel.org>

fs: add file_check_backing()

This commit adds file_check_backing() to validate whether a userspace
file can be used as backing storage by checking its open mode, file type,
pseudo-filesystem status, and read/write iterator operations.

> diff --git a/fs/read_write.c b/fs/read_write.c
> index 6767283108991..cedbe654add65 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -866,6 +866,54 @@ static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter,
>  	return ret;
>  }
>  
[ ... ]
> +int file_check_backing(const struct file *file, fmode_t mode, umode_t type)
> +{
> +	fmode_t io = mode & (FMODE_READ | FMODE_WRITE);
> +	/* Drivers may point ->f_mapping at something else. */
> +	umode_t fmt = file_inode(file)->i_mode & S_IFMT;
> +
> +	/* O_PATH files are only fine for callers that just use the path. */
> +	if ((file->f_mode & FMODE_PATH) && !(mode & FMODE_PATH))
> +		return -EBADF;
> +
> +	if ((file->f_mode & io) != io)
> +		return -EBADF;
> +
> +	if (fmt != S_IFREG && !(type == S_IFBLK && fmt == S_IFBLK))
> +		return -EINVAL;
> +
> +	/* Look through stacking filesystems at the inode with the data. */
> +	if (d_real_inode(file->f_path.dentry)->i_sb->s_iflags & SB_I_PSEUDO)
> +		return -EINVAL;

[Severity: Medium]
Can calling d_real_inode() on an O_PATH overlayfs file trigger a WARN(1)
in ovl_d_real() and bypass the SB_I_PSEUDO check?

When a file is opened with O_PATH, do_dentry_open() returns before calling
f->f_op->open(), so ovl_open() -> ovl_verify_lowerdata() has not run yet.
When file_check_backing() is called with FMODE_PATH, d_real_inode()
invokes ovl_d_real(dentry, D_REAL_DATA):

fs/overlayfs/super.c:ovl_d_real() {
    ...
	err = ovl_verify_lowerdata(dentry);
	if (err)
		goto bug;
	lower = ovl_dentry_lowerdata(dentry);
	if (!lower)
		goto bug;

real_lower:
	/* Handle recursion into stacked lower fs */
	return d_real(lower, type);

bug:
	WARN(1, "%s(%pd4, %d): real dentry not found\n", __func__, dentry, type);
	return dentry;
}

On an overlayfs mount using data-only lower layers or metacopy verity
validation, ovl_verify_lowerdata() takes the inode lock interruptibly via
ovl_inode_lock_interruptible() and performs lazy lowerdata lookup or
verity checks.  If that fails (for instance with -EINTR from a pending
signal, or -ENOENT / -EIO), ovl_d_real() fires WARN(1) and returns the
overlayfs dentry itself.  Because the overlayfs superblock does not set
SB_I_PSEUDO, the check here passes even if the underlying data layer is a
pseudo filesystem.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-work-loop-pseudo-v2-0-98639a2286fb@kernel.org?part=33