On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi 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,
> paving the
> way for following patches to change when dataredirects are allowed.
>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
Reviewed-by: Alexander Larsson <alexl@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.
> + */
> + 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
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a witless guerilla astronaut with a mysterious suitcase handcuffed
to his arm. She's a mistrustful communist journalist with an incredible
destiny. They fight crime!
