| Message ID | 1431450889-27968-1-git-send-email-dsterba@suse.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi David, On 2015-05-12 19:14, David Sterba wrote: > The INO_LOOKUP ioctl can lookup path for a given inode number and is > thus restricted. As a sideefect it can find the root id of the > containing subvolume and we're using this int the 'btrfs inspect rootid' > command. > > The restriction is unnecessary in case we set the ioctl args > args::treeid = 0 > args::objectid = 256 (BTRFS_FIRST_FREE_OBJECTID) > > Then the path will be empty and the treeid is filled with the root id of > the inode on which the ioctl is called. This behaviour is unchanged, > after the root restriction is removed. I think that make sense to add another ioctl instead of relaxing the privilege check on the basis of the parameters. If I read correctly the code, in this case the function btrfs_search_path_in_tree() is not even called: it is requested to return only BTRFS_I(inode)->root->root_key.objectid; Goffredo > > Signed-off-by: David Sterba <dsterba@suse.cz> > --- > > fs/btrfs/ioctl.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 1c22c6518504..578ff63a9b74 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2271,10 +2271,7 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, > { > struct btrfs_ioctl_ino_lookup_args *args; > struct inode *inode; > - int ret; > - > - if (!capable(CAP_SYS_ADMIN)) > - return -EPERM; > + int ret = 0; > > args = memdup_user(argp, sizeof(*args)); > if (IS_ERR(args)) > @@ -2282,13 +2279,28 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, > > inode = file_inode(file); > > + /* > + * Unprivileged query to obtain the containing subvolume root id. The > + * path is reset so it's consistent with btrfs_search_path_in_tree. > + */ > if (args->treeid == 0) > args->treeid = BTRFS_I(inode)->root->root_key.objectid; > > + if (args->objectid == BTRFS_FIRST_FREE_OBJECTID) { > + args->name[0] = 0; > + goto out; > + } > + > + if (!capable(CAP_SYS_ADMIN)) { > + ret = -EPERM; > + goto out; > + } > + > ret = btrfs_search_path_in_tree(BTRFS_I(inode)->root->fs_info, > args->treeid, args->objectid, > args->name); > > +out: > if (ret == 0 && copy_to_user(argp, args, sizeof(*args))) > ret = -EFAULT; > >
On Wed, May 13, 2015 at 06:41:54PM +0200, Goffredo Baroncelli wrote: > I think that make sense to add another ioctl instead of relaxing the privilege check > on the basis of the parameters. Yeah, that was another way how to do it. In general I'm more inclined to use the existing ioctls and related structures and avoid single purpose ioctl. Furthermore, we can teach btrfs_ioctl_ino_lookup to verify access rights to the file it finds. The root restriction is not unnecessary in all cases. We've relaxed the ioctl permissions in the past, see FS_INFO for example. I hope we can afford that if that's for sake of better usability and user convenience. > If I read correctly the code, in this case the function > btrfs_search_path_in_tree() is not even called: it is requested to return only > > BTRFS_I(inode)->root->root_key.objectid; Right. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 1c22c6518504..578ff63a9b74 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2271,10 +2271,7 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, { struct btrfs_ioctl_ino_lookup_args *args; struct inode *inode; - int ret; - - if (!capable(CAP_SYS_ADMIN)) - return -EPERM; + int ret = 0; args = memdup_user(argp, sizeof(*args)); if (IS_ERR(args)) @@ -2282,13 +2279,28 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, inode = file_inode(file); + /* + * Unprivileged query to obtain the containing subvolume root id. The + * path is reset so it's consistent with btrfs_search_path_in_tree. + */ if (args->treeid == 0) args->treeid = BTRFS_I(inode)->root->root_key.objectid; + if (args->objectid == BTRFS_FIRST_FREE_OBJECTID) { + args->name[0] = 0; + goto out; + } + + if (!capable(CAP_SYS_ADMIN)) { + ret = -EPERM; + goto out; + } + ret = btrfs_search_path_in_tree(BTRFS_I(inode)->root->fs_info, args->treeid, args->objectid, args->name); +out: if (ret == 0 && copy_to_user(argp, args, sizeof(*args))) ret = -EFAULT;
The INO_LOOKUP ioctl can lookup path for a given inode number and is thus restricted. As a sideefect it can find the root id of the containing subvolume and we're using this int the 'btrfs inspect rootid' command. The restriction is unnecessary in case we set the ioctl args args::treeid = 0 args::objectid = 256 (BTRFS_FIRST_FREE_OBJECTID) Then the path will be empty and the treeid is filled with the root id of the inode on which the ioctl is called. This behaviour is unchanged, after the root restriction is removed. Signed-off-by: David Sterba <dsterba@suse.cz> --- fs/btrfs/ioctl.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)