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