On Tue, Apr 8, 2025 at 5:40 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 an inconsistency in not checking metacopy found from the index.
>
> And also 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 these inconsistencies and make the logic more straightforward, paving
> the way for following patches to change when dataredirects are allowed.
missing space: dataredirects
>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> ---
> fs/overlayfs/namei.c | 81 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 57 insertions(+), 24 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index be5c65d6f848..5cebdd05ab3a 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -16,6 +16,7 @@
>
> struct ovl_lookup_data {
> struct super_block *sb;
> + struct dentry *dentry;
> const struct ovl_layer *layer;
> struct qstr name;
> bool is_dir;
> @@ -23,6 +24,8 @@ struct ovl_lookup_data {
> bool xwhiteouts;
> bool stop;
> bool last;
> + bool nextredirect;
> + bool nextmetacopy;
I think these are not needed
> char *redirect;
> int metacopy;
> /* Referring to last redirect xattr */
> @@ -1024,6 +1027,31 @@ int ovl_verify_lowerdata(struct dentry *dentry)
> return ovl_maybe_validate_verity(dentry);
> }
>
> +/*
> + * 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.
> + */
> +static bool ovl_check_nextredirect(struct ovl_lookup_data *d)
Looks much better with the helper.
May I suggest ovl_check_follow_redirect()
> +{
> + struct ovl_fs *ofs = OVL_FS(d->sb);
> +
> + if (d->nextmetacopy && !ofs->config.metacopy) {
Should be equivalent to
if (d->metacopy && !ofs->config.metacopy) {
In current code
> + pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
> + return false;
> + }
> + if (d->nextredirect && !ovl_redirect_follow(ofs)) {
Should be equivalent to
if (d->redirect && !ovl_redirect_follow(ofs)) {
With minor changes to index lookup code
> + pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
> + return false;
> + }
> + return true;
> +}
> +
> struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> unsigned int flags)
> {
> @@ -1047,6 +1075,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> int metacopy_size = 0;
> struct ovl_lookup_data d = {
> .sb = dentry->d_sb,
> + .dentry = dentry,
> .name = dentry->d_name,
> .is_dir = false,
> .opaque = false,
> @@ -1054,6 +1083,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> .redirect = NULL,
> .metacopy = 0,
> + .nextredirect = false,
> + .nextmetacopy = false,
> };
>
> if (dentry->d_name.len > ofs->namelen)
> @@ -1087,8 +1118,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;
> + d.nextmetacopy = true;
always set IFF (d.metacopy)
> + }
> metacopy_size = d.metacopy;
> }
>
> @@ -1099,6 +1132,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_put_upper;
> if (d.redirect[0] == '/')
> poe = roe;
> + d.nextredirect = true;
mostly set IFF (d.redirect)
> }
> upperopaque = d.opaque;
> }
> @@ -1113,6 +1147,11 @@ 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];
>
> + if (!ovl_check_nextredirect(&d)) {
> + 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 +1165,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)
> + d.nextmetacopy = true;
>
> /*
> * If no origin fh is stored in upper of a merge dir, store fh
> @@ -1185,22 +1220,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)
> + d.nextredirect = true;
>
> if (d.stop)
> break;
> @@ -1218,6 +1239,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> ctr++;
> }
>
> + if (!ovl_check_nextredirect(&d)) {
> + 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
> @@ -1307,11 +1333,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> upperredirect = NULL;
> goto out_free_oe;
> }
> + d.nextredirect = upperredirect;
> +
> err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
> if (err < 0)
> goto out_free_oe;
> - uppermetacopy = err;
> + d.nextmetacopy = uppermetacopy = err;
Could be changed to:
+ d.metacopy = uppermetacopy = err;
> metacopy_size = err;
> +
> + if (!ovl_check_nextredirect(&d)) {
> + err = -EPERM;
> + goto out_free_oe;
> + }
> }
>
We never really follow redirect from index
All upperredirect is ever used for is to suppress ovl_set_redirect()
after copy up of another lower hardlink and rename,
but also in that case, upperredirect is not going to be followed
(or trusted for that matter) until a new lookup of the copied up
alias and at that point ovl_check_follow_redirect() will be
called when upperdentry is found.
I think we do not need to check follow of redirect from index
I think it is enough to check follow of metacopy from index.
Therefore, I think there d.nextmetacopy and d.nextredirect are
completely implied from d.metacopy and d.redirect.
Thanks,
Amir.
