Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 Aug 2022 15:25:52 +0200
From:   Christian Brauner <brauner@kernel.org>
To:     Zhang Tianci <zhangtianci.1997@bytedance.com>
Cc:     miklos@szeredi.hu, linux-unionfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, amir73il@gmail.com,
        Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
Subject: Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link()
Message-ID: <20220825132552.loybzprnkwdkmu3k@wittgenstein>
References: <20220825130552.29587-1-zhangtianci.1997@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220825130552.29587-1-zhangtianci.1997@bytedance.com>
Precedence: bulk

On Thu, Aug 25, 2022 at 09:05:52PM +0800, Zhang Tianci wrote:
> There is a wrong case of link() on overlay:
>   $ mkdir /lower /fuse /merge
>   $ mount -t fuse /fuse
>   $ mkdir /fuse/upper /fuse/work
>   $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\
>     workdir=work
>   $ touch /merge/file
>   $ chown bin.bin /merge/file // the file's caller becomes "bin"
>   $ ln /merge/file /merge/lnkfile
> 
> Then we will get an error(EACCES) because fuse daemon checks the link()'s
> caller is "bin", it denied this request.
> 
> In the changing history of ovl_link(), there are two key commits:
> 
> The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which
> overrides the cred's fsuid/fsgid using the new inode. The new inode's
> owner is initialized by inode_init_owner(), and inode->fsuid is
> assigned to the current user. So the override fsuid becomes the
> current user. We know link() is actually modifying the directory, so
> the caller must have the MAY_WRITE permission on the directory. The
> current caller may should have this permission. This is acceptable
> to use the caller's fsuid.
> 
> The second is commit 51f7e52dc943 ("ovl: share inode for hard link")
> which removed the inode creation in ovl_link(). This commit move
> inode_init_owner() into ovl_create_object(), so the ovl_link() just
> give the old inode to ovl_create_or_link(). Then the override fsuid
> becomes the old inode's fsuid, neither the caller nor the overlay's
> creator! So this is incorrect.
> 
> Fix this bug by using current fsuid/fsgid to do underlying fs's link().
> 
> Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T
> 
> Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com>
> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---

(Should probably also use a 
Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>, I
think. But not that important.)

Looks good to me but this really should be tested to survive xfstests,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

>  fs/overlayfs/dir.c       | 16 +++++++++++-----
>  fs/overlayfs/overlayfs.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 6b03457f72bb..dd84e6fc5f6e 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>  	err = -ENOMEM;
>  	override_cred = prepare_creds();
>  	if (override_cred) {
> -		override_cred->fsuid = inode->i_uid;
> -		override_cred->fsgid = inode->i_gid;
> +		override_cred->fsuid = attr->fsuid;
> +		override_cred->fsgid = attr->fsgid;
>  		if (!attr->hardlink) {
>  			err = security_dentry_create_files_as(dentry,
>  					attr->mode, &dentry->d_name, old_cred,
> @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
>  	inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
>  	attr.mode = inode->i_mode;
>  
> +	attr.fsuid = inode->i_uid;
> +	attr.fsgid = inode->i_gid;
>  	err = ovl_create_or_link(dentry, inode, &attr, false);
>  	/* Did we end up using the preallocated inode? */
>  	if (inode != d_inode(dentry))
> @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
>  {
>  	int err;
>  	struct inode *inode;
> +	struct ovl_cattr attr;
>  
>  	err = ovl_want_write(old);
>  	if (err)
> @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
>  	inode = d_inode(old);
>  	ihold(inode);
>  
> -	err = ovl_create_or_link(new, inode,
> -			&(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)},
> -			ovl_type_origin(old));
> +	attr = (struct ovl_cattr) {
> +		.hardlink = ovl_dentry_upper(old),
> +		.fsuid = current_fsuid(),
> +		.fsgid = current_fsgid(),
> +	};
> +	err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old));
>  	if (err)
>  		iput(inode);
>  
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 87759165d32b..85043123a103 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -655,6 +655,8 @@ struct ovl_cattr {
>  	umode_t mode;
>  	const char *link;
>  	struct dentry *hardlink;
> +	kuid_t fsuid;
> +	kgid_t fsgid;
>  };
>  
>  #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
> -- 
> 2.32.1 (Apple Git-133)
>