On Wed, Apr 9, 2025 at 8:09 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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.
On second thought, if unpriv user suppresses ovl_set_redirect()
by setting some mock redirect value on index maybe that lead to some
risk. Not worth overthinking about it.
Attached patch removed next* variables without this compromise.
Tested it squashed to patch 1 and minor rebase conflicts fixes in patch 2.
It passed your tests.
Thanks,
Amir.
From 2305730bc1c4ee35fcf0b88305e4add828bdff0b Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Wed, 9 Apr 2025 09:02:43 +0200
Subject: [PATCH] ovl: remove unneeded nextredirect/nextmetacopy
---
fs/overlayfs/namei.c | 48 +++++++++++++++++---------------------------
1 file changed, 18 insertions(+), 30 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5cebdd05ab3a..4fc2c47ebc55 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -24,9 +24,8 @@ struct ovl_lookup_data {
bool xwhiteouts;
bool stop;
bool last;
- bool nextredirect;
- bool nextmetacopy;
char *redirect;
+ char *upperredirect;
int metacopy;
/* Referring to last redirect xattr */
bool absolute_redirect;
@@ -1037,15 +1036,15 @@ int ovl_verify_lowerdata(struct dentry *dentry)
* Only following redirects when redirects are enabled disables this attack
* vector when not necessary.
*/
-static bool ovl_check_nextredirect(struct ovl_lookup_data *d)
+static bool ovl_check_follow_redirect(struct ovl_lookup_data *d)
{
struct ovl_fs *ofs = OVL_FS(d->sb);
- if (d->nextmetacopy && !ofs->config.metacopy) {
+ if (d->metacopy && !ofs->config.metacopy) {
pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
return false;
}
- if (d->nextredirect && !ovl_redirect_follow(ofs)) {
+ if ((d->redirect || d->upperredirect) && !ovl_redirect_follow(ofs)) {
pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
return false;
}
@@ -1067,7 +1066,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
unsigned int ctr = 0;
struct inode *inode = NULL;
bool upperopaque = false;
- char *upperredirect = NULL;
struct dentry *this;
unsigned int i;
int err;
@@ -1082,9 +1080,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
.stop = false,
.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
.redirect = NULL,
+ .upperredirect = NULL,
.metacopy = 0,
- .nextredirect = false,
- .nextmetacopy = false,
};
if (dentry->d_name.len > ofs->namelen)
@@ -1120,19 +1117,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
if (d.metacopy) {
uppermetacopy = true;
- d.nextmetacopy = true;
}
metacopy_size = d.metacopy;
}
if (d.redirect) {
err = -ENOMEM;
- upperredirect = kstrdup(d.redirect, GFP_KERNEL);
- if (!upperredirect)
+ d.upperredirect = kstrdup(d.redirect, GFP_KERNEL);
+ if (!d.upperredirect)
goto out_put_upper;
if (d.redirect[0] == '/')
poe = roe;
- d.nextredirect = true;
}
upperopaque = d.opaque;
}
@@ -1147,7 +1142,7 @@ 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)) {
+ if (!ovl_check_follow_redirect(&d)) {
err = -EPERM;
goto out_put;
}
@@ -1165,9 +1160,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
if (!this)
continue;
- if (d.metacopy)
- d.nextmetacopy = true;
-
/*
* If no origin fh is stored in upper of a merge dir, store fh
* of lower dir and set upper parent "impure".
@@ -1220,9 +1212,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
ctr++;
}
- if (d.redirect)
- d.nextredirect = true;
-
if (d.stop)
break;
@@ -1239,7 +1228,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
ctr++;
}
- if (!ovl_check_nextredirect(&d)) {
+ if (!ovl_check_follow_redirect(&d)) {
err = -EPERM;
goto out_put;
}
@@ -1324,24 +1313,23 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
/*
* It's safe to assign upperredirect here: the previous
- * assignment of happens only if upperdentry is non-NULL, and
+ * assignment happens only if upperdentry is non-NULL, and
* this one only if upperdentry is NULL.
*/
- upperredirect = ovl_get_redirect_xattr(ofs, &upperpath, 0);
- if (IS_ERR(upperredirect)) {
- err = PTR_ERR(upperredirect);
- upperredirect = NULL;
+ d.upperredirect = ovl_get_redirect_xattr(ofs, &upperpath, 0);
+ if (IS_ERR(d.upperredirect)) {
+ err = PTR_ERR(d.upperredirect);
+ d.upperredirect = NULL;
goto out_free_oe;
}
- d.nextredirect = upperredirect;
err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
if (err < 0)
goto out_free_oe;
- d.nextmetacopy = uppermetacopy = err;
+ d.metacopy = uppermetacopy = err;
metacopy_size = err;
- if (!ovl_check_nextredirect(&d)) {
+ if (!ovl_check_follow_redirect(&d)) {
err = -EPERM;
goto out_free_oe;
}
@@ -1352,7 +1340,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
.upperdentry = upperdentry,
.oe = oe,
.index = index,
- .redirect = upperredirect,
+ .redirect = d.upperredirect,
};
/* Store lowerdata redirect for lazy lookup */
@@ -1394,7 +1382,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
kfree(origin_path);
}
dput(upperdentry);
- kfree(upperredirect);
+ kfree(d.upperredirect);
out:
kfree(d.redirect);
ovl_revert_creds(old_cred);
--
2.34.1
