ACK/cmnt: [SRU][UNSTABLE/EOAN/FOCAL][PATCH] UBUNTU: SAUCE: shiftfs: let userns root destroy subvolumes from other users

[Kleber Souza]

On 2020-05-20 13:44, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1879688
> 
> Stéphane reported a bug found during NorthSec that makes heavy use of
> shiftfs. When a subvolume or snapshot is created as userns root in the
> container and then chowned to another user a delete as the root user
> will fail. The reason for this is that we drop all capabilities as a
> safety measure before calling btrfs ioctls. The only workable fix I
> could think of is to retain the CAP_DAC_OVERRIDE capability for the
> BTRFS_IOC_SNAP_DESTROY ioctl. All other solutions would be way more
> invasive.
> 
> Cc: Seth Forshee <seth.forshee at canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>

The change looks good to me, but I'd also like to see Seth's comment
on it.

Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>


Thanks,
Kleber

> ---
>  fs/shiftfs.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 652fb67b0e0b..2636871f05f4 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -1343,7 +1343,7 @@ static int shiftfs_fadvise(struct file *file, loff_t offset, loff_t len,
>  	return ret;
>  }
>  
> -static int shiftfs_override_ioctl_creds(const struct super_block *sb,
> +static int shiftfs_override_ioctl_creds(int cmd, const struct super_block *sb,
>  					const struct cred **oldcred,
>  					struct cred **newcred)
>  {
> @@ -1368,6 +1368,16 @@ static int shiftfs_override_ioctl_creds(const struct super_block *sb,
>  	cap_clear((*newcred)->cap_inheritable);
>  	cap_clear((*newcred)->cap_permitted);
>  
> +	if (cmd == BTRFS_IOC_SNAP_DESTROY) {
> +		kuid_t kuid_root = make_kuid(sb->s_user_ns, 0);
> +		/*
> +		 * Allow the root user in the container to remove subvolumes
> +		 * from other users.
> +		 */
> +		if (uid_valid(kuid_root) && uid_eq(fsuid, kuid_root))
> +			cap_raise((*newcred)->cap_effective, CAP_DAC_OVERRIDE);
> +	}
> +
>  	put_cred(override_creds(*newcred));
>  	return 0;
>  }
> @@ -1500,7 +1510,7 @@ static long shiftfs_real_ioctl(struct file *file, unsigned int cmd,
>  	if (ret)
>  		goto out_restore;
>  
> -	ret = shiftfs_override_ioctl_creds(sb, &oldcred, &newcred);
> +	ret = shiftfs_override_ioctl_creds(cmd, sb, &oldcred, &newcred);
>  	if (ret)
>  		goto out_fdput;
>  
>