On Tue, Jun 20, 2023 at 1:15 PM Alexander Larsson <alexl@xxxxxxxxxx> 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>
Looks good.
Minor comments below
> ---
> fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
> fs/overlayfs/overlayfs.h | 3 +++
> fs/overlayfs/util.c | 32 +++++++++++++++++++++++++++-
> 3 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 68f01fd7f211..6e6c25836e52 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);
> +
> + if (metacopy_data.digest_algo != 0)
Nit: please drop "!= 0"
> + c->metacopy_digest = true;
> +
> + if (!err)
> + err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> +
> if (err)
> return err;
> }
> @@ -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);
> @@ -813,6 +833,12 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
> if (err)
> goto out_fput;
>
> + 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));
> ovl_inode_update(d_inode(c->dentry), dget(temp));
> @@ -918,6 +944,19 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
> if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
> return false;
>
> + /* Fall back to full copy if no fsverity on source data and we require verity */
> + 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;
> + }
> + }
> +
> return true;
> }
>
> @@ -984,6 +1023,8 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> if (err)
> goto out_free;
>
> + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> ovl_set_upperdata(d_inode(c->dentry));
> out_free:
> kfree(capability);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index c2213a8ad16e..eef4a3243e8a 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -517,11 +517,14 @@ int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> struct ovl_metacopy *metacopy);
> bool ovl_is_metacopy_dentry(struct dentry *dentry);
> char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
> +int ovl_ensure_verity_loaded(struct path *path);
> int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> u8 *digest_buf, int *buf_length);
> int ovl_validate_verity(struct ovl_fs *ofs,
> struct path *metapath,
> struct path *datapath);
> +int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
> + struct ovl_metacopy *metacopy);
> int ovl_sync_status(struct ovl_fs *ofs);
>
> static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 66448964f753..3841f04baf35 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1185,7 +1185,7 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
> }
>
> /* Call with mounter creds as it may open the file */
> -static int ovl_ensure_verity_loaded(struct path *datapath)
> +int ovl_ensure_verity_loaded(struct path *datapath)
> {
> struct inode *inode = d_inode(datapath->dentry);
> const struct fsverity_info *vi;
> @@ -1263,6 +1263,36 @@ int ovl_validate_verity(struct ovl_fs *ofs,
> return 0;
> }
>
> +int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
> + struct ovl_metacopy *metacopy)
> +{
> + int err, digest_size;
> +
> + if (!ofs->config.verity_mode || !S_ISREG(d_inode(src->dentry)->i_mode))
> + return 0;
> +
> + err = ovl_ensure_verity_loaded(src);
> + if (err < 0) {
> + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
> + src->dentry);
> + return -EIO;
> + }
> +
> + digest_size = fsverity_get_digest(d_inode(src->dentry),
> + metacopy->digest, &metacopy->digest_algo, NULL);
> + if (digest_size == 0) {
|| WARN_ON_ONCE(digest_size > FS_VERITY_MAX_DIGEST_SIZE))
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
> + src->dentry);
> + return -EIO;
> + }
> + return 0;
> + }
> +
ovl and fsverity are different modules, so we should have this
fail safety assertion in place.
It may look ridiculous that fsverity_get_digest() would return a size
larger than their own constant, but people may be loading an overlayfs
module not compiled with exact same fsverity code.
The results of this may be undefined, but at least we can make
the problem heard.
Thanks,
Amir.
