APPLIED[B]: [bionic:linux-aws][PATCH 1/1] UBUNTU: SAUCE: overlayfs: internal getxattr operations without sepolicy checking

Kelsey Skunberg kelsey.skunberg at canonical.com
Thu Jul 30 21:29:42 UTC 2020


Applied to Bionic/linux-aws. Thank you! 

-Kelsey

On 2020-07-09 15:14:41 , Marcelo Henrique Cerri wrote:
> From: Mark Salyzyn <salyzyn at android.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1864669
> 
> Check impure, opaque, origin & meta xattr with no sepolicy audit
> (using __vfs_getxattr) since these operations are internal to
> overlayfs operations and do not disclose any data.  This became
> an issue for credential override off since sys_admin would have
> been required by the caller; whereas would have been inherently
> present for the creator since it performed the mount.
> 
> This is a change in operations since we do not check in the new
> ovl_do_vfs_getxattr function if the credential override is off or
> not.  Reasoning is that the sepolicy check is unnecessary overhead,
> especially since the check can be expensive.
> 
> Because for override credentials off, this affects _everyone_ that
> underneath performs private xattr calls without the appropriate
> sepolicy permissions and sys_admin capability.  Providing blanket
> support for sys_admin would be bad for all possible callers.
> 
> For the override credentials on, this will affect only the mounter,
> should it lack sepolicy permissions. Not considered a security
> problem since mounting by definition has sys_admin capabilities,
> but sepolicy contexts would still need to be crafted.
> 
> It should be noted that there is precedence, __vfs_getxattr is used
> in other filesystems for their own internal trusted xattr management.
> 
> Signed-off-by: Mark Salyzyn <salyzyn at android.com>
> Cc: Miklos Szeredi <miklos at szeredi.hu>
> Cc: Jonathan Corbet <corbet at lwn.net>
> Cc: Vivek Goyal <vgoyal at redhat.com>
> Cc: Eric W. Biederman <ebiederm at xmission.com>
> Cc: Amir Goldstein <amir73il at gmail.com>
> Cc: Randy Dunlap <rdunlap at infradead.org>
> Cc: Stephen Smalley <sds at tycho.nsa.gov>
> Cc: linux-unionfs at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: kernel-team at android.com
> Cc: linux-security-module at vger.kernel.org
> 
> v15 - revert to v13 as xattr_gs_args was rejected.
>     - move ovl_do_wrapper from util.c to inline in overlayfs.h
> 
> v14 - rebase to use xattr_gs_args.
> 
> v13 - rebase to use __vfs_getxattr flags option
> 
> v12 - rebase
> 
> v11 - switch name to ovl_do_vfs_getxattr, fortify comment
> 
> v10 - added to patch series
> 
> [marcelo.cerri at canonical.com: Ignored missing context, adjusted
>  arguments for __vfs_getxattr and ovl_do_vfs_getxattr]
> [Based on v15: https://lore.kernel.org/patchwork/patch/1148514/]
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
> Acked-by: Kleber Souza <kleber.souza at canonical.com>
> Signed-off-by: Khalid Elmously <khalid.elmously at canonical.com>
> ---
>  fs/overlayfs/namei.c     | 19 ++++++++++---------
>  fs/overlayfs/overlayfs.h |  7 +++++++
>  fs/overlayfs/util.c      |  8 ++++----
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index ae683af0f5e3..53cb05ed2625 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -28,10 +28,10 @@ struct ovl_lookup_data {
>  static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>  			      size_t prelen, const char *post)
>  {
> -	int res;
> +	ssize_t res;
>  	char *s, *next, *buf = NULL;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, NULL, 0);
> +	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_REDIRECT, NULL, 0);
>  	if (res < 0) {
>  		if (res == -ENODATA || res == -EOPNOTSUPP)
>  			return 0;
> @@ -44,7 +44,7 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>  	if (res == 0)
>  		goto invalid;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, buf, res);
> +	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_REDIRECT, buf, res);
>  	if (res < 0)
>  		goto fail;
>  	if (res == 0)
> @@ -84,7 +84,7 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>  	kfree(buf);
>  	return 0;
>  fail:
> -	pr_warn_ratelimited("overlayfs: failed to get redirect (%i)\n", res);
> +	pr_warn_ratelimited("overlayfs: failed to get redirect (%zi)\n", res);
>  	goto err_free;
>  invalid:
>  	pr_warn_ratelimited("overlayfs: invalid redirect (%s)\n", buf);
> @@ -98,10 +98,10 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry)
>  
>  static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  {
> -	int res;
> +	ssize_t res;
>  	struct ovl_fh *fh = NULL;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
> +	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
>  	if (res < 0) {
>  		if (res == -ENODATA || res == -EOPNOTSUPP)
>  			return NULL;
> @@ -115,7 +115,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  	if (!fh)
>  		return ERR_PTR(-ENOMEM);
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, fh, res);
> +	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_ORIGIN, fh, res);
>  	if (res < 0)
>  		goto fail;
>  
> @@ -141,10 +141,11 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
>  	return NULL;
>  
>  fail:
> -	pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
> +	pr_warn_ratelimited("overlayfs: failed to get origin (%zi)\n", res);
>  	goto out;
>  invalid:
> -	pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n", res, fh);
> +	pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n",
> +			    (int)res, fh);
>  	goto out;
>  }
>  
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 6fd160b3db8a..b3ae63f61605 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -198,6 +198,13 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
>  	return ret;
>  }
>  
> +static inline ssize_t ovl_do_vfs_getxattr(struct dentry *dentry,
> +					  const char *name, void *buf,
> +					  size_t size)
> +{
> +	return __vfs_getxattr(dentry, d_inode(dentry), name, buf, size);
> +}
> +
>  /* util.c */
>  int ovl_want_write(struct dentry *dentry);
>  void ovl_drop_write(struct dentry *dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 4b0d52cd7cf4..57d8ee84e657 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -348,9 +348,9 @@ void ovl_copy_up_end(struct dentry *dentry)
>  
>  bool ovl_check_origin_xattr(struct dentry *dentry)
>  {
> -	int res;
> +	ssize_t res;
>  
> -	res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
> +	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
>  
>  	/* Zero size value means "copied up but origin unknown" */
>  	if (res >= 0)
> @@ -361,13 +361,13 @@ bool ovl_check_origin_xattr(struct dentry *dentry)
>  
>  bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
>  {
> -	int res;
> +	ssize_t res;
>  	char val;
>  
>  	if (!d_is_dir(dentry))
>  		return false;
>  
> -	res = vfs_getxattr(dentry, name, &val, 1);
> +	res = ovl_do_vfs_getxattr(dentry, name, &val, 1);
>  	if (res == 1 && val == 'y')
>  		return true;
>  
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team



More information about the kernel-team mailing list