[PATCH] btrfs: make root id query unprivileged

[PATCH] btrfs: make root id query unprivileged

[David Sterba]

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(-)

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;
 
-- 
1.8.4.5

Re: [PATCH] btrfs: make root id query unprivileged

[Goffredo Baroncelli]

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;
>  
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH] btrfs: make root id query unprivileged

[David Sterba]

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.