permission checking for F_SET_RW_HINT

permission checking for F_SET_RW_HINT

[Christoph Hellwig]

Hi all

I've been expanding test coverage for F_SET_RW_HINT, and noticed that
right now there is no permission checking whatsoever.  Patch 1 fixes that
by requiring the owner or capable to set the write hint.  Patch 2 also
requires an fd open for write, which is a bit more arguable but still
feels right.

Diffstat:
 fcntl.c |    5 +++++
 1 file changed, 5 insertions(+)

[PATCH 1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT

[Christoph Hellwig]

F_SET_RW_HINT controls data placement in the file system and / or
device and should not be available to everyone who can read a given file.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/fcntl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 22dd9dcce7ec..7fc6190da342 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -375,6 +375,9 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
 	u64 __user *argp = (u64 __user *)arg;
 	u64 hint;
 
+	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
+		return -EPERM;
+
 	if (copy_from_user(&hint, argp, sizeof(hint)))
 		return -EFAULT;
 	if (!rw_hint_valid(hint))
-- 
2.45.2

Re: [PATCH 1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT

[Jan Kara]

On Fri 22-11-24 13:29:24, Christoph Hellwig wrote:
> F_SET_RW_HINT controls data placement in the file system and / or
> device and should not be available to everyone who can read a given file.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Makes sense. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fcntl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..7fc6190da342 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -375,6 +375,9 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
>  	u64 __user *argp = (u64 __user *)arg;
>  	u64 hint;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
> +		return -EPERM;
> +
>  	if (copy_from_user(&hint, argp, sizeof(hint)))
>  		return -EFAULT;
>  	if (!rw_hint_valid(hint))
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT

[Christian Brauner]

On Fri, Nov 22, 2024 at 01:29:24PM +0100, Christoph Hellwig wrote:
> F_SET_RW_HINT controls data placement in the file system and / or
> device and should not be available to everyone who can read a given file.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/fcntl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..7fc6190da342 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -375,6 +375,9 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
>  	u64 __user *argp = (u64 __user *)arg;
>  	u64 hint;
>  
> +	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
> +		return -EPERM;
> +

Bah, nice catch.

Re: (subset) [PATCH 1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT

[Christian Brauner]

On Fri, 22 Nov 2024 13:29:24 +0100, Christoph Hellwig wrote:
> F_SET_RW_HINT controls data placement in the file system and / or
> device and should not be available to everyone who can read a given file.
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT
      https://git.kernel.org/vfs/vfs/c/b6512519496e

[PATCH 2/2] fs: require FMODE_WRITE for F_SET_RW_HINT

[Christoph Hellwig]

F_SET_RW_HINT controls the placement of written file data.  A read-only
fd should not allow for that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/fcntl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 7fc6190da342..12f60c42743a 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -377,6 +377,8 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
 
 	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
 		return -EPERM;
+	if (!(file->f_mode & FMODE_WRITE))
+		return -EBADF;
 
 	if (copy_from_user(&hint, argp, sizeof(hint)))
 		return -EFAULT;
-- 
2.45.2

Re: [PATCH 2/2] fs: require FMODE_WRITE for F_SET_RW_HINT

[Jan Kara]

On Fri 22-11-24 13:29:25, Christoph Hellwig wrote:
> F_SET_RW_HINT controls the placement of written file data.  A read-only
> fd should not allow for that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Here I'm not so sure. Firstly, since you are an owner this doesn't add any
additional practical restriction. Secondly, you are not changing anything
on disk, just IO hints in memory... Thirdly, we generally don't require
writeable fd even to do file attribute changes (like with fchmod, fchown,
etc.).  So although the check makes some sense, it seems to be mostly
inconsistent with how we treat similar stuff.

								Honza
> ---
>  fs/fcntl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 7fc6190da342..12f60c42743a 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -377,6 +377,8 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd,
>  
>  	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
>  		return -EPERM;
> +	if (!(file->f_mode & FMODE_WRITE))
> +		return -EBADF;
>  
>  	if (copy_from_user(&hint, argp, sizeof(hint)))
>  		return -EFAULT;
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] fs: require FMODE_WRITE for F_SET_RW_HINT

[Christoph Hellwig]

On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote:
> Here I'm not so sure. Firstly, since you are an owner this doesn't add any
> additional practical restriction. Secondly, you are not changing anything
> on disk, just IO hints in memory... Thirdly, we generally don't require
> writeable fd even to do file attribute changes (like with fchmod, fchown,
> etc.).  So although the check makes some sense, it seems to be mostly
> inconsistent with how we treat similar stuff.

As I said I'm not quite convince either, so just doing the first one
is probably fine.

Re: [PATCH 2/2] fs: require FMODE_WRITE for F_SET_RW_HINT

[Matthew Wilcox]

On Fri, Nov 22, 2024 at 05:15:47PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote:
> > Here I'm not so sure. Firstly, since you are an owner this doesn't add any
> > additional practical restriction. Secondly, you are not changing anything
> > on disk, just IO hints in memory... Thirdly, we generally don't require
> > writeable fd even to do file attribute changes (like with fchmod, fchown,
> > etc.).  So although the check makes some sense, it seems to be mostly
> > inconsistent with how we treat similar stuff.
> 
> As I said I'm not quite convince either, so just doing the first one
> is probably fine.

We do require FMODE_WRITE to do a dedupe, which isn't exactly the same
but is similar in concept (we're not changing the content of the file;
we're changing how it's laid out on storage).  So I think it's reasonable
to require FMODE_WRITE to set the write hints.

Re: [PATCH 2/2] fs: require FMODE_WRITE for F_SET_RW_HINT

[Christian Brauner]

On Fri, Nov 22, 2024 at 04:40:31PM +0000, Matthew Wilcox wrote:
> On Fri, Nov 22, 2024 at 05:15:47PM +0100, Christoph Hellwig wrote:
> > On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote:
> > > Here I'm not so sure. Firstly, since you are an owner this doesn't add any
> > > additional practical restriction. Secondly, you are not changing anything
> > > on disk, just IO hints in memory... Thirdly, we generally don't require
> > > writeable fd even to do file attribute changes (like with fchmod, fchown,
> > > etc.).  So although the check makes some sense, it seems to be mostly
> > > inconsistent with how we treat similar stuff.
> > 
> > As I said I'm not quite convince either, so just doing the first one
> > is probably fine.
> 
> We do require FMODE_WRITE to do a dedupe, which isn't exactly the same
> but is similar in concept (we're not changing the content of the file;
> we're changing how it's laid out on storage).  So I think it's reasonable
> to require FMODE_WRITE to set the write hints.

I tend to agree with Jan. I think the dedupe case is different because
you actually use the file to perform a write(-like) operation whereas
toggling write hints is just setting an option.