| Message ID | 20200709181442.1299717-2-marcelo.cerri@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | LP:#1864669 - overlayfs regression - internal getxattr operations without sepolicy checking | expand |
Applied to Bionic/linux-aws. Thank you! -Kelsey On 2020-07-09 15:14:41 , 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 > > [marcelo.cerri@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@canonical.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > Acked-by: Kleber Souza <kleber.souza@canonical.com> > Signed-off-by: Khalid Elmously <khalid.elmously@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@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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;