diff mbox series

[v14,01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

Message ID 20240405214040.101396-2-gnoack@google.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Landlock: IOCTL support | expand

Commit Message

Günther Noack April 5, 2024, 9:40 p.m. UTC 
These IOCTL commands should be implemented by setting attributes on the
superblock, rather than in the IOCTL hooks in struct file_operations.

By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
logic to return -ENOTTY immediately, rather than attempting to call
f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.

Why this is safe:

Before this change, fs/ioctl.c would unsuccessfully attempt calling the
IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
return the same error code immediately, but save ourselves the fallback
attempt.

Motivation:

This simplifies the logic for these IOCTL commands and lets us reason about
the side effects of these IOCTLs more easily.  It will be possible to
permit these IOCTLs under LSM IOCTL policies, without having to worry about
them getting dispatched to problematic device drivers (which sometimes do
work before looking at the IOCTL command number).

Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 fs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kent Overstreet April 5, 2024, 9:54 p.m. UTC | #1 
On Fri, Apr 05, 2024 at 09:40:29PM +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> Why this is safe:
> 
> Before this change, fs/ioctl.c would unsuccessfully attempt calling the
> IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
> return the same error code immediately, but save ourselves the fallback
> attempt.
> 
> Motivation:
> 
> This simplifies the logic for these IOCTL commands and lets us reason about
> the side effects of these IOCTLs more easily.  It will be possible to
> permit these IOCTLs under LSM IOCTL policies, without having to worry about
> them getting dispatched to problematic device drivers (which sometimes do
> work before looking at the IOCTL command number).
> 
> Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Günther Noack <gnoack@google.com>

Acked-by: Kent Overstreet <kent.overstreet@linux.dev>

> ---
>  fs/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d5abfdf0f22..fb0628e680c4 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -769,7 +769,7 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  	struct fsuuid2 u = { .len = sb->s_uuid_len, };
>  
>  	if (!sb->s_uuid_len)
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
>  
> @@ -781,7 +781,7 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
>  	struct super_block *sb = file_inode(file)->i_sb;
>  
>  	if (!strlen(sb->s_sysfs_name))
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	struct fs_sysfs_path u = {};
>  
> -- 
> 2.44.0.478.gd926399ef9-goog
>
Christian Brauner April 9, 2024, 10:08 a.m. UTC | #2 
On Fri, 05 Apr 2024 21:40:29 +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> [...]

Taking this as a bugfix for this cycle.

---

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

[01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail
        https://git.kernel.org/vfs/vfs/c/abe6acfa7d7b
Mickaël Salaün April 9, 2024, 12:11 p.m. UTC | #3 
On Tue, Apr 09, 2024 at 12:08:23PM +0200, Christian Brauner wrote:
> On Fri, 05 Apr 2024 21:40:29 +0000, Günther Noack wrote:
> > These IOCTL commands should be implemented by setting attributes on the
> > superblock, rather than in the IOCTL hooks in struct file_operations.
> > 
> > By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> > logic to return -ENOTTY immediately, rather than attempting to call
> > f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> > 
> > [...]
> 
> Taking this as a bugfix for this cycle.

Looks good.

FYI, I added the following tags and re-formated the commit message in my
next branch, so it is already in linux-next (but I'll remove it when
yours will be merged):
https://git.kernel.org/mic/c/5b5c05340e67d1127a3839d1ccb7d7acbb7b9a82

Fixes: 41bcbe59c3b3 ("fs: FS_IOC_GETUUID")
Fixes: ae8c51175730 ("fs: add FS_IOC_GETFSSYSFSPATH")

You can also add:
Acked-by: Mickaël Salaün <mic@digikod.net>

> 
> ---
> 
> 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
> 
> [01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail
>         https://git.kernel.org/vfs/vfs/c/abe6acfa7d7b
>
Mickaël Salaün April 12, 2024, 3:17 p.m. UTC | #4 
Could we have a test that failed if this patch is not applied?  I'm not
sure this is possible because it would require a device file to handle
FS_IOC_GETUUID, which wouldn't be worth it implementing for this test.


On Fri, Apr 05, 2024 at 09:40:29PM +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> Why this is safe:
> 
> Before this change, fs/ioctl.c would unsuccessfully attempt calling the
> IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
> return the same error code immediately, but save ourselves the fallback
> attempt.
> 
> Motivation:
> 
> This simplifies the logic for these IOCTL commands and lets us reason about
> the side effects of these IOCTLs more easily.  It will be possible to
> permit these IOCTLs under LSM IOCTL policies, without having to worry about
> them getting dispatched to problematic device drivers (which sometimes do
> work before looking at the IOCTL command number).
> 
> Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  fs/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d5abfdf0f22..fb0628e680c4 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -769,7 +769,7 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  	struct fsuuid2 u = { .len = sb->s_uuid_len, };
>  
>  	if (!sb->s_uuid_len)
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
>  
> @@ -781,7 +781,7 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
>  	struct super_block *sb = file_inode(file)->i_sb;
>  
>  	if (!strlen(sb->s_sysfs_name))
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	struct fs_sysfs_path u = {};
>  
> -- 
> 2.44.0.478.gd926399ef9-goog
> 
>
diff mbox series

Patch

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d5abfdf0f22..fb0628e680c4 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -769,7 +769,7 @@  static int ioctl_getfsuuid(struct file *file, void __user *argp)
 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
 
 	if (!sb->s_uuid_len)
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
 
@@ -781,7 +781,7 @@  static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
 	struct super_block *sb = file_inode(file)->i_sb;
 
 	if (!strlen(sb->s_sysfs_name))
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	struct fs_sysfs_path u = {};