[Devel] [PATCH RH7] overlayfs: avoid permission check for priveleged processes
Alexander Mikhalitsyn
alexander.mikhalitsyn at virtuozzo.com
Thu Oct 15 16:07:50 MSK 2020
On Thu, 15 Oct 2020 16:03:52 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn at virtuozzo.com> wrote:
> Please, see comments inline
>
> On Wed, 14 Oct 2020 02:05:21 +0300
> Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com> wrote:
>
> > Overlayfs temporary override credentials in copy_up function to ones which was
> > used to create mount. Unfortunately vfs_setxattr requires CAP_SYS_ADMIN
> > capability in current user namespace. This leads to strange situations.
> > For example, if overlayfs mount was made inside ve it is impossible to use
> > copy_up from init_user_ns even with CAP_SYS_ADMIN. This is because overriden
> > credentials are not sufficient in init_user_ns to set xattr to file.
> > This is also required for criu since copy_up can be triggered on dump stage:
> > reading inotify fhandle from /proc may start copy_up.
> >
> > Add an option to avoid vfs_setxattr CAP_SYS_ADMIN check if current credentials
> > have CAP_SYS_ADMIN in namespace that is recorded in overlayfs mount superblock.
> >
> > https://jira.sw.ru/browse/PSBM-108122
> > Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
> > ---
> > fs/overlayfs/copy_up.c | 25 +++++++++++++++++++------
> > fs/overlayfs/overlayfs.h | 39 ++++++++++++++++++++++++++-------------
> > fs/overlayfs/util.c | 32 ++++++++++++++++++++++++++++----
> > fs/xattr.c | 2 +-
> > 4 files changed, 74 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 1564a35..d6b285f 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -20,6 +20,7 @@
> > #include <linux/fdtable.h>
> > #include <linux/ratelimit.h>
> > #include <linux/exportfs.h>
> > +#include <linux/capability.h>
> > #include "overlayfs.h"
> >
> > #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
> > @@ -321,8 +322,8 @@ out:
> > return fh;
> > }
> >
> > -int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> > - struct dentry *upper)
> > +int ovl_set_origin_ext(struct dentry *dentry, struct dentry *lower,
> > + struct dentry *upper, int propagate_cap)
> > {
> > const struct ovl_fh *fh = NULL;
> > int err;
> > @@ -341,8 +342,8 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> > /*
> > * Do not fail when upper doesn't support xattrs.
> > */
> > - err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> > - fh ? fh->len : 0, 0);
> > + err = ovl_check_setxattr_ext(dentry, upper, OVL_XATTR_ORIGIN, fh,
> > + fh ? fh->len : 0, 0, propagate_cap);
> > kfree(fh);
> >
> > return err;
> > @@ -433,6 +434,7 @@ struct ovl_copy_up_ctx {
> > struct dentry *destdir;
> > struct qstr destname;
> > struct dentry *workdir;
> > + int propagate_cap;
> > bool tmpfile;
> > bool origin;
> > bool indexed;
> > @@ -711,7 +713,7 @@ out:
> > }
> >
> > static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > - int flags)
> > + int flags, int propagate_cap)
> > {
> > int err;
> > struct path parentpath;
> > @@ -719,6 +721,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > .parent = parent,
> > .dentry = dentry,
> > .workdir = ovl_workdir(dentry),
> > + .propagate_cap = propagate_cap,
>
> Maybe I've missed something but I can't see where this field is used.
>
> > };
> >
> > if (WARN_ON(!ctx.workdir))
> > @@ -768,9 +771,19 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > return err;
> > }
> >
> > +static int ovl_can_propagate_cap(struct dentry *dentry)
> > +{
> > + struct super_block *sb = dentry->d_sb;
> > + struct ovl_fs *ofs = sb->s_fs_info;
> > + struct user_namespace *ovl_ns = ofs->creator_cred->user_ns;
> > +
> > + return ns_capable(ovl_ns, CAP_SYS_ADMIN);
> > +}
> > +
> > int ovl_copy_up_flags(struct dentry *dentry, int flags)
> > {
> > int err = 0;
> > + int propagate_cap = ovl_can_propagate_cap(dentry);
> > const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
> > bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
> >
> > @@ -815,7 +828,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
> > next = parent;
> > }
> >
> > - err = ovl_copy_up_one(parent, next, flags);
> > + err = ovl_copy_up_one(parent, next, flags, propagate_cap);
> >
> > dput(parent);
> > dput(next);
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 7052938..6917acd 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -149,15 +149,6 @@ static inline int ovl_do_symlink(struct inode *dir, struct dentry *dentry,
> > return err;
> > }
> >
> > -static inline int ovl_do_setxattr(struct dentry *dentry, const char *name,
> > - const void *value, size_t size, int flags)
> > -{
> > - int err = vfs_setxattr(dentry, name, value, size, flags);
> > - pr_debug("setxattr(%pd2, \"%s\", \"%*pE\", %zu, 0x%x) = %i\n",
> > - dentry, name, min((int)size, 48), value, size, flags, err);
> > - return err;
> > -}
> > -
> > static inline int ovl_do_removexattr(struct dentry *dentry, const char *name)
> > {
> > int err = vfs_removexattr(dentry, name);
> > @@ -245,9 +236,12 @@ int ovl_copy_up_start(struct dentry *dentry);
> > void ovl_copy_up_end(struct dentry *dentry);
> > bool ovl_check_origin_xattr(struct dentry *dentry);
> > bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
> > -int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> > +int ovl_check_setxattr_ext(struct dentry *dentry, struct dentry *upperdentry,
> > const char *name, const void *value, size_t size,
> > - int xerr);
> > + int xerr, int propagate_cap);
> > +int ovl_do_setxattr_ext(struct dentry *dentry, const char *name,
> > + const void *value, size_t size, int flags,
> > + int propagate_cap);
> > int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
> > void ovl_set_flag(unsigned long flag, struct inode *inode);
> > void ovl_clear_flag(unsigned long flag, struct inode *inode);
> > @@ -279,6 +273,19 @@ static inline unsigned int ovl_xino_bits(struct super_block *sb)
> > return ofs->xino_bits;
> > }
> >
> > +static inline int ovl_check_setxattr(struct dentry *dentry,
> > + struct dentry *upperdentry, const char *name,
> > + const void *value, size_t size, int xerr)
> > +{
> > + return ovl_check_setxattr_ext(dentry, upperdentry, name, value, size,
> > + xerr, 0);
> > +}
> > +
> > +static inline int ovl_do_setxattr(struct dentry *dentry, const char *name,
> > + const void *value, size_t size, int flags)
> > +{
> > + return ovl_do_setxattr_ext(dentry, name, value, size, flags, 0);
> > +}
> >
> > /* namei.c */
> > int ovl_check_fh_len(struct ovl_fh *fh, int fh_len);
> > @@ -400,8 +407,14 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags);
> > int ovl_copy_xattr(struct dentry *old, struct dentry *new);
> > int ovl_set_attr(struct dentry *upper, struct kstat *stat);
> > struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
> > -int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> > - struct dentry *upper);
> > +int ovl_set_origin_ext(struct dentry *dentry, struct dentry *lower,
> > + struct dentry *upper, int propagate_cap);
> > +
> > +static inline int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> > + struct dentry *upper)
> > +{
> > + return ovl_set_origin_ext(dentry, lower, upper, 0);
> > +}
> >
> > /* export.c */
> > extern const struct export_operations ovl_export_operations;
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 52a5116..30c10f1 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -419,9 +419,32 @@ bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
> > return false;
> > }
> >
> > -int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> > - const char *name, const void *value, size_t size,
> > - int xerr)
> > +int ovl_do_setxattr_ext(struct dentry *dentry, const char *name,
> > + const void *value, size_t size, int flags, int propagate_cap)
> > +{
> > + int err = vfs_setxattr(dentry, name, value, size, flags);
> > +
> > + if (propagate_cap && err == -EPERM) {
> > + struct inode *ino = dentry->d_inode;
> > +
> > + if (IS_IMMUTABLE(ino) || IS_APPEND(ino))
> > + goto out;
> > +
> > + inode_lock(ino);
> > + err = __vfs_setxattr_noperm(dentry, name, value, size, flags);
>
> Speaking honestly, this is looks as very unsafe solution because
> you have exported function that was "private" and as I can see you
> have copied some checks from xattr_permission function (fs/xattr.c) also.
> What if design will change and we will need to have some extra checks
> here?
I can also add here, that as an option we may create special function in
fs/xattr.c something like vfs_ovl_setxattr_noperm() and keep this function close
to xattr_permission() and __vfs_setxattr_noperm() and during rebase we have minimal
probability of errors here if design will change.
>
> As I can see in xattr_permission function we have also:
> /*
> * Updating an xattr will likely cause i_uid and i_gid
> * to be writen back improperly if their true value is
> * unknown to the vfs.
> */
> if (HAS_UNMAPPED_ID(inode))
> return -EPERM;
>
> Do we need to have this check here too?
>
> > + inode_unlock(ino);
> > + }
> > +
> > +out:
> > + pr_debug("setxattr(%pd2, \"%s\", \"%*pE\", %zu, 0x%x) = %i, propagate_cap = %d\n",
> > + dentry, name, min((int)size, 48), value, size, flags, err,
> > + propagate_cap);
> > + return err;
> > +}
> > +
> > +int ovl_check_setxattr_ext(struct dentry *dentry, struct dentry *upperdentry,
> > + const char *name, const void *value, size_t size,
> > + int xerr, int propagate_cap)
> > {
> > int err;
> > struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > @@ -429,7 +452,8 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> > if (ofs->noxattr)
> > return xerr;
> >
> > - err = ovl_do_setxattr(upperdentry, name, value, size, 0);
> > + err = ovl_do_setxattr_ext(upperdentry, name, value, size, 0,
> > + propagate_cap);
> >
> > if (err == -EOPNOTSUPP) {
> > pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 9c24acc..c164e83 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -124,7 +124,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >
> > return error;
> > }
> > -
> > +EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
> >
> > int
> > vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Devel mailing list
> > Devel at openvz.org
> > https://lists.openvz.org/mailman/listinfo/devel
>
>
> --
> Regards,
> Alex.
--
Regards,
Alex.
More information about the Devel
mailing list