On Tue, Dec 29, 2020 at 04:25:47AM -0800, L.A. Walsh wrote:
> xfs_io checks for CAP_SYS_ADMIN in order to open a
> file_by_inode -- however, if the file one is opening
> is owned by the user performing the call, the call should
> not fail.
>
> (i.e. it opens the user's own file).
>
> patch against 5.10.2 is attached.
>
> It gets rid of some unnecessary error messages if you
> run xfs_restore to restore one of your own files.
>
The current logic seems to go a ways back. Can you or somebody elaborate
on whether there's any risks with loosening the permissions as such?
E.g., any reason we might not want to allow regular users to perform
handle lookups, etc.? If not, should some of the other _by_handle() ops
get similar treatment?
> --- fs/xfs/xfs_ioctl.c 2020-12-22 21:11:02.000000000 -0800
> +++ fs/xfs/xfs_ioctl.c 2020-12-29 04:14:48.681102804 -0800
> @@ -194,15 +194,21 @@
> struct dentry *dentry;
> fmode_t fmode;
> struct path path;
> + bool conditional_perm = 0;
Variable name alignment and I believe we try to use true/false for
boolean values.
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> + if (!capable(CAP_SYS_ADMIN)) conditional_perm=1;
This should remain two lines..
>
> dentry = xfs_handlereq_to_dentry(parfilp, hreq);
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
> inode = d_inode(dentry);
>
> + /* only allow user access to their own file */
> + if (conditional_perm && !inode_owner_or_capable(inode)) {
> + error = -EPERM;
> + goto out_dput;
> + }
> +
... but then again, is there any reason we couldn't just move the
capable() check down to this hunk and avoid the new variable?
Brian
> /* Restrict xfs_open_by_handle to directories & regular files. */
> if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> error = -EPERM;
