| Message ID | 20220914142632.2016571-1-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Remove user_ns check for FUSE_DEV_IOC_CLONE | expand |
On Wed, 14 Sept 2022 at 16:27, Jann Horn <jannh@google.com> wrote: > > Commit 8ed1f0e22f49e ("fs/fuse: fix ioctl type confusion") fixed a type > confusion bug by adding an ->f_op comparison. > > Based on some off-list discussion back then, another check was added to > compare the f_cred->user_ns. This is not for security reasons, but was > based on the idea that a FUSE device FD should be using the UID/GID > mappings of its f_cred->user_ns, and those translations are done using > fc->user_ns, which matches the f_cred->user_ns of the initial > FUSE device FD thanks to the check in fuse_fill_super(). > See also commit 8cb08329b0809 ("fuse: Support fuse filesystems outside of > init_user_ns"). > > But FUSE_DEV_IOC_CLONE is, at a higher level, a *cloning* operation that > copies an existing context (with a weird API that involves first opening > /dev/fuse, then tying the resulting new FUSE device FD to an existing FUSE > instance). So if an application is already passing FUSE FDs across > userns boundaries and dealing with the resulting ID mapping complications > somehow, it doesn't make much sense to block this cloning operation. > > I've heard that this check is an obstacle for some folks, and I don't see > a good reason to keep it, so remove it. > > Signed-off-by: Jann Horn <jannh@google.com> I see no issues with this. f_cred seems to be unused by the VFS, so this should have zero effect on anything other than rejecting or allowing FUSE_DEV_IOC_CLONE. Applied. Thanks, Miklos
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 51897427a5346..920480054e1dc 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -2266,8 +2266,7 @@ static long fuse_dev_ioctl(struct file *file, unsigned int cmd, * Check against file->f_op because CUSE * uses the same ioctl handler. */ - if (old->f_op == file->f_op && - old->f_cred->user_ns == file->f_cred->user_ns) + if (old->f_op == file->f_op) fud = fuse_get_dev(old); if (fud) {
Commit 8ed1f0e22f49e ("fs/fuse: fix ioctl type confusion") fixed a type confusion bug by adding an ->f_op comparison. Based on some off-list discussion back then, another check was added to compare the f_cred->user_ns. This is not for security reasons, but was based on the idea that a FUSE device FD should be using the UID/GID mappings of its f_cred->user_ns, and those translations are done using fc->user_ns, which matches the f_cred->user_ns of the initial FUSE device FD thanks to the check in fuse_fill_super(). See also commit 8cb08329b0809 ("fuse: Support fuse filesystems outside of init_user_ns"). But FUSE_DEV_IOC_CLONE is, at a higher level, a *cloning* operation that copies an existing context (with a weird API that involves first opening /dev/fuse, then tying the resulting new FUSE device FD to an existing FUSE instance). So if an application is already passing FUSE FDs across userns boundaries and dealing with the resulting ID mapping complications somehow, it doesn't make much sense to block this cloning operation. I've heard that this check is an obstacle for some folks, and I don't see a good reason to keep it, so remove it. Signed-off-by: Jann Horn <jannh@google.com> --- @Eric: Does this look reasonable to you? I dug through my old mails, and in the off-list discussion back then, Linus and you were in favor of adding this check. fs/fuse/dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) base-commit: 3245cb65fd91cd514801bf91f5a3066d562f0ac4