On Mon, May 7, 2018 at 7:40 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> This patch modifies ovl_lookup() and friends to lookup metacopy dentries.
> It also allows for presence of metacopy dentries in lower layer.
>
> During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
> set OVL_UPPERDATA bit in flags.
>
> We don't support metacopy feature with nfs_export. So in nfs_export code,
> we set OVL_UPPERDATA flag set unconditionally if upper inode exists.
>
> Do not follow metacopy origin if we find a metacopy only inode and metacopy
> feature is not enabled for that mount. Like redirect, this can have security
> implications where an attacker could hand craft upper and try to gain
> access to file on lower which it should not have to begin with.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/export.c | 3 ++
> fs/overlayfs/inode.c | 11 ++++-
> fs/overlayfs/namei.c | 108 +++++++++++++++++++++++++++++++++++++++++------
> fs/overlayfs/overlayfs.h | 1 +
> fs/overlayfs/util.c | 22 ++++++++++
> 5 files changed, 130 insertions(+), 15 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 0549286cc55e..52a09a9f74b7 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -314,6 +314,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
> return ERR_CAST(inode);
> }
>
> + if (upper)
> + ovl_set_flag(OVL_UPPERDATA, inode);
> +
> dentry = d_find_any_alias(inode);
> if (!dentry) {
> dentry = d_alloc_anon(inode->i_sb);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index c128d5d54d0f..83b276ce0240 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -770,7 +770,7 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
> bool bylower = ovl_hash_bylower(oip->sb, upperdentry, lowerdentry,
> oip->index);
> int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
> - bool is_dir;
> + bool is_dir, metacopy = false;
> unsigned long ino = 0;
> int err = -ENOMEM;
>
> @@ -830,6 +830,15 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
> if (oip->index)
> ovl_set_flag(OVL_INDEX, inode);
>
> + if (upperdentry) {
> + err = ovl_check_metacopy_xattr(upperdentry);
> + if (err < 0)
> + goto out_err;
> + metacopy = err;
> + if (!metacopy)
> + ovl_set_flag(OVL_UPPERDATA, inode);
> + }
> +
> OVL_I(inode)->redirect = oip->redirect;
>
> /* Check for non-merge dir that may have whiteouts */
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 8fd817bf5529..b2ff08985e29 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -24,6 +24,7 @@ struct ovl_lookup_data {
> bool stop;
> bool last;
> char *redirect;
> + bool metacopy;
> };
>
> static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
> @@ -253,19 +254,29 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> goto put_and_out;
> }
> if (!d_can_lookup(this)) {
> - d->stop = true;
> - if (d->is_dir)
> + if (d->is_dir) {
> + d->stop = true;
> goto put_and_out;
> -
> + }
> /*
> * NB: handle failure to lookup non-last element when non-dir
> * redirects become possible
> */
> WARN_ON(!last_element);
This warning now triggers if we have a metacopy inode on upper
(d->is_dir == false) but lookup on lower layer fails in the middle
because a path element is not a directory (contrary to the comment,
this is not related to redirects, but to non-dir "merge" objects,
which we call metacopy).
I think we should handle this case together with the d->is_dir case
above: stop and get out. Lookup should handle the case where we
failed to find the data dentry. BTW, I think EIO is more appropriate
for this than ESTALE. The former means (among other things)
filesystem image is corrupted. The latter means some inconsistencies
were found when performing an operation. They are similar, but while
EIO is permanent (until the source of the error is corrected) ESTALE
can go away (e.g. if redoing the lookup or mounting the filesystem
fresh).
> + err = ovl_check_metacopy_xattr(this);
> + if (err < 0)
> + goto out_err;
> + d->stop = !err;
> + d->metacopy = !!err;
> goto out;
> }
> - if (last_element)
> + if (last_element) {
> + if (d->metacopy) {
> + err = -ESTALE;
Again, just stop and get out and caller will detect the error.
> + goto out_err;
> + }
> d->is_dir = true;
> + }
> if (d->last)
> goto out;
>
> @@ -823,7 +834,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> struct ovl_entry *poe = dentry->d_parent->d_fsdata;
> struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> - struct ovl_path *stack = NULL;
> + struct ovl_path *stack = NULL, *origin_path = NULL;
> struct dentry *upperdir, *upperdentry = NULL;
> struct dentry *origin = NULL;
> struct dentry *index = NULL;
> @@ -834,6 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> struct dentry *this;
> unsigned int i;
> int err;
> + bool metacopy = false;
> struct ovl_lookup_data d = {
> .name = dentry->d_name,
> .is_dir = false,
> @@ -841,6 +853,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> .stop = false,
> .last = ofs->config.redirect_follow ? false : !poe->numlower,
> .redirect = NULL,
> + .metacopy = false,
> };
>
> if (dentry->d_name.len > ofs->namelen)
> @@ -859,7 +872,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out;
> }
> if (upperdentry && !d.is_dir) {
> - BUG_ON(!d.stop || d.redirect);
> + unsigned int origin_ctr = 0;
> + BUG_ON(d.redirect);
> /*
> * Lookup copy up origin by decoding origin file handle.
> * We may get a disconnected dentry, which is fine,
> @@ -870,9 +884,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> * number - it's the same as if we held a reference
> * to a dentry in lower layer that was moved under us.
> */
> - err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
> + err = ovl_check_origin(ofs, upperdentry, &origin_path,
> + &origin_ctr);
> if (err)
> goto out_put_upper;
> +
> + if (d.metacopy)
> + metacopy = true;
> }
>
> if (d.redirect) {
> @@ -913,7 +931,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> * If no origin fh is stored in upper of a merge dir, store fh
> * of lower dir and set upper parent "impure".
> */
> - if (upperdentry && !ctr && !ofs->noxattr) {
> + if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
> err = ovl_fix_origin(dentry, this, upperdentry);
> if (err) {
> dput(this);
> @@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> * When "verify_lower" feature is enabled, do not merge with a
> * lower dir that does not match a stored origin xattr. In any
> * case, only verified origin is used for index lookup.
> + *
> + * For non-dir dentry, make sure dentry found by lookup
> + * matches the origin stored in upper. Otherwise its an
> + * error.
Umm, why we need to be so strict? This would break the case where
the layers are copied with xattr intact, but the origin pointer will
obviously be "wrong", which shouldn't be a problem, since that's only
needed to get a unique st_ino, nothing else.
> */
> - if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
> + if (upperdentry && !ctr &&
> + ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
> + (!d.is_dir && origin_path))) {
> err = ovl_verify_origin(upperdentry, this, false);
> if (err) {
> dput(this);
> - break;
> + if (d.is_dir)
> + break;
> + goto out_put;
> }
> -
> - /* Bless lower dir as verified origin */
> + /* Bless lower as verified origin */
> origin = this;
> }
>
> + if (d.metacopy)
> + metacopy = true;
> + /*
> + * Do not store intermediate metacopy dentries in chain,
> + * except top most lower metacopy dentry
I don't get it. We need the bottom most metacopy dentry, not the
topmost. Am I missing something?
We also need to check file type here, only regular file makes sense as
metacopy, so if it's something else, then get out with EIO.
> + */
> + if (d.metacopy && ctr) {
> + dput(this);
> + continue;
> + }
> +
> stack[ctr].dentry = this;
> stack[ctr].layer = lower.layer;
> ctr++;
> @@ -968,13 +1004,49 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> }
> }
>
> + if (metacopy) {
> + /*
> + * Found a metacopy dentry but did not find corresponding
> + * data dentry
> + */
> + if (d.metacopy) {
> + err = -ESTALE;
-EIO.
> + goto out_put;
> + }
> +
> + err = -EPERM;
> + if (!ofs->config.metacopy) {
> + pr_warn_ratelimited("overlay: refusing to follow"
> + " metacopy origin for (%pd2)\n",
> + dentry);
> + goto out_put;
> + }
> + } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> + if (WARN_ON(stack != NULL)) {
> + err = -EIO;
> + goto out_put;
> + }
> + stack = origin_path;
> + ctr = 1;
> + origin_path = NULL;
> + }
> +
> /*
> * Lookup index by lower inode and verify it matches upper inode.
> * We only trust dir index if we verified that lower dir matches
> * origin, otherwise dir index entries may be inconsistent and we
> - * ignore them. Always lookup index of non-dir and non-upper.
> + * ignore them.
> + *
> + * For non-dir upper metacopy dentry, we already set "origin" if we
> + * verified that lower matched upper origin. If upper origin was
> + * not present (because lower layer did not support fh encode/decode),
> + * do not set "origin" and skip looking up index. This case should
> + * be handled in same way as a non-dir upper without ORIGIN is
> + * handled.
> + *
> + * Always lookup index of non-dir non-metacopy and non-upper.
> */
> - if (ctr && (!upperdentry || !d.is_dir))
> + if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> origin = stack[0].dentry;
>
> if (origin && ovl_indexdir(dentry->d_sb) &&
> @@ -1015,6 +1087,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> }
>
> revert_creds(old_cred);
> + if (origin_path) {
> + dput(origin_path->dentry);
> + kfree(origin_path);
> + }
> dput(index);
> kfree(stack);
> kfree(d.redirect);
> @@ -1029,6 +1105,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> dput(stack[i].dentry);
> kfree(stack);
> out_put_upper:
> + if (origin_path) {
> + dput(origin_path->dentry);
> + kfree(origin_path);
> + }
> dput(upperdentry);
> kfree(upperredirect);
> out:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 2daea529b7eb..e8954fff1c45 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -274,6 +274,7 @@ bool ovl_need_index(struct dentry *dentry);
> int ovl_nlink_start(struct dentry *dentry, bool *locked);
> void ovl_nlink_end(struct dentry *dentry, bool locked);
> int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
> +int ovl_check_metacopy_xattr(struct dentry *dentry);
>
> static inline bool ovl_is_impuredir(struct dentry *dentry)
> {
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f8e3c95711b8..ab9a8fae0f99 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -778,3 +778,25 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
> pr_err("overlayfs: failed to lock workdir+upperdir\n");
> return -EIO;
> }
> +
> +/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> +int ovl_check_metacopy_xattr(struct dentry *dentry)
> +{
> + int 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);
> + if (res < 0) {
> + if (res == -ENODATA || res == -EOPNOTSUPP)
> + return 0;
> + goto out;
> + }
> +
> + return 1;
> +out:
> + pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> + return res;
> +}
> --
> 2.13.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
