Re: [PATCH] f2fs: require inode ownership for ioctls that change per-file state

[Chao Yu <chao@kernel.org>]

On 11/8/26 12:57, Hungyu Lin wrote:
> F2FS_IOC_RELEASE_COMPRESS_BLOCKS, F2FS_IOC_RESERVE_COMPRESS_BLOCKS,
> F2FS_IOC_SET_PIN_FILE and F2FS_IOC_IO_PRIO change the state of the inode
> behind the file descriptor without checking who the caller is. Any user
> who can open a file, even read-only, can:

How about checking write permission instead of inode_owner_or_capable()?

	if (inode_permission(file_mnt_idmap(filp), inode, MAY_WRITE))
		return -EACCES;

> 
>  - release its compressed blocks, after which writes to the file fail
>    with -EPERM until the blocks are reserved again or the file is
>    truncated to zero;
>  - reserve released blocks again, which consumes free space;
>  - pin it while it has no data blocks, after which writes that extend
>    the file fail with -EIO;
>  - unpin it, so that garbage collection may move blocks that a user of
>    F2FS_IOC_SET_PIN_FILE expects to stay in place;
>  - set its write I/O priority hint.
> 
> F2FS_IOC_SET_PIN_FILE checked inode_owner_or_capable() when it was
> added. Commit aff7b628ac2d ("f2fs: set pin_file under CAP_SYS_ADMIN")
> replaced that with a CAP_SYS_ADMIN check, and commit b13bdf03bb80
> ("f2fs: allow all the users to pin a file") then removed the
> CAP_SYS_ADMIN check, leaving the ioctl without any permission check.
> 
> The atomic write ioctls require both FMODE_WRITE and
> inode_owner_or_capable(). Requiring FMODE_WRITE here would break
> f2fs_io, which releases and reserves compressed blocks through a

I think we can fix f2fs_io.

> read-only file descriptor, and Android, which releases them the same
> way. Require only inode_owner_or_capable(), as F2FS_IOC_SET_PIN_FILE
> originally did, so that only the owner or a caller with CAP_FOWNER can
> change this state. Users can still pin their own files.
> 
> Fixes: b13bdf03bb80 ("f2fs: allow all the users to pin a file")
> Fixes: ef8d563f184e ("f2fs: introduce F2FS_IOC_RELEASE_COMPRESS_BLOCKS")
> Fixes: c75488fb4d82 ("f2fs: introduce F2FS_IOC_RESERVE_COMPRESS_BLOCKS")
> Fixes: 5f95c1812a65 ("f2fs: add ioctl to get IO priority hint")

IMO, we'd better to fix them in separated patches.

Thanks,

> Cc: stable@vger.kernel.org
> Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
> ---
>  fs/f2fs/file.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index e2c0f5083313..0730de36554a 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3664,6 +3664,9 @@ static int f2fs_ioc_set_pin_file(struct file *filp, unsigned long arg)
>  	if (!pin && IS_DEVICE_ALIASING(inode))
>  		return -EOPNOTSUPP;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(filp), inode))
> +		return -EACCES;
> +
>  	ret = mnt_want_write_file(filp);
>  	if (ret)
>  		return ret;
> @@ -3982,6 +3985,9 @@ static int f2fs_ioc_io_prio(struct file *filp, unsigned long arg)
>  	if (!S_ISREG(inode->i_mode) || level >= F2FS_IOPRIO_MAX)
>  		return -EINVAL;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(filp), inode))
> +		return -EACCES;
> +
>  	inode_lock(inode);
>  	F2FS_I(inode)->ioprio_hint = level;
>  	inode_unlock(inode);
> @@ -4235,6 +4241,9 @@ static int f2fs_release_compress_blocks(struct file *filp, unsigned long arg)
>  	if (f2fs_readonly(sbi->sb))
>  		return -EROFS;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(filp), inode))
> +		return -EACCES;
> +
>  	ret = mnt_want_write_file(filp);
>  	if (ret)
>  		return ret;
> @@ -4430,6 +4439,9 @@ static int f2fs_reserve_compress_blocks(struct file *filp, unsigned long arg)
>  	if (f2fs_readonly(sbi->sb))
>  		return -EROFS;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(filp), inode))
> +		return -EACCES;
> +
>  	ret = mnt_want_write_file(filp);
>  	if (ret)
>  		return ret;