Re: [PATCH v4 4/4] ovl: Handle verity during copy-up

[Eric Biggers <ebiggers@kernel.org>]

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@redhat.com>
> ---
>  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().

> +
> +		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;

>  	}
> @@ -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.

> +	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().

- Eric