On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote:
>
> When overlayfs finds a file with metacopy and/or redirect attributes and
> the metacopy and/or redirect features are not enabled, then it refuses to
> act on those attributes while also issuing a warning.
>
> There was a slight inconsistency of only warning on an upper metacopy if it
> found the next file on the lower layer, while always warning for metacopy
> found on a lower layer.
>
> Fix this inconsistency and make the logic more straightforward, pavig the
> way for following patches to change when dataredirects are allowed.
>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> ---
> fs/overlayfs/namei.c | 67 +++++++++++++++++++++++++++++---------------
> 1 file changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index be5c65d6f848..da322e9768d1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1040,6 +1040,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> struct inode *inode = NULL;
> bool upperopaque = false;
> char *upperredirect = NULL;
> + bool nextredirect = false;
> + bool nextmetacopy = false;
> struct dentry *this;
> unsigned int i;
> int err;
> @@ -1087,8 +1089,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (err)
> goto out_put_upper;
>
> - if (d.metacopy)
> + if (d.metacopy) {
> uppermetacopy = true;
> + nextmetacopy = true;
> + }
> metacopy_size = d.metacopy;
> }
>
> @@ -1099,6 +1103,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_put_upper;
> if (d.redirect[0] == '/')
> poe = roe;
> + nextredirect = true;
> }
> upperopaque = d.opaque;
> }
> @@ -1113,6 +1118,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
> struct ovl_path lower = ovl_lowerstack(poe)[i];
>
> + /*
> + * Following redirects/metacopy can have security consequences:
> + * it's like a symlink into the lower layer without the
> + * permission checks.
> + *
> + * This is only a problem if the upper layer is untrusted (e.g
> + * comes from an USB drive). This can allow a non-readable file
> + * or directory to become readable.
> + *
> + * Only following redirects when redirects are enabled disables
> + * this attack vector when not necessary.
> + */
What do you say about moving this comment outside the loop and leaving here
only:
/* Should redirects/metacopy to lower layers be followed? */
if ((nextmetacopy && !ofs->config.metacopy) ||
(nextredirect && !ovl_redirect_follow(ofs)))
break;
> + if (nextmetacopy && !ofs->config.metacopy) {
> + pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> + err = -EPERM;
> + goto out_put;
> + }
> + if (nextredirect && !ovl_redirect_follow(ofs)) {
> + pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> + err = -EPERM;
> + goto out_put;
> + }
> +
> if (!ovl_redirect_follow(ofs))
> d.last = i == ovl_numlower(poe) - 1;
> else if (d.is_dir || !ofs->numdatalayer)
> @@ -1126,12 +1154,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (!this)
> continue;
>
> - if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> - dput(this);
> - err = -EPERM;
> - pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> - goto out_put;
> - }
> + if (d.metacopy)
> + nextmetacopy = true;
>
> /*
> * If no origin fh is stored in upper of a merge dir, store fh
> @@ -1185,22 +1209,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> ctr++;
> }
>
> - /*
> - * Following redirects can have security consequences: it's like
> - * a symlink into the lower layer without the permission checks.
> - * This is only a problem if the upper layer is untrusted (e.g
> - * comes from an USB drive). This can allow a non-readable file
> - * or directory to become readable.
> - *
> - * Only following redirects when redirects are enabled disables
> - * this attack vector when not necessary.
> - */
> - err = -EPERM;
> - if (d.redirect && !ovl_redirect_follow(ofs)) {
> - pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
> - dentry);
> - goto out_put;
> - }
> + if (d.redirect)
> + nextredirect = true;
>
> if (d.stop)
> break;
> @@ -1218,6 +1228,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> ctr++;
> }
>
> + if (nextmetacopy && !ofs->config.metacopy) {
> + pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> + err = -EPERM;
> + goto out_put;
> + }
> + if (nextredirect && !ovl_redirect_follow(ofs)) {
> + pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> + err = -EPERM;
> + goto out_put;
> + }
> +
> /*
> * For regular non-metacopy upper dentries, there is no lower
> * path based lookup, hence ctr will be zero. If a dentry is found
> --
> 2.48.1
>
