| Message ID | 20250408154011.673891-2-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: metacopy/verity fixes and improvements | expand |
On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> 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@redhat.com> > --- > 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.
On Wed, Apr 9, 2025 at 8:09 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> 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@redhat.com> > > --- > > 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.
On Wed, 9 Apr 2025 at 10:25, Amir Goldstein <amir73il@gmail.com> wrote: > 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. One more change: in this patch we just want the consistency fix, not the behavior change introduced in 2/3. So move the ovl_check_follow_redirect() to before the lazy-data check here and restore the order in the next patch. Pushed to overlayfs/vfs.git#overlayfs-next Thanks, Miklos
On Wed, Apr 9, 2025 at 1:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Wed, 9 Apr 2025 at 10:25, Amir Goldstein <amir73il@gmail.com> wrote: > > > 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. > > One more change: in this patch we just want the consistency fix, not > the behavior change introduced in 2/3. So move the > ovl_check_follow_redirect() to before the lazy-data check here and > restore the order in the next patch. Right. > > Pushed to overlayfs/vfs.git#overlayfs-next Looks good. Thanks, Amir. FYI, I am going to be on vacation 6.15-rc3..6.15-rc5.
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; 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) +{ + struct ovl_fs *ofs = OVL_FS(d->sb); + + if (d->nextmetacopy && !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)) { + 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; + } 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; } 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; metacopy_size = err; + + if (!ovl_check_nextredirect(&d)) { + err = -EPERM; + goto out_free_oe; + } } if (upperdentry || ctr) {
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. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/overlayfs/namei.c | 81 +++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 24 deletions(-)