Re: [PATCH][v3] overlayfs: Do not lose security.capability xattr over metadata file copy-up

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Feb 04, 2019 at 10:05:26AM +0100, Miklos Szeredi wrote:
> On Wed, Jan 30, 2019 at 02:01:57PM -0500, Vivek Goyal wrote:
> > If a file has been copied up metadata only, and later data is copied up,
> > upper loses any security.capability xattr it has (underlying filesystem
> > clears it as upon file write).
> > 
> > From a user's point of view, this is just a file copy-up and that should
> > not result in losing security.capability xattr. Hence, before data copy
> > up, save security.capability xattr (if any) and restore it on upper after
> > data copy up is complete.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> 
> Cleaned it up a bit.  Hope I didn't break it...

Hi Miklos, 

I compiled it and xfstest I wrote to test it passes. Patch also looks good
to me.

Thanks
Vivek
> 
> Thanks,
> Miklos
> 
> ---
>  fs/overlayfs/copy_up.c   |   27 +++++++++++++++++++++--
>  fs/overlayfs/overlayfs.h |    2 +
>  fs/overlayfs/util.c      |   55 +++++++++++++++++++++++++++++------------------
>  3 files changed, 62 insertions(+), 22 deletions(-)
> 
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -863,28 +863,49 @@ bool ovl_is_metacopy_dentry(struct dentr
>  	return (oe->numlower > 1);
>  }
>  
> -char *ovl_get_redirect_xattr(struct dentry *dentry, int padding)
> +ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
> +		     size_t padding)
>  {
> -	int res;
> -	char *s, *next, *buf = NULL;
> +	ssize_t res;
> +	char *buf = NULL;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, NULL, 0);
> +	res = vfs_getxattr(dentry, name, NULL, 0);
>  	if (res < 0) {
>  		if (res == -ENODATA || res == -EOPNOTSUPP)
> -			return NULL;
> +			return -ENODATA;
>  		goto fail;
>  	}
>  
> -	buf = kzalloc(res + padding + 1, GFP_KERNEL);
> -	if (!buf)
> -		return ERR_PTR(-ENOMEM);
> +	if (res != 0) {
> +		buf = kzalloc(res + padding, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +
> +		res = vfs_getxattr(dentry, name, buf, res);
> +		if (res < 0)
> +			goto fail;
> +	}
> +	*value = buf;
>  
> -	if (res == 0)
> -		goto invalid;
> +	return res;
> +
> +fail:
> +	pr_warn_ratelimited("overlayfs: failed to get xattr %s: err=%zi)\n",
> +			    name, res);
> +	kfree(buf);
> +	return res;
> +}
> +
> +char *ovl_get_redirect_xattr(struct dentry *dentry, int padding)
> +{
> +	int res;
> +	char *s, *next, *buf = NULL;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, buf, res);
> +	res = ovl_getxattr(dentry, OVL_XATTR_REDIRECT, &buf, padding + 1);
> +	if (res == -ENODATA)
> +		return NULL;
>  	if (res < 0)
> -		goto fail;
> +		return ERR_PTR(res);
>  	if (res == 0)
>  		goto invalid;
>  
> @@ -900,15 +921,9 @@ char *ovl_get_redirect_xattr(struct dent
>  	}
>  
>  	return buf;
> -
> -err_free:
> -	kfree(buf);
> -	return ERR_PTR(res);
> -fail:
> -	pr_warn_ratelimited("overlayfs: failed to get redirect (%i)\n", res);
> -	goto err_free;
>  invalid:
>  	pr_warn_ratelimited("overlayfs: invalid redirect (%s)\n", buf);
>  	res = -EINVAL;
> -	goto err_free;
> +	kfree(buf);
> +	return ERR_PTR(res);
>  }
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -277,6 +277,8 @@ int ovl_lock_rename_workdir(struct dentr
>  int ovl_check_metacopy_xattr(struct dentry *dentry);
>  bool ovl_is_metacopy_dentry(struct dentry *dentry);
>  char *ovl_get_redirect_xattr(struct dentry *dentry, int padding);
> +ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
> +		     size_t padding);
>  
>  static inline bool ovl_is_impuredir(struct dentry *dentry)
>  {
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -742,6 +742,8 @@ static int ovl_copy_up_meta_inode_data(s
>  {
>  	struct path upperpath, datapath;
>  	int err;
> +	char *capability = NULL;
> +	ssize_t uninitialized_var(cap_size);
>  
>  	ovl_path_upper(c->dentry, &upperpath);
>  	if (WARN_ON(upperpath.dentry == NULL))
> @@ -751,15 +753,36 @@ static int ovl_copy_up_meta_inode_data(s
>  	if (WARN_ON(datapath.dentry == NULL))
>  		return -EIO;
>  
> +	if (c->stat.size) {
> +		err = cap_size = ovl_getxattr(upperpath.dentry, XATTR_NAME_CAPS,
> +					      &capability, 0);
> +		if (err < 0 && err != -ENODATA)
> +			goto out;
> +	}
> +
>  	err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
>  	if (err)
> -		return err;
> +		goto out_free;
> +
> +	/*
> +	 * Writing to upper file will clear security.capability xattr. We
> +	 * don't want that to happen for normal copy-up operation.
> +	 */
> +	if (capability) {
> +		err = ovl_do_setxattr(upperpath.dentry, XATTR_NAME_CAPS, capability, cap_size, 0);
> +		if (err)
> +			goto out_free;
> +	}
> +
>  
>  	err = vfs_removexattr(upperpath.dentry, OVL_XATTR_METACOPY);
>  	if (err)
> -		return err;
> +		goto out_free;
>  
>  	ovl_set_upperdata(d_inode(c->dentry));
> +out_free:
> +	kfree(capability);
> +out:
>  	return err;
>  }
>