| Message ID | 20250325104634.162496-4-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: metacopy/verity fixes and improvements | expand |
On Tue, Mar 25, 2025 at 11:46 AM 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 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@redhat.com> Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > 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 > -- > 2.49.0 >
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@redhat.com> Reviewed-by: Alexander Larsson <alexl@redhat.com> > --- > 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
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
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@redhat.com> --- fs/overlayfs/namei.c | 67 +++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 23 deletions(-)