On Mon, Jul 3, 2023 at 9:30 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> On Wed, Jun 21, 2023 at 01:18:28PM +0200, Alexander Larsson wrote:
> > During regular metacopy, if lowerdata file has fs-verity enabled, and
> > the verity option is enabled, we add the digest to the metacopy xattr.
> >
> > If verity is required, and lowerdata does not have fs-verity enabled,
> > fall back to full copy-up (or the generated metacopy would not
> > validate).
> >
> > Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
> > fs/overlayfs/overlayfs.h | 3 +++
> > fs/overlayfs/util.c | 33 ++++++++++++++++++++++++++++-
> > 3 files changed, 78 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 68f01fd7f211..fce7d048673c 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -544,6 +544,7 @@ struct ovl_copy_up_ctx {
> > bool origin;
> > bool indexed;
> > bool metacopy;
> > + bool metacopy_digest;
> > };
> >
> > static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > @@ -641,8 +642,21 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > }
> >
> > if (c->metacopy) {
> > - err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
> > - NULL, 0, -EOPNOTSUPP);
> > + struct path lowerdatapath;
> > + struct ovl_metacopy metacopy_data = OVL_METACOPY_INIT;
> > +
> > + ovl_path_lowerdata(c->dentry, &lowerdatapath);
> > + if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> > + err = -EIO;
> > + else
> > + err = ovl_set_verity_xattr_from(ofs, &lowerdatapath, &metacopy_data);
>
> There's no dedicated verity xattr anymore, so maybe ovl_set_verity_xattr_from()
> should be renamed to something like ovl_get_verity_digest().
Yeah, that makes a lot of sense.
> > +
> > + if (metacopy_data.digest_algo)
> > + c->metacopy_digest = true;
> > +
> > + if (!err)
> > + err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> > +
> > if (err)
> > return err;
>
> The error handling above is a bit weird. Some early returns would make it
> easier to read:
>
> ovl_path_lowerdata(c->dentry, &lowerdatapath);
> if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> return -EIO;
> err = ovl_get_verity_digest(ofs, &lowerdatapath, &metacopy_data);
> if (err)
> return err;
>
> if (metacopy_data.digest_algo)
> c->metacopy_digest = true;
>
> err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> if (err)
> return err;
Yeah.
> > }
> > @@ -751,6 +765,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > if (err)
> > goto cleanup;
> >
> > + if (c->metacopy_digest)
> > + ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> > + else
> > + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> > + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> > +
> > if (!c->metacopy)
> > ovl_set_upperdata(d_inode(c->dentry));
> > inode = d_inode(c->dentry);
>
> Maybe the line 'inode = d_inode(c->dentry);' should be moved earlier, and then
> 'inode' used instead of 'd_inode(c->dentry)' later on.
I wonder why this wasn't already done for the ovl_set_upperdata(), but
it seems ok so I did this.
> > + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> > + struct path lowerdata;
> > +
> > + ovl_path_lowerdata(dentry, &lowerdata);
> > +
> > + if (WARN_ON_ONCE(lowerdata.dentry == NULL) ||
> > + ovl_ensure_verity_loaded(&lowerdata) ||
> > + !fsverity_get_info(d_inode(lowerdata.dentry))) {
> > + return false;
>
> Please use !fsverity_active() instead of !fsverity_get_info().
Done.
All these changes are in git:
https://github.com/alexlarsson/linux/commits/overlay-verity
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
