| Message ID | 20200709181442.1299717-3-marcelo.cerri@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [eoan:linux-aws,focal:linux-aws,1/1] UBUNTU: SAUCE: overlayfs: internal getxattr operations without sepolicy checking | expand |
Applied to Focal/aws. thank you! -Kelsey On 2020-07-09 15:14:42 , Marcelo Henrique Cerri wrote: > From: Mark Salyzyn <salyzyn@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@android.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: Eric W. Biederman <ebiederm@xmission.com> > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Stephen Smalley <sds@tycho.nsa.gov> > Cc: linux-unionfs@vger.kernel.org > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: kernel-team@android.com > Cc: linux-security-module@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@canonical.com: Adjusted __vfs_getxattr() args and > removed XATTR_NOSECURITY] > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > Signed-off-by: Seth Forshee <seth.forshee@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@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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; }