APPLIED[F]: [eoan:linux-aws, focal:linux-aws][PATCH 1/1] UBUNTU: SAUCE: overlayfs: internal getxattr operations without sepolicy checking
Kelsey Skunberg
kelsey.skunberg at canonical.com
Tue Aug 4 00:54:14 UTC 2020
Applied to Focal/aws. thank you!
-Kelsey
On 2020-07-09 15:14:42 , 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
>
> [Based on v15: https://lore.kernel.org/patchwork/patch/1148514/]
> [marcelo.cerri at canonical.com: Adjusted __vfs_getxattr() args and
> removed XATTR_NOSECURITY]
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
> Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> Signed-off-by: Seth Forshee <seth.forshee at canonical.com>
> ---
> fs/overlayfs/namei.c | 12 +++++++-----
> fs/overlayfs/overlayfs.h | 7 +++++++
> fs/overlayfs/util.c | 18 +++++++++---------
> 3 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index f47c591402d7..126a93d6a5ab 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -106,10 +106,11 @@ int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
>
> static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> {
> - int res, err;
> + ssize_t res;
> + int err;
> struct ovl_fh *fh = NULL;
>
> - res = vfs_getxattr(dentry, name, NULL, 0);
> + res = ovl_do_vfs_getxattr(dentry, name, NULL, 0);
> if (res < 0) {
> if (res == -ENODATA || res == -EOPNOTSUPP)
> return NULL;
> @@ -123,7 +124,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> if (!fh)
> return ERR_PTR(-ENOMEM);
>
> - res = vfs_getxattr(dentry, name, fh, res);
> + res = ovl_do_vfs_getxattr(dentry, name, fh, res);
> if (res < 0)
> goto fail;
>
> @@ -141,10 +142,11 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> 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 ca10974b9f44..94f840eb7d7d 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -212,6 +212,13 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
> return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
> }
>
> +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 e78d873acc3e..40b12d153c52 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -550,9 +550,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)
> @@ -563,13 +563,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;
>
> @@ -850,13 +850,13 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
> /* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> int ovl_check_metacopy_xattr(struct dentry *dentry)
> {
> - int res;
> + ssize_t res;
>
> /* Only regular files can have metacopy xattr */
> if (!S_ISREG(d_inode(dentry)->i_mode))
> return 0;
>
> - res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> + res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> if (res < 0) {
> if (res == -ENODATA || res == -EOPNOTSUPP)
> return 0;
> @@ -865,7 +865,7 @@ int ovl_check_metacopy_xattr(struct dentry *dentry)
>
> return 1;
> out:
> - pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> + pr_warn_ratelimited("overlayfs: failed to get metacopy (%zi)\n", res);
> return res;
> }
>
> @@ -891,7 +891,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
> ssize_t res;
> char *buf = NULL;
>
> - res = vfs_getxattr(dentry, name, NULL, 0);
> + res = ovl_do_vfs_getxattr(dentry, name, NULL, 0);
> if (res < 0) {
> if (res == -ENODATA || res == -EOPNOTSUPP)
> return -ENODATA;
> @@ -903,7 +903,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
> if (!buf)
> return -ENOMEM;
>
> - res = vfs_getxattr(dentry, name, buf, res);
> + res = ovl_do_vfs_getxattr(dentry, name, buf, res);
> if (res < 0)
> goto fail;
> }
> --
> 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