[PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Alexander Larsson]

This patchset adds support for using fs-verity to validate lowerdata
files by specifying an overlay.verity xattr on the metacopy
files.

This is primarily motivated by the Composefs usecase, where there will
be a read-only EROFS layer that contains redirect into a base data
layer which has fs-verity enabled on all files. However, it is also
useful in general if you want to ensure that the lowerdata files
matches the expected content over time.

This patch series is based on the lazy lowerdata patch series by Amir[1].

I have also added some tests for this feature to xfstests[2].

I'm also CC:ing the fsverity list and maintainers because there is one
(tiny) fsverity change, and there may be interest in this usecase.

Changes since v1:
 * Rebased on v2 lazy lowerdata series
 * Dropped the "validate" mount option variant. We now only support
   "off", "on" and "require", where "off" is the default.
 * We now store the digest algorithm used in the overlay.verity xattr.
 * Dropped ability to configure default verity options, as this could
   cause problems moving layers between machines.
 * We now properly resolve dependent mount options by automatically
   enabling metacopy and redirect_dir if verity is on, or failing
   if the specified options conflict.
 * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
 * Renamed new helpers from ovl_entry_path_ to ovl_e_path_

[1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
[2] https://github.com/alexlarsson/xfstests/commits/verity-tests

Alexander Larsson (6):
  fsverity: Export fsverity_get_digest
  ovl: Break out ovl_e_path_real() from ovl_i_path_real()
  ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
  ovl: Add framework for verity support
  ovl: Validate verity xattr when resolving lowerdata
  ovl: Handle verity during copy-up

 Documentation/filesystems/overlayfs.rst |  27 ++++
 fs/overlayfs/copy_up.c                  |  31 +++++
 fs/overlayfs/namei.c                    |  42 +++++-
 fs/overlayfs/overlayfs.h                |  12 ++
 fs/overlayfs/ovl_entry.h                |   3 +
 fs/overlayfs/super.c                    |  74 ++++++++++-
 fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
 fs/verity/measure.c                     |   1 +
 8 files changed, 343 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH v2 1/6] fsverity: Export fsverity_get_digest

[Alexander Larsson]

Overlayfs needs to call this when built in module form, so
we need to export the symbol. This uses EXPORT_SYMBOL_GPL
like the other fsverity functions do.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/verity/measure.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index 5c79ea1b2468..875d143e0c7e 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -85,3 +85,4 @@ int fsverity_get_digest(struct inode *inode,
 	*alg = hash_alg->algo_id;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(fsverity_get_digest);
-- 
2.39.2

[PATCH v2 2/6] ovl: Break out ovl_e_path_real() from ovl_i_path_real()

[Alexander Larsson]

This allows us to get the real path from the ovl_entry in ovl_lookup()
before having finished setting up the resulting inode.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/util.c      | 25 ++++++++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index c1233eec2d40..6ce1c7906bb9 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -391,6 +391,8 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 void ovl_i_path_real(struct inode *inode, struct path *path);
+void ovl_e_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
+		     struct dentry *upperdentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index e526ab059872..c32252153e5e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -331,19 +331,30 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
 	return ovl_upperdentry_dereference(OVL_I(inode));
 }
 
-void ovl_i_path_real(struct inode *inode, struct path *path)
-{
-	struct ovl_path *lowerpath = ovl_lowerpath(OVL_I_E(inode));
+void ovl_e_path_real(struct ovl_fs *ofs,
+		     struct ovl_entry *oe,
+		     struct dentry *upperdentry,
+		     struct path *path)
+{
+	if (upperdentry) {
+		path->dentry = upperdentry;
+		path->mnt = ovl_upper_mnt(ofs);
+	} else {
+		struct ovl_path *lowerpath = ovl_lowerpath(oe);
 
-	path->dentry = ovl_i_dentry_upper(inode);
-	if (!path->dentry) {
 		path->dentry = lowerpath->dentry;
 		path->mnt = lowerpath->layer->mnt;
-	} else {
-		path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
 	}
 }
 
+void ovl_i_path_real(struct inode *inode, struct path *path)
+{
+	ovl_e_path_real(OVL_FS(inode->i_sb),
+			OVL_I_E(inode),
+			ovl_i_dentry_upper(inode),
+			path);
+}
+
 struct inode *ovl_inode_upper(struct inode *inode)
 {
 	struct dentry *upperdentry = ovl_i_dentry_upper(inode);
-- 
2.39.2

[PATCH v2 3/6] ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()

[Alexander Larsson]

This will be needed later when getting the lowerdata path from the
ovl_entry in ovl_lookup() before the dentry is set up.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h | 1 +
 fs/overlayfs/util.c      | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6ce1c7906bb9..a4867ff97115 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -391,6 +391,7 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 void ovl_i_path_real(struct inode *inode, struct path *path);
+void ovl_e_path_lowerdata(struct ovl_entry *oe, struct path *path);
 void ovl_e_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
 		     struct dentry *upperdentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index c32252153e5e..74077ef50bb3 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -222,9 +222,9 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 	}
 }
 
-void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
+void ovl_e_path_lowerdata(struct ovl_entry *oe,
+			  struct path *path)
 {
-	struct ovl_entry *oe = OVL_E(dentry);
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
 	struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
 
@@ -242,6 +242,11 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 	}
 }
 
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
+{
+	return ovl_e_path_lowerdata(OVL_E(dentry), path);
+}
+
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path)
 {
 	enum ovl_path_type type = ovl_path_type(dentry);
-- 
2.39.2

[PATCH v2 4/6] ovl: Add framework for verity support

[Alexander Larsson]

This adds the scaffolding (docs, config, mount options) for supporting
for a new overlay xattr "overlay.verity", which contains a fs-verity
digest. This is used for metacopy files, and the actual fs-verity
digest of the lowerdata file needs to match it. The mount option
"verity" specifies how this xattrs is handled.

If you enable verity it ("verity=on") all existing xattrs are
validated before use, and during metacopy we generate verity xattr in
the upper metacopy file if the source file has verity enabled. This
means later accesses can guarantee that the correct data is used.

Additionally you can use "verity=require". In this mode all metacopy
files must have a valid verity xattr. For this to work metadata
copy-up must be able to create a verity xattr (so that later accesses
are validated). Therefore, in this mode, if the lower data file
doesn't have fs-verity enabled we fall back to a full copy rather than
a metacopy.

Actual implementation follows in a separate commit.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 Documentation/filesystems/overlayfs.rst | 27 +++++++++
 fs/overlayfs/ovl_entry.h                |  3 +
 fs/overlayfs/super.c                    | 74 ++++++++++++++++++++++++-
 3 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index bc95343bafba..7e2b445a4139 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -407,6 +407,33 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect"
 to the absolute path of the "lower data" file in the "data-only" lower layer.
 
 
+fs-verity support
+----------------------
+
+When metadata copy up is used for a file, then the xattr
+"trusted.overlay.verity" may be set on the metacopy file. This
+specifies the expected fs-verity digest of the lowerdata file. This
+may then be used to verify the content of the source file at the time
+the file is opened. During metacopy copy up overlayfs can also set
+this xattr.
+
+This is controlled by the "verity" mount option, which supports
+these values:
+
+- "off":
+    The verity xattr is never used. This is the default if verity
+    option is not specified.
+- "on":
+    Whenever a metacopy files specifies an expected digest, the
+    corresponding data file must match the specified digest.
+    When generating a metacopy file the verity xattr will be set
+    from the source file fs-verity digest (if it has one).
+- "require":
+    Same as "on", but additionally all metacopy files must specify a
+    verity xattr. This means metadata copy up will only be used if
+    the data file has fs-verity enabled, otherwise a full copy-up is
+    used.
+
 Sharing and copying layers
 --------------------------
 
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index c6c7d09b494e..95464a1cb371 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -13,6 +13,9 @@ struct ovl_config {
 	bool redirect_dir;
 	bool redirect_follow;
 	const char *redirect_mode;
+	bool verity;
+	bool require_verity;
+	const char *verity_mode;
 	bool index;
 	bool uuid;
 	bool nfs_export;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c6209592bb3f..a4662883b619 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -244,6 +244,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 	kfree(ofs->config.upperdir);
 	kfree(ofs->config.workdir);
 	kfree(ofs->config.redirect_mode);
+	kfree(ofs->config.verity_mode);
 	if (ofs->creator_cred)
 		put_cred(ofs->creator_cred);
 	kfree(ofs);
@@ -334,6 +335,11 @@ static const char *ovl_redirect_mode_def(void)
 	return ovl_redirect_dir_def ? "on" : "off";
 }
 
+static const char *ovl_verity_mode_def(void)
+{
+	return "off";
+}
+
 static const char * const ovl_xino_str[] = {
 	"off",
 	"auto",
@@ -383,6 +389,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 		seq_puts(m, ",volatile");
 	if (ofs->config.userxattr)
 		seq_puts(m, ",userxattr");
+	if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0)
+		seq_printf(m, ",verity=%s", ofs->config.verity_mode);
 	return 0;
 }
 
@@ -438,6 +446,7 @@ enum {
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
 	OPT_VOLATILE,
+	OPT_VERITY,
 	OPT_ERR,
 };
 
@@ -460,6 +469,7 @@ static const match_table_t ovl_tokens = {
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_VOLATILE,			"volatile"},
+	{OPT_VERITY,			"verity=%s"},
 	{OPT_ERR,			NULL}
 };
 
@@ -509,6 +519,21 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
 	return 0;
 }
 
+static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode)
+{
+	if (strcmp(mode, "on") == 0) {
+		config->verity = true;
+	} else if (strcmp(mode, "require") == 0) {
+		config->verity = true;
+		config->require_verity = true;
+	} else if (strcmp(mode, "off") != 0) {
+		pr_err("bad mount option \"verity=%s\"\n", mode);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ovl_parse_opt(char *opt, struct ovl_config *config)
 {
 	char *p;
@@ -520,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	if (!config->redirect_mode)
 		return -ENOMEM;
 
+	config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL);
+	if (!config->verity_mode)
+		return -ENOMEM;
+
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;
 		substring_t args[MAX_OPT_ARGS];
@@ -620,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->userxattr = true;
 			break;
 
+		case OPT_VERITY:
+			kfree(config->verity_mode);
+			config->verity_mode = match_strdup(&args[0]);
+			if (!config->verity_mode)
+				return -ENOMEM;
+			break;
+
 		default:
 			pr_err("unrecognized mount option \"%s\" or missing value\n",
 					p);
@@ -651,6 +687,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	if (err)
 		return err;
 
+	err = ovl_parse_verity_mode(config, config->verity_mode);
+	if (err)
+		return err;
+
+	/* Resolve verity -> metacopy dependency */
+	if (config->verity && !config->metacopy) {
+		/* Don't allow explicit specified conflicting combinations */
+		if (metacopy_opt) {
+			pr_err("conflicting options: metacopy=off,verity=%s\n",
+			       config->verity_mode);
+			return -EINVAL;
+		}
+		/* Otherwise automatically enable metacopy. */
+		config->metacopy = true;
+	}
+
 	/*
 	 * This is to make the logic below simpler.  It doesn't make any other
 	 * difference, since config->redirect_dir is only used for upper.
@@ -665,6 +717,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			       config->redirect_mode);
 			return -EINVAL;
 		}
+		if (config->verity && redirect_opt) {
+			pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
+			       config->verity_mode, config->redirect_mode);
+			return -EINVAL;
+		}
 		if (redirect_opt) {
 			/*
 			 * There was an explicit redirect_dir=... that resulted
@@ -700,7 +757,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
-	/* Resolve nfs_export -> !metacopy dependency */
+	/* Resolve nfs_export -> !metacopy && !verity dependency */
 	if (config->nfs_export && config->metacopy) {
 		if (nfs_export_opt && metacopy_opt) {
 			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
@@ -713,6 +770,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			 */
 			pr_info("disabling nfs_export due to metacopy=on\n");
 			config->nfs_export = false;
+		} else if (config->verity) {
+			/*
+			 * There was an explicit verity=.. that resulted
+			 * in this conflict.
+			 */
+			pr_info("disabling nfs_export due to verity=%s\n",
+				config->verity_mode);
+			config->nfs_export = false;
 		} else {
 			/*
 			 * There was an explicit nfs_export=on that resulted
@@ -724,7 +789,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	}
 
 
-	/* Resolve userxattr -> !redirect && !metacopy dependency */
+	/* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
 	if (config->userxattr) {
 		if (config->redirect_follow && redirect_opt) {
 			pr_err("conflicting options: userxattr,redirect_dir=%s\n",
@@ -735,6 +800,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			pr_err("conflicting options: userxattr,metacopy=on\n");
 			return -EINVAL;
 		}
+		if (config->verity) {
+			pr_err("conflicting options: userxattr,verity=%s\n",
+			       config->verity_mode);
+			return -EINVAL;
+		}
 		/*
 		 * Silently disable default setting of redirect and metacopy.
 		 * This shall be the default in the future as well: these
-- 
2.39.2

[PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

When resolving lowerdata (lazily or non-lazily) we check the
overlay.verity xattr on the metadata inode, and if set verify that the
source lowerdata inode matches it (according to the verity options
enabled).

Note that this changes the location of the revert_creds() call
in ovl_maybe_lookup_lowerdata() to ensure that we use the mounter creds
during the call to ovl_validate_verity() for the possible file access in
ovl_ensure_verity_loaded().

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/namei.c     | 42 +++++++++++++++++-
 fs/overlayfs/overlayfs.h |  6 +++
 fs/overlayfs/util.c      | 96 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 292b8a948f1a..d664ecc93e0f 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 /* Lazy lookup of lowerdata */
 int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 {
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct inode *inode = d_inode(dentry);
 	const char *redirect = ovl_lowerdata_redirect(inode);
 	struct ovl_path datapath = {};
@@ -915,9 +916,25 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
-	revert_creds(old_cred);
 	if (err)
-		goto out_err;
+		goto out_revert_creds;
+
+	if (ofs->config.verity) {
+		struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, };
+		struct path metapath = {};
+
+		ovl_path_real(dentry, &metapath);
+		if (!metapath.dentry) {
+			err = -EIO;
+			goto out_revert_creds;
+		}
+
+		err = ovl_validate_verity(ofs, &metapath, &data);
+		if (err)
+			goto out_revert_creds;
+	}
+
+	revert_creds(old_cred);
 
 	err = ovl_dentry_set_lowerdata(dentry, &datapath);
 	if (err)
@@ -929,6 +946,9 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 
 	return err;
 
+ out_revert_creds:
+	revert_creds(old_cred);
+
 out_err:
 	pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2, err=%i)\n",
 			    dentry, err);
@@ -1187,6 +1207,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
 
+	/* Validate verity of lower-data */
+	if (ofs->config.verity &&
+	    !d.is_dir && (uppermetacopy || ctr > 1)) {
+		struct path datapath;
+
+		ovl_e_path_lowerdata(oe, &datapath);
+
+		/* Is NULL for lazy lookup, will be verified later */
+		if (datapath.dentry) {
+			struct path metapath;
+
+			ovl_e_path_real(ofs, oe, upperdentry, &metapath);
+			err = ovl_validate_verity(ofs, &metapath, &datapath);
+			if (err < 0)
+				goto out_free_oe;
+		}
+	}
+
 	if (upperopaque)
 		ovl_dentry_set_opaque(dentry);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index a4867ff97115..07475eaae2ca 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -38,6 +38,7 @@ enum ovl_xattr {
 	OVL_XATTR_UPPER,
 	OVL_XATTR_METACOPY,
 	OVL_XATTR_PROTATTR,
+	OVL_XATTR_VERITY,
 };
 
 enum ovl_inode_flag {
@@ -463,6 +464,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
 int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
 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_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_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 74077ef50bb3..ee296614bd73 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -10,7 +10,9 @@
 #include <linux/cred.h>
 #include <linux/xattr.h>
 #include <linux/exportfs.h>
+#include <linux/file.h>
 #include <linux/fileattr.h>
+#include <linux/fsverity.h>
 #include <linux/uuid.h>
 #include <linux/namei.h>
 #include <linux/ratelimit.h>
@@ -720,6 +722,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path,
 #define OVL_XATTR_UPPER_POSTFIX		"upper"
 #define OVL_XATTR_METACOPY_POSTFIX	"metacopy"
 #define OVL_XATTR_PROTATTR_POSTFIX	"protattr"
+#define OVL_XATTR_VERITY_POSTFIX	"verity"
 
 #define OVL_XATTR_TAB_ENTRY(x) \
 	[x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
@@ -734,6 +737,7 @@ const char *const ovl_xattr_table[][2] = {
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR),
+	OVL_XATTR_TAB_ENTRY(OVL_XATTR_VERITY),
 };
 
 int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
@@ -1166,6 +1170,98 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
 	return ERR_PTR(res);
 }
 
+int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
+			 u8 *digest_buf, int *buf_length)
+{
+	int res;
+
+	res = ovl_path_getxattr(ofs, path, OVL_XATTR_VERITY, digest_buf, *buf_length);
+	if (res == -ENODATA || res == -EOPNOTSUPP)
+		return -ENODATA;
+	if (res < 0) {
+		pr_warn_ratelimited("failed to get digest (%i)\n", res);
+		return res;
+	}
+
+	*buf_length = res;
+	return 0;
+}
+
+/* Call with mounter creds as it may open the file */
+static int ovl_ensure_verity_loaded(struct path *datapath)
+{
+	struct inode *inode = d_inode(datapath->dentry);
+	const struct fsverity_info *vi;
+	struct file *filp;
+
+	vi = fsverity_get_info(inode);
+	if (vi == NULL && IS_VERITY(inode)) {
+		/*
+		 * If this inode was not yet opened, the verity info hasn't been
+		 * loaded yet, so we need to do that here to force it into memory.
+		 * We use open_with_fake_path to avoid ENFILE.
+		 */
+		filp = open_with_fake_path(datapath, O_RDONLY, inode, current_cred());
+		if (IS_ERR(filp))
+			return PTR_ERR(filp);
+		fput(filp);
+	}
+
+	return 0;
+}
+
+int ovl_validate_verity(struct ovl_fs *ofs,
+			struct path *metapath,
+			struct path *datapath)
+{
+	u8 xattr_data[1+FS_VERITY_MAX_DIGEST_SIZE];
+	u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
+	enum hash_algo verity_algo;
+	int xattr_len;
+	int err;
+
+	if (!ofs->config.verity ||
+	    /* Verity only works on regular files */
+	    !S_ISREG(d_inode(metapath->dentry)->i_mode))
+		return 0;
+
+	xattr_len = sizeof(xattr_data);
+	err = ovl_get_verity_xattr(ofs, metapath, xattr_data, &xattr_len);
+	if (err == -ENODATA) {
+		if (ofs->config.require_verity) {
+			pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n",
+					    metapath->dentry);
+			return -EIO;
+		}
+		return 0;
+	}
+	if (err < 0)
+		return err;
+
+	err = ovl_ensure_verity_loaded(datapath);
+	if (err < 0) {
+		pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
+				    datapath->dentry);
+		return -EIO;
+	}
+
+	err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo);
+	if (err < 0) {
+		pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry);
+		return -EIO;
+	}
+
+	if (xattr_len != 1 + hash_digest_size[verity_algo] ||
+	    xattr_data[0] != (u8) verity_algo ||
+	    memcmp(xattr_data+1, actual_digest, xattr_len - 1) != 0) {
+		pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
+				    datapath->dentry);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 /*
  * ovl_sync_status() - Check fs sync status for volatile mounts
  *
-- 
2.39.2

[PATCH v2 6/6] ovl: Handle verity during copy-up

[Alexander Larsson]

During regular metacopy, if lowerdata file has fs-verity enabled,
set the new overlay.verity xattr (if enabled).

During real data copy up, remove any old overlay.verity 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   | 31 +++++++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |  3 +++
 fs/overlayfs/util.c      | 39 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index eb266fb68730..e25bdc2baef3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -19,6 +19,7 @@
 #include <linux/fdtable.h>
 #include <linux/ratelimit.h>
 #include <linux/exportfs.h>
+#include <linux/fsverity.h>
 #include "overlayfs.h"
 
 #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -644,6 +645,18 @@ 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);
+
+		/* Copy the verity digest if any so we can validate the copy-up later */
+		if (!err) {
+			struct path lowerdatapath;
+
+			ovl_path_lowerdata(c->dentry, &lowerdatapath);
+			if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
+				err = -EIO;
+			else
+				err = ovl_set_verity_xattr_from(ofs, temp, &lowerdatapath);
+		}
+
 		if (err)
 			return err;
 	}
@@ -919,6 +932,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.require_verity) {
+		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;
 }
 
@@ -985,6 +1011,11 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto out_free;
 
+	err = ovl_removexattr(ofs, upperpath.dentry, OVL_XATTR_VERITY);
+	if (err && err != -ENODATA)
+		goto out_free;
+
+	err = 0;
 	ovl_set_upperdata(d_inode(c->dentry));
 out_free:
 	kfree(capability);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 07475eaae2ca..1cc3c8df3a4d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -464,11 +464,14 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
 int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
 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 dentry *dst,
+			      struct path *src);
 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 ee296614bd73..733871775b80 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1188,7 +1188,7 @@ int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
 }
 
 /* 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;
@@ -1262,6 +1262,43 @@ int ovl_validate_verity(struct ovl_fs *ofs,
 	return 0;
 }
 
+int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct dentry *dst,
+			      struct path *src)
+{
+	int err;
+	u8 src_digest[1+FS_VERITY_MAX_DIGEST_SIZE];
+	enum hash_algo verity_algo;
+
+	if (!ofs->config.verity || !S_ISREG(d_inode(dst)->i_mode))
+		return 0;
+
+	err = -EIO;
+	if (src) {
+		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;
+		}
+
+		err = fsverity_get_digest(d_inode(src->dentry), src_digest + 1, &verity_algo);
+	}
+	if (err == -ENODATA) {
+		if (ofs->config.require_verity) {
+			pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
+					    src->dentry);
+			return -EIO;
+		}
+		return 0;
+	}
+	if (err < 0)
+		return err;
+
+	src_digest[0] = (u8)verity_algo;
+	return ovl_check_setxattr(ofs, dst, OVL_XATTR_VERITY,
+				  src_digest, 1 + hash_digest_size[verity_algo], -EOPNOTSUPP);
+}
+
 /*
  * ovl_sync_status() - Check fs sync status for volatile mounts
  *
-- 
2.39.2

Re: [PATCH v2 4/6] ovl: Add framework for verity support

[Amir Goldstein]

On Wed, May 3, 2023 at 11:52 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> This adds the scaffolding (docs, config, mount options) for supporting
> for a new overlay xattr "overlay.verity", which contains a fs-verity
> digest. This is used for metacopy files, and the actual fs-verity
> digest of the lowerdata file needs to match it. The mount option
> "verity" specifies how this xattrs is handled.
>
> If you enable verity it ("verity=on") all existing xattrs are
> validated before use, and during metacopy we generate verity xattr in
> the upper metacopy file if the source file has verity enabled. This
> means later accesses can guarantee that the correct data is used.
>
> Additionally you can use "verity=require". In this mode all metacopy
> files must have a valid verity xattr. For this to work metadata
> copy-up must be able to create a verity xattr (so that later accesses
> are validated). Therefore, in this mode, if the lower data file
> doesn't have fs-verity enabled we fall back to a full copy rather than
> a metacopy.
>
> Actual implementation follows in a separate commit.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  Documentation/filesystems/overlayfs.rst | 27 +++++++++
>  fs/overlayfs/ovl_entry.h                |  3 +
>  fs/overlayfs/super.c                    | 74 ++++++++++++++++++++++++-
>  3 files changed, 102 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index bc95343bafba..7e2b445a4139 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -407,6 +407,33 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect"
>  to the absolute path of the "lower data" file in the "data-only" lower layer.
>
>
> +fs-verity support
> +----------------------
> +
> +When metadata copy up is used for a file, then the xattr
> +"trusted.overlay.verity" may be set on the metacopy file. This
> +specifies the expected fs-verity digest of the lowerdata file. This
> +may then be used to verify the content of the source file at the time
> +the file is opened. During metacopy copy up overlayfs can also set
> +this xattr.
> +
> +This is controlled by the "verity" mount option, which supports
> +these values:
> +
> +- "off":
> +    The verity xattr is never used. This is the default if verity
> +    option is not specified.
> +- "on":
> +    Whenever a metacopy files specifies an expected digest, the
> +    corresponding data file must match the specified digest.
> +    When generating a metacopy file the verity xattr will be set
> +    from the source file fs-verity digest (if it has one).
> +- "require":
> +    Same as "on", but additionally all metacopy files must specify a
> +    verity xattr. This means metadata copy up will only be used if
> +    the data file has fs-verity enabled, otherwise a full copy-up is
> +    used.
> +
>  Sharing and copying layers
>  --------------------------
>
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index c6c7d09b494e..95464a1cb371 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -13,6 +13,9 @@ struct ovl_config {
>         bool redirect_dir;
>         bool redirect_follow;
>         const char *redirect_mode;
> +       bool verity;
> +       bool require_verity;
> +       const char *verity_mode;
>         bool index;
>         bool uuid;
>         bool nfs_export;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index c6209592bb3f..a4662883b619 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -244,6 +244,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>         kfree(ofs->config.upperdir);
>         kfree(ofs->config.workdir);
>         kfree(ofs->config.redirect_mode);
> +       kfree(ofs->config.verity_mode);
>         if (ofs->creator_cred)
>                 put_cred(ofs->creator_cred);
>         kfree(ofs);
> @@ -334,6 +335,11 @@ static const char *ovl_redirect_mode_def(void)
>         return ovl_redirect_dir_def ? "on" : "off";
>  }
>
> +static const char *ovl_verity_mode_def(void)
> +{
> +       return "off";
> +}
> +
>  static const char * const ovl_xino_str[] = {
>         "off",
>         "auto",
> @@ -383,6 +389,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>                 seq_puts(m, ",volatile");
>         if (ofs->config.userxattr)
>                 seq_puts(m, ",userxattr");
> +       if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0)
> +               seq_printf(m, ",verity=%s", ofs->config.verity_mode);
>         return 0;
>  }
>
> @@ -438,6 +446,7 @@ enum {
>         OPT_METACOPY_ON,
>         OPT_METACOPY_OFF,
>         OPT_VOLATILE,
> +       OPT_VERITY,
>         OPT_ERR,
>  };
>
> @@ -460,6 +469,7 @@ static const match_table_t ovl_tokens = {
>         {OPT_METACOPY_ON,               "metacopy=on"},
>         {OPT_METACOPY_OFF,              "metacopy=off"},
>         {OPT_VOLATILE,                  "volatile"},
> +       {OPT_VERITY,                    "verity=%s"},
>         {OPT_ERR,                       NULL}
>  };
>
> @@ -509,6 +519,21 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
>         return 0;
>  }
>
> +static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode)
> +{
> +       if (strcmp(mode, "on") == 0) {
> +               config->verity = true;
> +       } else if (strcmp(mode, "require") == 0) {
> +               config->verity = true;
> +               config->require_verity = true;
> +       } else if (strcmp(mode, "off") != 0) {
> +               pr_err("bad mount option \"verity=%s\"\n", mode);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  {
>         char *p;
> @@ -520,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>         if (!config->redirect_mode)
>                 return -ENOMEM;
>
> +       config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL);
> +       if (!config->verity_mode)
> +               return -ENOMEM;
> +
>         while ((p = ovl_next_opt(&opt)) != NULL) {
>                 int token;
>                 substring_t args[MAX_OPT_ARGS];
> @@ -620,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         config->userxattr = true;
>                         break;
>
> +               case OPT_VERITY:
> +                       kfree(config->verity_mode);
> +                       config->verity_mode = match_strdup(&args[0]);
> +                       if (!config->verity_mode)
> +                               return -ENOMEM;
> +                       break;
> +
>                 default:
>                         pr_err("unrecognized mount option \"%s\" or missing value\n",
>                                         p);
> @@ -651,6 +687,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>         if (err)
>                 return err;
>
> +       err = ovl_parse_verity_mode(config, config->verity_mode);
> +       if (err)
> +               return err;
> +
> +       /* Resolve verity -> metacopy dependency */
> +       if (config->verity && !config->metacopy) {
> +               /* Don't allow explicit specified conflicting combinations */
> +               if (metacopy_opt) {
> +                       pr_err("conflicting options: metacopy=off,verity=%s\n",
> +                              config->verity_mode);
> +                       return -EINVAL;
> +               }
> +               /* Otherwise automatically enable metacopy. */
> +               config->metacopy = true;
> +       }
> +
>         /*
>          * This is to make the logic below simpler.  It doesn't make any other
>          * difference, since config->redirect_dir is only used for upper.
> @@ -665,6 +717,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                                config->redirect_mode);
>                         return -EINVAL;
>                 }
> +               if (config->verity && redirect_opt) {
> +                       pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
> +                              config->verity_mode, config->redirect_mode);
> +                       return -EINVAL;
> +               }
>                 if (redirect_opt) {
>                         /*
>                          * There was an explicit redirect_dir=... that resulted
> @@ -700,7 +757,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                 }
>         }
>
> -       /* Resolve nfs_export -> !metacopy dependency */
> +       /* Resolve nfs_export -> !metacopy && !verity dependency */
>         if (config->nfs_export && config->metacopy) {
>                 if (nfs_export_opt && metacopy_opt) {
>                         pr_err("conflicting options: nfs_export=on,metacopy=on\n");
> @@ -713,6 +770,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                          */
>                         pr_info("disabling nfs_export due to metacopy=on\n");
>                         config->nfs_export = false;
> +               } else if (config->verity) {
> +                       /*
> +                        * There was an explicit verity=.. that resulted
> +                        * in this conflict.
> +                        */
> +                       pr_info("disabling nfs_export due to verity=%s\n",
> +                               config->verity_mode);
> +                       config->nfs_export = false;
>                 } else {
>                         /*
>                          * There was an explicit nfs_export=on that resulted
> @@ -724,7 +789,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>         }
>
>
> -       /* Resolve userxattr -> !redirect && !metacopy dependency */
> +       /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
>         if (config->userxattr) {
>                 if (config->redirect_follow && redirect_opt) {
>                         pr_err("conflicting options: userxattr,redirect_dir=%s\n",
> @@ -735,6 +800,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         pr_err("conflicting options: userxattr,metacopy=on\n");
>                         return -EINVAL;
>                 }
> +               if (config->verity) {
> +                       pr_err("conflicting options: userxattr,verity=%s\n",
> +                              config->verity_mode);
> +                       return -EINVAL;
> +               }
>                 /*
>                  * Silently disable default setting of redirect and metacopy.
>                  * This shall be the default in the future as well: these
> --
> 2.39.2
>

Re: [PATCH v2 6/6] ovl: Handle verity during copy-up

[Amir Goldstein]

On Wed, May 3, 2023 at 11:52 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> During regular metacopy, if lowerdata file has fs-verity enabled,
> set the new overlay.verity xattr (if enabled).
>
> During real data copy up, remove any old overlay.verity 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>

Looks fine to me
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/copy_up.c   | 31 +++++++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  3 +++
>  fs/overlayfs/util.c      | 39 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index eb266fb68730..e25bdc2baef3 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -19,6 +19,7 @@
>  #include <linux/fdtable.h>
>  #include <linux/ratelimit.h>
>  #include <linux/exportfs.h>
> +#include <linux/fsverity.h>
>  #include "overlayfs.h"
>
>  #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
> @@ -644,6 +645,18 @@ 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);
> +
> +               /* Copy the verity digest if any so we can validate the copy-up later */
> +               if (!err) {
> +                       struct path lowerdatapath;
> +
> +                       ovl_path_lowerdata(c->dentry, &lowerdatapath);
> +                       if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> +                               err = -EIO;
> +                       else
> +                               err = ovl_set_verity_xattr_from(ofs, temp, &lowerdatapath);
> +               }
> +
>                 if (err)
>                         return err;
>         }
> @@ -919,6 +932,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.require_verity) {
> +               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;
>  }
>
> @@ -985,6 +1011,11 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>         if (err)
>                 goto out_free;
>
> +       err = ovl_removexattr(ofs, upperpath.dentry, OVL_XATTR_VERITY);
> +       if (err && err != -ENODATA)
> +               goto out_free;
> +
> +       err = 0;
>         ovl_set_upperdata(d_inode(c->dentry));
>  out_free:
>         kfree(capability);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 07475eaae2ca..1cc3c8df3a4d 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -464,11 +464,14 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
>  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 dentry *dst,
> +                             struct path *src);
>  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 ee296614bd73..733871775b80 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1188,7 +1188,7 @@ int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
>  }
>
>  /* 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;
> @@ -1262,6 +1262,43 @@ int ovl_validate_verity(struct ovl_fs *ofs,
>         return 0;
>  }
>
> +int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct dentry *dst,
> +                             struct path *src)
> +{
> +       int err;
> +       u8 src_digest[1+FS_VERITY_MAX_DIGEST_SIZE];
> +       enum hash_algo verity_algo;
> +
> +       if (!ofs->config.verity || !S_ISREG(d_inode(dst)->i_mode))
> +               return 0;
> +
> +       err = -EIO;
> +       if (src) {
> +               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;
> +               }
> +
> +               err = fsverity_get_digest(d_inode(src->dentry), src_digest + 1, &verity_algo);
> +       }
> +       if (err == -ENODATA) {
> +               if (ofs->config.require_verity) {
> +                       pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
> +                                           src->dentry);
> +                       return -EIO;
> +               }
> +               return 0;
> +       }
> +       if (err < 0)
> +               return err;
> +
> +       src_digest[0] = (u8)verity_algo;
> +       return ovl_check_setxattr(ofs, dst, OVL_XATTR_VERITY,
> +                                 src_digest, 1 + hash_digest_size[verity_algo], -EOPNOTSUPP);
> +}
> +
>  /*
>   * ovl_sync_status() - Check fs sync status for volatile mounts
>   *
> --
> 2.39.2
>

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Wed, May 3, 2023 at 11:52 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> When resolving lowerdata (lazily or non-lazily) we check the
> overlay.verity xattr on the metadata inode, and if set verify that the
> source lowerdata inode matches it (according to the verity options
> enabled).
>
> Note that this changes the location of the revert_creds() call
> in ovl_maybe_lookup_lowerdata() to ensure that we use the mounter creds
> during the call to ovl_validate_verity() for the possible file access in
> ovl_ensure_verity_loaded().
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>

As far as I can tell without knowing fsverity to well...

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/namei.c     | 42 +++++++++++++++++-
>  fs/overlayfs/overlayfs.h |  6 +++
>  fs/overlayfs/util.c      | 96 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 292b8a948f1a..d664ecc93e0f 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
>  /* Lazy lookup of lowerdata */
>  int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
>  {
> +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>         struct inode *inode = d_inode(dentry);
>         const char *redirect = ovl_lowerdata_redirect(inode);
>         struct ovl_path datapath = {};
> @@ -915,9 +916,25 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
>
>         old_cred = ovl_override_creds(dentry->d_sb);
>         err = ovl_lookup_data_layers(dentry, redirect, &datapath);
> -       revert_creds(old_cred);
>         if (err)
> -               goto out_err;
> +               goto out_revert_creds;
> +
> +       if (ofs->config.verity) {
> +               struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, };
> +               struct path metapath = {};
> +
> +               ovl_path_real(dentry, &metapath);
> +               if (!metapath.dentry) {
> +                       err = -EIO;
> +                       goto out_revert_creds;
> +               }
> +
> +               err = ovl_validate_verity(ofs, &metapath, &data);
> +               if (err)
> +                       goto out_revert_creds;
> +       }
> +
> +       revert_creds(old_cred);
>
>         err = ovl_dentry_set_lowerdata(dentry, &datapath);
>         if (err)
> @@ -929,6 +946,9 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
>
>         return err;
>
> + out_revert_creds:
> +       revert_creds(old_cred);
> +
>  out_err:
>         pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2, err=%i)\n",
>                             dentry, err);
> @@ -1187,6 +1207,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
>         ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
>
> +       /* Validate verity of lower-data */
> +       if (ofs->config.verity &&
> +           !d.is_dir && (uppermetacopy || ctr > 1)) {
> +               struct path datapath;
> +
> +               ovl_e_path_lowerdata(oe, &datapath);
> +
> +               /* Is NULL for lazy lookup, will be verified later */
> +               if (datapath.dentry) {
> +                       struct path metapath;
> +
> +                       ovl_e_path_real(ofs, oe, upperdentry, &metapath);
> +                       err = ovl_validate_verity(ofs, &metapath, &datapath);
> +                       if (err < 0)
> +                               goto out_free_oe;
> +               }
> +       }
> +
>         if (upperopaque)
>                 ovl_dentry_set_opaque(dentry);
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index a4867ff97115..07475eaae2ca 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -38,6 +38,7 @@ enum ovl_xattr {
>         OVL_XATTR_UPPER,
>         OVL_XATTR_METACOPY,
>         OVL_XATTR_PROTATTR,
> +       OVL_XATTR_VERITY,
>  };
>
>  enum ovl_inode_flag {
> @@ -463,6 +464,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
>  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_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_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 74077ef50bb3..ee296614bd73 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -10,7 +10,9 @@
>  #include <linux/cred.h>
>  #include <linux/xattr.h>
>  #include <linux/exportfs.h>
> +#include <linux/file.h>
>  #include <linux/fileattr.h>
> +#include <linux/fsverity.h>
>  #include <linux/uuid.h>
>  #include <linux/namei.h>
>  #include <linux/ratelimit.h>
> @@ -720,6 +722,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path,
>  #define OVL_XATTR_UPPER_POSTFIX                "upper"
>  #define OVL_XATTR_METACOPY_POSTFIX     "metacopy"
>  #define OVL_XATTR_PROTATTR_POSTFIX     "protattr"
> +#define OVL_XATTR_VERITY_POSTFIX       "verity"
>
>  #define OVL_XATTR_TAB_ENTRY(x) \
>         [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
> @@ -734,6 +737,7 @@ const char *const ovl_xattr_table[][2] = {
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR),
> +       OVL_XATTR_TAB_ENTRY(OVL_XATTR_VERITY),
>  };
>
>  int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
> @@ -1166,6 +1170,98 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
>         return ERR_PTR(res);
>  }
>
> +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> +                        u8 *digest_buf, int *buf_length)
> +{
> +       int res;
> +
> +       res = ovl_path_getxattr(ofs, path, OVL_XATTR_VERITY, digest_buf, *buf_length);
> +       if (res == -ENODATA || res == -EOPNOTSUPP)
> +               return -ENODATA;
> +       if (res < 0) {
> +               pr_warn_ratelimited("failed to get digest (%i)\n", res);
> +               return res;
> +       }
> +
> +       *buf_length = res;
> +       return 0;
> +}
> +
> +/* Call with mounter creds as it may open the file */
> +static int ovl_ensure_verity_loaded(struct path *datapath)
> +{
> +       struct inode *inode = d_inode(datapath->dentry);
> +       const struct fsverity_info *vi;
> +       struct file *filp;
> +
> +       vi = fsverity_get_info(inode);
> +       if (vi == NULL && IS_VERITY(inode)) {
> +               /*
> +                * If this inode was not yet opened, the verity info hasn't been
> +                * loaded yet, so we need to do that here to force it into memory.
> +                * We use open_with_fake_path to avoid ENFILE.
> +                */
> +               filp = open_with_fake_path(datapath, O_RDONLY, inode, current_cred());
> +               if (IS_ERR(filp))
> +                       return PTR_ERR(filp);
> +               fput(filp);
> +       }
> +
> +       return 0;
> +}
> +
> +int ovl_validate_verity(struct ovl_fs *ofs,
> +                       struct path *metapath,
> +                       struct path *datapath)
> +{
> +       u8 xattr_data[1+FS_VERITY_MAX_DIGEST_SIZE];
> +       u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
> +       enum hash_algo verity_algo;
> +       int xattr_len;
> +       int err;
> +
> +       if (!ofs->config.verity ||
> +           /* Verity only works on regular files */
> +           !S_ISREG(d_inode(metapath->dentry)->i_mode))
> +               return 0;
> +
> +       xattr_len = sizeof(xattr_data);
> +       err = ovl_get_verity_xattr(ofs, metapath, xattr_data, &xattr_len);
> +       if (err == -ENODATA) {
> +               if (ofs->config.require_verity) {
> +                       pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n",
> +                                           metapath->dentry);
> +                       return -EIO;
> +               }
> +               return 0;
> +       }
> +       if (err < 0)
> +               return err;
> +
> +       err = ovl_ensure_verity_loaded(datapath);
> +       if (err < 0) {
> +               pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
> +                                   datapath->dentry);
> +               return -EIO;
> +       }
> +
> +       err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo);
> +       if (err < 0) {
> +               pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry);
> +               return -EIO;
> +       }
> +
> +       if (xattr_len != 1 + hash_digest_size[verity_algo] ||
> +           xattr_data[0] != (u8) verity_algo ||
> +           memcmp(xattr_data+1, actual_digest, xattr_len - 1) != 0) {
> +               pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> +                                   datapath->dentry);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * ovl_sync_status() - Check fs sync status for volatile mounts
>   *
> --
> 2.39.2
>

Re: [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Eric Biggers]

Hi Alexander,

On Wed, May 03, 2023 at 10:51:33AM +0200, Alexander Larsson wrote:
> This patchset adds support for using fs-verity to validate lowerdata
> files by specifying an overlay.verity xattr on the metacopy
> files.
> 
> This is primarily motivated by the Composefs usecase, where there will
> be a read-only EROFS layer that contains redirect into a base data
> layer which has fs-verity enabled on all files. However, it is also
> useful in general if you want to ensure that the lowerdata files
> matches the expected content over time.
> 
> This patch series is based on the lazy lowerdata patch series by Amir[1].
> 
> I have also added some tests for this feature to xfstests[2].
> 
> I'm also CC:ing the fsverity list and maintainers because there is one
> (tiny) fsverity change, and there may be interest in this usecase.
> 
> Changes since v1:
>  * Rebased on v2 lazy lowerdata series
>  * Dropped the "validate" mount option variant. We now only support
>    "off", "on" and "require", where "off" is the default.
>  * We now store the digest algorithm used in the overlay.verity xattr.
>  * Dropped ability to configure default verity options, as this could
>    cause problems moving layers between machines.
>  * We now properly resolve dependent mount options by automatically
>    enabling metacopy and redirect_dir if verity is on, or failing
>    if the specified options conflict.
>  * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
>  * Renamed new helpers from ovl_entry_path_ to ovl_e_path_
> 
> [1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
> [2] https://github.com/alexlarsson/xfstests/commits/verity-tests
> 
> Alexander Larsson (6):
>   fsverity: Export fsverity_get_digest
>   ovl: Break out ovl_e_path_real() from ovl_i_path_real()
>   ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
>   ovl: Add framework for verity support
>   ovl: Validate verity xattr when resolving lowerdata
>   ovl: Handle verity during copy-up
> 
>  Documentation/filesystems/overlayfs.rst |  27 ++++
>  fs/overlayfs/copy_up.c                  |  31 +++++
>  fs/overlayfs/namei.c                    |  42 +++++-
>  fs/overlayfs/overlayfs.h                |  12 ++
>  fs/overlayfs/ovl_entry.h                |   3 +
>  fs/overlayfs/super.c                    |  74 ++++++++++-
>  fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
>  fs/verity/measure.c                     |   1 +
>  8 files changed, 343 insertions(+), 12 deletions(-)

Thanks for presenting this topic at LSFMM!

I'm not an expert in overlayfs, but I've been working through this patchset.

One thing that seems to be missing, and has been tripping me up while reviewing
this patchset, is that the overlayfs documentation
(Documentation/filesystems/overlayfs.rst) is not properly up to date with the
use case that is intended here.

For example, the overlayfs documentation says "An overlay filesystem combines
two filesystems - an 'upper' filesystem and a 'lower' filesystem.".

Apparently, that is out of date.  I think a correct statement would be: An
overlay filesystem combines an optional upper directory with one or more lower
directories.

And as I understand it, the use case here actually involves two lower
directories and no upper directory.

There is also the "metacopy" feature, which the documentation describes in the
section "Metadata only copy up".  The documentation makes it sound like an
overlayfs internal optimization.

However, when this patchset introduces the fsverity support, it talks about
"metacopy files".  As I understand it, the user is expected to create a
read-only filesystem that contains these "metacopy files".  It doesn't seem to
be documented what these are, exactly, and how to create them.  I assume that
these are part of the implementation of "Metadata only copy up", but there seems
to be a gap where the documentation goes from describing the behavior of
"metadata only copy up", to expecting users of overlayfs to know what a
"metacopy file" is and how to create them.

I think that it would be easier to understand and review this feature if the
documentation was gotten up to date.

- Eric

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers]

On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> When resolving lowerdata (lazily or non-lazily) we check the
> overlay.verity xattr on the metadata inode, and if set verify that the
> source lowerdata inode matches it (according to the verity options
> enabled).

Keep in mind that the lifetime of an inode's fsverity digest is from when it is
first opened to when the inode is evicted from the inode cache.

If the inode gets evicted from cache and re-instantiated, it could have been
arbitrarily changed.

Given that, does this verification happen in the right place?  I would have
expected it to happen whenever the file is opened, but it seems you do it when
the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
explanation.

- Eric

Re: [PATCH v2 4/6] ovl: Add framework for verity support

[Eric Biggers]

On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote:
> +- "require":
> +    Same as "on", but additionally all metacopy files must specify a
> +    verity xattr. This means metadata copy up will only be used if
> +    the data file has fs-verity enabled, otherwise a full copy-up is
> +    used.

The second sentence makes it sound like an attacker can inject arbitrary data
just by replacing a data file with one that doesn't have fsverity enabled.

I really hope that's not the case?

I *think* there is a subtlety here involving "metacopy files" that were created
ahead of time by the user, vs. being generated by overlayfs.  But it's not
really explained.

- Eric

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > When resolving lowerdata (lazily or non-lazily) we check the
> > overlay.verity xattr on the metadata inode, and if set verify that the
> > source lowerdata inode matches it (according to the verity options
> > enabled).
>
> Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> first opened to when the inode is evicted from the inode cache.
>
> If the inode gets evicted from cache and re-instantiated, it could have been
> arbitrarily changed.
>
> Given that, does this verification happen in the right place?  I would have
> expected it to happen whenever the file is opened, but it seems you do it when
> the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> explanation.

Hmm. I do not think it is wrong because the overlay file cannot be opened before
the inode overlay is looked up and fsverity is verified on lookup.
In theory, overlay inode with lower could have been instantiated by decode_fh(),
but verity=on and nfs_export=on are conflicting options.

However, I agree that doing verify check on lookup is a bit too early, as
ls -lR will incur the overhead of verifying all file's data even
though their data
is not accessed in a non-lazy-lower-data scenario.

The intuition of doing verity check before file is opened (or copied up)
when there is a realfile open is not wrong, it would have gotten rid of the
dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
implement (not sure).

My suggestion for Alexander:
- Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
- Implement ovl_maybe_validate_verity() similar to
  ovl_maybe_lookup_lowerdata()
- Implement a helper ovl_verify_lowerdata()
  that calls them both
- Replace the ovl_maybe_lookup_lowerdata() calls with
  ovl_verify_lowerdata() calls

Then before opening (or copy up) a file, it could have either
lazy lower data lookup or lazy lower data validate or both (or none).

This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
just before it is needed and it is a bit easier to take ovl_inode_lock
unconditionally, in those call sites then deeper within copy_up, where
ovl_inode_lock is already taken.

I *think* this is a good idea, but we won't know until you try it,
so please take my suggestion with a grain of salt.

Thanks,
Amir.

Re: [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Amir Goldstein]

On Sun, May 14, 2023 at 10:09 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi Alexander,
>
> On Wed, May 03, 2023 at 10:51:33AM +0200, Alexander Larsson wrote:
> > This patchset adds support for using fs-verity to validate lowerdata
> > files by specifying an overlay.verity xattr on the metacopy
> > files.
> >
> > This is primarily motivated by the Composefs usecase, where there will
> > be a read-only EROFS layer that contains redirect into a base data
> > layer which has fs-verity enabled on all files. However, it is also
> > useful in general if you want to ensure that the lowerdata files
> > matches the expected content over time.
> >
> > This patch series is based on the lazy lowerdata patch series by Amir[1].
> >
> > I have also added some tests for this feature to xfstests[2].
> >
> > I'm also CC:ing the fsverity list and maintainers because there is one
> > (tiny) fsverity change, and there may be interest in this usecase.
> >
> > Changes since v1:
> >  * Rebased on v2 lazy lowerdata series
> >  * Dropped the "validate" mount option variant. We now only support
> >    "off", "on" and "require", where "off" is the default.
> >  * We now store the digest algorithm used in the overlay.verity xattr.
> >  * Dropped ability to configure default verity options, as this could
> >    cause problems moving layers between machines.
> >  * We now properly resolve dependent mount options by automatically
> >    enabling metacopy and redirect_dir if verity is on, or failing
> >    if the specified options conflict.
> >  * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
> >  * Renamed new helpers from ovl_entry_path_ to ovl_e_path_
> >
> > [1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
> > [2] https://github.com/alexlarsson/xfstests/commits/verity-tests
> >
> > Alexander Larsson (6):
> >   fsverity: Export fsverity_get_digest
> >   ovl: Break out ovl_e_path_real() from ovl_i_path_real()
> >   ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
> >   ovl: Add framework for verity support
> >   ovl: Validate verity xattr when resolving lowerdata
> >   ovl: Handle verity during copy-up
> >
> >  Documentation/filesystems/overlayfs.rst |  27 ++++
> >  fs/overlayfs/copy_up.c                  |  31 +++++
> >  fs/overlayfs/namei.c                    |  42 +++++-
> >  fs/overlayfs/overlayfs.h                |  12 ++
> >  fs/overlayfs/ovl_entry.h                |   3 +
> >  fs/overlayfs/super.c                    |  74 ++++++++++-
> >  fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
> >  fs/verity/measure.c                     |   1 +
> >  8 files changed, 343 insertions(+), 12 deletions(-)
>
> Thanks for presenting this topic at LSFMM!
>
> I'm not an expert in overlayfs, but I've been working through this patchset.
>
> One thing that seems to be missing, and has been tripping me up while reviewing
> this patchset, is that the overlayfs documentation
> (Documentation/filesystems/overlayfs.rst) is not properly up to date with the
> use case that is intended here.
>
> For example, the overlayfs documentation says "An overlay filesystem combines
> two filesystems - an 'upper' filesystem and a 'lower' filesystem.".
>
> Apparently, that is out of date.  I think a correct statement would be: An
> overlay filesystem combines an optional upper directory with one or more lower
> directories.
>
> And as I understand it, the use case here actually involves two lower
> directories and no upper directory.
>
> There is also the "metacopy" feature, which the documentation describes in the
> section "Metadata only copy up".  The documentation makes it sound like an
> overlayfs internal optimization.
>
> However, when this patchset introduces the fsverity support, it talks about
> "metacopy files".  As I understand it, the user is expected to create a
> read-only filesystem that contains these "metacopy files".  It doesn't seem to
> be documented what these are, exactly, and how to create them.  I assume that
> these are part of the implementation of "Metadata only copy up", but there seems
> to be a gap where the documentation goes from describing the behavior of
> "metadata only copy up", to expecting users of overlayfs to know what a
> "metacopy file" is and how to create them.
>

What may confuse you is that Alexander has a tool mkfs.composefs
that creates hand crafted overlayfs, but it is not up to overlayfs.rst to
document this tool.

This patch set and documentation of the feature are standalone to
overlayfs and they affect and explain standard user workloads.

A "metacopy file" is one that has metadata changes in upper layer
but the data is still read from the lower layer.

The classic example, for which metacopy feature was developed is
chwon -R on a mounted overlay without having to copy up the data.

I agree that it would be good to clarify this term before using it.

With mount options metacopy=on,verity=on, once a file's metadata
was copied up, and its owner changed for the sake of the story, the
lower data file cannot be replaced with another file with different data
or without fsverity, while overlayfs is offline.

> I think that it would be easier to understand and review this feature if the
> documentation was gotten up to date.
>

Those are all valid comments, but I don't think it would be fair to
require Alexander to fix the out of date documentation of overlayfs.

IMO, the documentation of verity feature should be judged for its
own clarity (and I myself find it pretty clear) assuming that the reader
is familiar with overlayfs and metacopy, but I will surely not object if
Alexander wants to help improve overlayfs.rst.

Thanks,
Amir.

Re: [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Gao Xiang]

Hi,

On 2023/5/15 14:25, Amir Goldstein wrote:
> On Sun, May 14, 2023 at 10:09 PM Eric Biggers <ebiggers@kernel.org> wrote:
>>
>> Hi Alexander,
>>
>> On Wed, May 03, 2023 at 10:51:33AM +0200, Alexander Larsson wrote:
>>> This patchset adds support for using fs-verity to validate lowerdata
>>> files by specifying an overlay.verity xattr on the metacopy
>>> files.
>>>
>>> This is primarily motivated by the Composefs usecase, where there will
>>> be a read-only EROFS layer that contains redirect into a base data
>>> layer which has fs-verity enabled on all files. However, it is also
>>> useful in general if you want to ensure that the lowerdata files
>>> matches the expected content over time.
>>>
>>> This patch series is based on the lazy lowerdata patch series by Amir[1].
>>>
>>> I have also added some tests for this feature to xfstests[2].
>>>
>>> I'm also CC:ing the fsverity list and maintainers because there is one
>>> (tiny) fsverity change, and there may be interest in this usecase.
>>>
>>> Changes since v1:
>>>   * Rebased on v2 lazy lowerdata series
>>>   * Dropped the "validate" mount option variant. We now only support
>>>     "off", "on" and "require", where "off" is the default.
>>>   * We now store the digest algorithm used in the overlay.verity xattr.
>>>   * Dropped ability to configure default verity options, as this could
>>>     cause problems moving layers between machines.
>>>   * We now properly resolve dependent mount options by automatically
>>>     enabling metacopy and redirect_dir if verity is on, or failing
>>>     if the specified options conflict.
>>>   * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
>>>   * Renamed new helpers from ovl_entry_path_ to ovl_e_path_
>>>
>>> [1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
>>> [2] https://github.com/alexlarsson/xfstests/commits/verity-tests
>>>
>>> Alexander Larsson (6):
>>>    fsverity: Export fsverity_get_digest
>>>    ovl: Break out ovl_e_path_real() from ovl_i_path_real()
>>>    ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
>>>    ovl: Add framework for verity support
>>>    ovl: Validate verity xattr when resolving lowerdata
>>>    ovl: Handle verity during copy-up
>>>
>>>   Documentation/filesystems/overlayfs.rst |  27 ++++
>>>   fs/overlayfs/copy_up.c                  |  31 +++++
>>>   fs/overlayfs/namei.c                    |  42 +++++-
>>>   fs/overlayfs/overlayfs.h                |  12 ++
>>>   fs/overlayfs/ovl_entry.h                |   3 +
>>>   fs/overlayfs/super.c                    |  74 ++++++++++-
>>>   fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
>>>   fs/verity/measure.c                     |   1 +
>>>   8 files changed, 343 insertions(+), 12 deletions(-)
>>
>> Thanks for presenting this topic at LSFMM!
>>
>> I'm not an expert in overlayfs, but I've been working through this patchset.
>>
>> One thing that seems to be missing, and has been tripping me up while reviewing
>> this patchset, is that the overlayfs documentation
>> (Documentation/filesystems/overlayfs.rst) is not properly up to date with the
>> use case that is intended here.
>>
>> For example, the overlayfs documentation says "An overlay filesystem combines
>> two filesystems - an 'upper' filesystem and a 'lower' filesystem.".
>>
>> Apparently, that is out of date.  I think a correct statement would be: An
>> overlay filesystem combines an optional upper directory with one or more lower
>> directories.
>>
>> And as I understand it, the use case here actually involves two lower
>> directories and no upper directory.
>>
>> There is also the "metacopy" feature, which the documentation describes in the
>> section "Metadata only copy up".  The documentation makes it sound like an
>> overlayfs internal optimization.
>>
>> However, when this patchset introduces the fsverity support, it talks about
>> "metacopy files".  As I understand it, the user is expected to create a
>> read-only filesystem that contains these "metacopy files".  It doesn't seem to
>> be documented what these are, exactly, and how to create them.  I assume that
>> these are part of the implementation of "Metadata only copy up", but there seems
>> to be a gap where the documentation goes from describing the behavior of
>> "metadata only copy up", to expecting users of overlayfs to know what a
>> "metacopy file" is and how to create them.
>>
> 
> What may confuse you is that Alexander has a tool mkfs.composefs
> that creates hand crafted overlayfs, but it is not up to overlayfs.rst to
> document this tool.

Although I also think it might be inappropriate to document the whole
mkfs.composefs tool in overlayfs.rst, I'm not sure if it could be helpful
to document (or mention) some other typical attractive use cases other than
the primary one ("present a filesystem which redirects writes to the upper
layer?") so people could easily find/use/develop such additional use cases
as well?

That could be in a seperate work though.

Thanks,
Gao Xiang

Re: [PATCH v2 4/6] ovl: Add framework for verity support

[Alexander Larsson]

On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote:
> > +- "require":
> > +    Same as "on", but additionally all metacopy files must specify a
> > +    verity xattr. This means metadata copy up will only be used if
> > +    the data file has fs-verity enabled, otherwise a full copy-up is
> > +    used.
>
> The second sentence makes it sound like an attacker can inject arbitrary data
> just by replacing a data file with one that doesn't have fsverity enabled.
>
> I really hope that's not the case?
>
> I *think* there is a subtlety here involving "metacopy files" that were created
> ahead of time by the user, vs. being generated by overlayfs.  But it's not
> really explained.

I'm not sure what you mean here? When you say "replacing a data file",
do you mean "changing the content of the lowerdir"? Because if you can
just change lowerdir content then you can make users of the overlayfs
mount read whatever data you want (independent of metacopy or any of
this).

The second sentence above relates to this case:

* A lower dir file "lower/a" does not have fsverity enabled.
* Someone does "chown mnt/a"
* This causes overlayfs to make a "copy up" operation of the "lower/a"
to "upper/a"
* But, we can't use the optimized "copy metacopy only" version of
"copy up", because we could then not specify a digest xattr on the new
file, so we copy the content of "lower/a" to "upper/a".

What exactly is your worry in this case?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Alexander Larsson]

On Sun, May 14, 2023 at 11:25 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, May 14, 2023 at 10:09 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > Hi Alexander,
> >
> > On Wed, May 03, 2023 at 10:51:33AM +0200, Alexander Larsson wrote:
> > > This patchset adds support for using fs-verity to validate lowerdata
> > > files by specifying an overlay.verity xattr on the metacopy
> > > files.
> > >
> > > This is primarily motivated by the Composefs usecase, where there will
> > > be a read-only EROFS layer that contains redirect into a base data
> > > layer which has fs-verity enabled on all files. However, it is also
> > > useful in general if you want to ensure that the lowerdata files
> > > matches the expected content over time.
> > >
> > > This patch series is based on the lazy lowerdata patch series by Amir[1].
> > >
> > > I have also added some tests for this feature to xfstests[2].
> > >
> > > I'm also CC:ing the fsverity list and maintainers because there is one
> > > (tiny) fsverity change, and there may be interest in this usecase.
> > >
> > > Changes since v1:
> > >  * Rebased on v2 lazy lowerdata series
> > >  * Dropped the "validate" mount option variant. We now only support
> > >    "off", "on" and "require", where "off" is the default.
> > >  * We now store the digest algorithm used in the overlay.verity xattr.
> > >  * Dropped ability to configure default verity options, as this could
> > >    cause problems moving layers between machines.
> > >  * We now properly resolve dependent mount options by automatically
> > >    enabling metacopy and redirect_dir if verity is on, or failing
> > >    if the specified options conflict.
> > >  * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
> > >  * Renamed new helpers from ovl_entry_path_ to ovl_e_path_
> > >
> > > [1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
> > > [2] https://github.com/alexlarsson/xfstests/commits/verity-tests
> > >
> > > Alexander Larsson (6):
> > >   fsverity: Export fsverity_get_digest
> > >   ovl: Break out ovl_e_path_real() from ovl_i_path_real()
> > >   ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
> > >   ovl: Add framework for verity support
> > >   ovl: Validate verity xattr when resolving lowerdata
> > >   ovl: Handle verity during copy-up
> > >
> > >  Documentation/filesystems/overlayfs.rst |  27 ++++
> > >  fs/overlayfs/copy_up.c                  |  31 +++++
> > >  fs/overlayfs/namei.c                    |  42 +++++-
> > >  fs/overlayfs/overlayfs.h                |  12 ++
> > >  fs/overlayfs/ovl_entry.h                |   3 +
> > >  fs/overlayfs/super.c                    |  74 ++++++++++-
> > >  fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
> > >  fs/verity/measure.c                     |   1 +
> > >  8 files changed, 343 insertions(+), 12 deletions(-)
> >
> > Thanks for presenting this topic at LSFMM!
> >
> > I'm not an expert in overlayfs, but I've been working through this patchset.
> >
> > One thing that seems to be missing, and has been tripping me up while reviewing
> > this patchset, is that the overlayfs documentation
> > (Documentation/filesystems/overlayfs.rst) is not properly up to date with the
> > use case that is intended here.
> >
> > For example, the overlayfs documentation says "An overlay filesystem combines
> > two filesystems - an 'upper' filesystem and a 'lower' filesystem.".
> >
> > Apparently, that is out of date.  I think a correct statement would be: An
> > overlay filesystem combines an optional upper directory with one or more lower
> > directories.
> >
> > And as I understand it, the use case here actually involves two lower
> > directories and no upper directory.
> >
> > There is also the "metacopy" feature, which the documentation describes in the
> > section "Metadata only copy up".  The documentation makes it sound like an
> > overlayfs internal optimization.
> >
> > However, when this patchset introduces the fsverity support, it talks about
> > "metacopy files".  As I understand it, the user is expected to create a
> > read-only filesystem that contains these "metacopy files".  It doesn't seem to
> > be documented what these are, exactly, and how to create them.  I assume that
> > these are part of the implementation of "Metadata only copy up", but there seems
> > to be a gap where the documentation goes from describing the behavior of
> > "metadata only copy up", to expecting users of overlayfs to know what a
> > "metacopy file" is and how to create them.
> >
>
> What may confuse you is that Alexander has a tool mkfs.composefs
> that creates hand crafted overlayfs, but it is not up to overlayfs.rst to
> document this tool.
>
> This patch set and documentation of the feature are standalone to
> overlayfs and they affect and explain standard user workloads.

Yeah, the documentation as part of the patch really just describes the
new options as they relate to how the kernel uses and produces these
new xattrs. I.e. as if you're using the new verity support only as a
way to make the meta-copy-up feature more robust by having the copy-up
operation record the digest of the source.

The way composefs uses the various features of overlayfs (metacopy,
redirect, verity, read-only-image) to combine into the "composefs"
feature isn't really described

> A "metacopy file" is one that has metadata changes in upper layer
> but the data is still read from the lower layer.

To be more specific, it is a sparse file in any layer other than the
lowermost lowerdir that has the "overlay.metacopy" xattr set. When
this file is accessed from the overlay mount, all the metadata is
taken from the file, but the content is taken from the similarly named
file in a lower layer.

In addition, an overlay.redirect xattr can be set on the file. This
allows specifying a separate filename for the content file in the
lower layer.

> The classic example, for which metacopy feature was developed is
> chwon -R on a mounted overlay without having to copy up the data.
>
> I agree that it would be good to clarify this term before using it.
>
> With mount options metacopy=on,verity=on, once a file's metadata
> was copied up, and its owner changed for the sake of the story, the
> lower data file cannot be replaced with another file with different data
> or without fsverity, while overlayfs is offline.
>
> > I think that it would be easier to understand and review this feature if the
> > documentation was gotten up to date.
> >
>
> Those are all valid comments, but I don't think it would be fair to
> require Alexander to fix the out of date documentation of overlayfs.
>
> IMO, the documentation of verity feature should be judged for its
> own clarity (and I myself find it pretty clear) assuming that the reader
> is familiar with overlayfs and metacopy, but I will surely not object if
> Alexander wants to help improve overlayfs.rst.
>
> Thanks,
> Amir.
>


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 4/6] ovl: Add framework for verity support

[Eric Biggers]

On Mon, May 15, 2023 at 07:44:13AM +0200, Alexander Larsson wrote:
> On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote:
> > > +- "require":
> > > +    Same as "on", but additionally all metacopy files must specify a
> > > +    verity xattr. This means metadata copy up will only be used if
> > > +    the data file has fs-verity enabled, otherwise a full copy-up is
> > > +    used.
> >
> > The second sentence makes it sound like an attacker can inject arbitrary data
> > just by replacing a data file with one that doesn't have fsverity enabled.
> >
> > I really hope that's not the case?
> >
> > I *think* there is a subtlety here involving "metacopy files" that were created
> > ahead of time by the user, vs. being generated by overlayfs.  But it's not
> > really explained.
> 
> I'm not sure what you mean here? When you say "replacing a data file",
> do you mean "changing the content of the lowerdir"?

Yes.  Specifically the data-only lowerdir.

> Because if you can just change lowerdir content then you can make users of the
> overlayfs mount read whatever data you want (independent of metacopy or any of
> this).

But isn't preventing that the whole point of your feature?

What am I missing?

- Eric

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Sun, May 14, 2023 at 9:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > When resolving lowerdata (lazily or non-lazily) we check the
> > overlay.verity xattr on the metadata inode, and if set verify that the
> > source lowerdata inode matches it (according to the verity options
> > enabled).
>
> Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> first opened to when the inode is evicted from the inode cache.
>
> If the inode gets evicted from cache and re-instantiated, it could have been
> arbitrarily changed.
>
> Given that, does this verification happen in the right place?  I would have
> expected it to happen whenever the file is opened, but it seems you do it when
> the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> explanation.

The overlayfs inode will, after lookup, keep a reference to the dentry
(and thus inode) of the lower file, until such a time that the overlay
inode is evicted from the cache. This will keep the fsverity digest on
the lower alive while the overlay inode is alive. If the overlay inode
is evicted, then we will re-validate the verity on lookup().

As amir mentioned, this may not be optimal, and it may be beneficial
to sometimes delay the digest validation, but that is more of a
performance detail.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > When resolving lowerdata (lazily or non-lazily) we check the
> > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > source lowerdata inode matches it (according to the verity options
> > > enabled).
> >
> > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > first opened to when the inode is evicted from the inode cache.
> >
> > If the inode gets evicted from cache and re-instantiated, it could have been
> > arbitrarily changed.
> >
> > Given that, does this verification happen in the right place?  I would have
> > expected it to happen whenever the file is opened, but it seems you do it when
> > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > explanation.
>
> Hmm. I do not think it is wrong because the overlay file cannot be opened before
> the inode overlay is looked up and fsverity is verified on lookup.
> In theory, overlay inode with lower could have been instantiated by decode_fh(),
> but verity=on and nfs_export=on are conflicting options.
>
> However, I agree that doing verify check on lookup is a bit too early, as
> ls -lR will incur the overhead of verifying all file's data even
> though their data
> is not accessed in a non-lazy-lower-data scenario.
>
> The intuition of doing verity check before file is opened (or copied up)
> when there is a realfile open is not wrong, it would have gotten rid of the
> dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> implement (not sure).
>
> My suggestion for Alexander:
> - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> - Implement ovl_maybe_validate_verity() similar to
>   ovl_maybe_lookup_lowerdata()
> - Implement a helper ovl_verify_lowerdata()
>   that calls them both
> - Replace the ovl_maybe_lookup_lowerdata() calls with
>   ovl_verify_lowerdata() calls
>
> Then before opening (or copy up) a file, it could have either
> lazy lower data lookup or lazy lower data validate or both (or none).
>
> This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> just before it is needed and it is a bit easier to take ovl_inode_lock
> unconditionally, in those call sites then deeper within copy_up, where
> ovl_inode_lock is already taken.
>
> I *think* this is a good idea, but we won't know until you try it,
> so please take my suggestion with a grain of salt.

I'll have a look at it in a bit. It would make performance of
verity=on in the non-lazy-lookup case better.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 4/6] ovl: Add framework for verity support

[Alexander Larsson]

On Mon, May 15, 2023 at 8:00 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, May 15, 2023 at 07:44:13AM +0200, Alexander Larsson wrote:
> > On Sun, May 14, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Wed, May 03, 2023 at 10:51:37AM +0200, Alexander Larsson wrote:
> > > > +- "require":
> > > > +    Same as "on", but additionally all metacopy files must specify a
> > > > +    verity xattr. This means metadata copy up will only be used if
> > > > +    the data file has fs-verity enabled, otherwise a full copy-up is
> > > > +    used.
> > >
> > > The second sentence makes it sound like an attacker can inject arbitrary data
> > > just by replacing a data file with one that doesn't have fsverity enabled.
> > >
> > > I really hope that's not the case?
> > >
> > > I *think* there is a subtlety here involving "metacopy files" that were created
> > > ahead of time by the user, vs. being generated by overlayfs.  But it's not
> > > really explained.
> >
> > I'm not sure what you mean here? When you say "replacing a data file",
> > do you mean "changing the content of the lowerdir"?
>
> Yes.  Specifically the data-only lowerdir.
>
> > Because if you can just change lowerdir content then you can make users of the
> > overlayfs mount read whatever data you want (independent of metacopy or any of
> > this).
>
> But isn't preventing that the whole point of your feature?
>
> What am I missing?

If by "your feature", you mean the full composefs idea, then things
are different. In this case the root mount is an overlayfs with two
lowerdirs and no upper. The uppermost lower is a read-only erofs mount
with metadata and overlay xattrs for all files, and the lowermost one
is a data-only dir. You cannot modify the uppermost layer (since it's
a fsverity readonly erofs file). If you modify a data-only file it
will be detected at validation, because it can only be reached from
redirects in the image file, and we ensured all image files specified
a digest. There is no upper dir, so the overlayfs mount itself is not
writable, and thus there can be any meta-copy-up operations triggered
at all.

However, even if you had an upper dir ("writable composefs" usecase) I
don't see a problem. If you trigger a full (i.e. non-metadata) copy-up
then the copy-up operation itself will validate the data from the
lower at copy time (due to the verity xattr on the "middle layer").


I think again there is some confusion due to the two kinds of usecases
for the verity support. The basic usecase is just for general
robustness. I.e it allows the kernel to record information about the
original state of the content file when generating a metacopy
reference to it. If you later accidentally mount the overlayfs against
the wrong lower version you will get told about it. The second usecase
is the tamper proofing image case (composefs), and its security
depends on more details (i.e. the readonly image part and its
construction).

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > source lowerdata inode matches it (according to the verity options
> > > > enabled).
> > >
> > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > first opened to when the inode is evicted from the inode cache.
> > >
> > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > arbitrarily changed.
> > >
> > > Given that, does this verification happen in the right place?  I would have
> > > expected it to happen whenever the file is opened, but it seems you do it when
> > > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > > explanation.
> >
> > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > the inode overlay is looked up and fsverity is verified on lookup.
> > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > but verity=on and nfs_export=on are conflicting options.
> >
> > However, I agree that doing verify check on lookup is a bit too early, as
> > ls -lR will incur the overhead of verifying all file's data even
> > though their data
> > is not accessed in a non-lazy-lower-data scenario.
> >
> > The intuition of doing verity check before file is opened (or copied up)
> > when there is a realfile open is not wrong, it would have gotten rid of the
> > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > implement (not sure).
> >
> > My suggestion for Alexander:
> > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > - Implement ovl_maybe_validate_verity() similar to
> >   ovl_maybe_lookup_lowerdata()
> > - Implement a helper ovl_verify_lowerdata()
> >   that calls them both
> > - Replace the ovl_maybe_lookup_lowerdata() calls with
> >   ovl_verify_lowerdata() calls
> >
> > Then before opening (or copy up) a file, it could have either
> > lazy lower data lookup or lazy lower data validate or both (or none).
> >
> > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > just before it is needed and it is a bit easier to take ovl_inode_lock
> > unconditionally, in those call sites then deeper within copy_up, where
> > ovl_inode_lock is already taken.
> >
> > I *think* this is a good idea, but we won't know until you try it,
> > so please take my suggestion with a grain of salt.
>
> I'll have a look at it in a bit. It would make performance of
> verity=on in the non-lazy-lookup case better.
>

Hi Alex,

Now that lazy lookup is queued for next, I wanted to ask about the
status of your patches.

Is the issue above the only thing you still need to look at?
No rush on my end, just wanted to be in sync.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Fri, Jun 9, 2023 at 3:03 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > > source lowerdata inode matches it (according to the verity options
> > > > > enabled).
> > > >
> > > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > > first opened to when the inode is evicted from the inode cache.
> > > >
> > > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > > arbitrarily changed.
> > > >
> > > > Given that, does this verification happen in the right place?  I would have
> > > > expected it to happen whenever the file is opened, but it seems you do it when
> > > > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > > > explanation.
> > >
> > > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > > the inode overlay is looked up and fsverity is verified on lookup.
> > > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > > but verity=on and nfs_export=on are conflicting options.
> > >
> > > However, I agree that doing verify check on lookup is a bit too early, as
> > > ls -lR will incur the overhead of verifying all file's data even
> > > though their data
> > > is not accessed in a non-lazy-lower-data scenario.
> > >
> > > The intuition of doing verity check before file is opened (or copied up)
> > > when there is a realfile open is not wrong, it would have gotten rid of the
> > > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > > implement (not sure).
> > >
> > > My suggestion for Alexander:
> > > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > > - Implement ovl_maybe_validate_verity() similar to
> > >   ovl_maybe_lookup_lowerdata()
> > > - Implement a helper ovl_verify_lowerdata()
> > >   that calls them both
> > > - Replace the ovl_maybe_lookup_lowerdata() calls with
> > >   ovl_verify_lowerdata() calls
> > >
> > > Then before opening (or copy up) a file, it could have either
> > > lazy lower data lookup or lazy lower data validate or both (or none).
> > >
> > > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > > just before it is needed and it is a bit easier to take ovl_inode_lock
> > > unconditionally, in those call sites then deeper within copy_up, where
> > > ovl_inode_lock is already taken.
> > >
> > > I *think* this is a good idea, but we won't know until you try it,
> > > so please take my suggestion with a grain of salt.
> >
> > I'll have a look at it in a bit. It would make performance of
> > verity=on in the non-lazy-lookup case better.
> >
>
> Hi Alex,
>
> Now that lazy lookup is queued for next, I wanted to ask about the
> status of your patches.
>
> Is the issue above the only thing you still need to look at?
> No rush on my end, just wanted to be in sync.

I spent some time on friday doing the initial work on this rework. In
case you want to look at it  I just pushed it to:

https://github.com/alexlarsson/linux/tree/overlay-verity

The first 3 commits are the same as before, and the last one is the
new approach. It's turned out pretty nice, simplifying things
considerable. But I really haven't had time to do a full re-review and
test of it, so please don't merge it yet. I'll spend Monday ensuring
it is in a good state for upstream.

Also, for the people following at home, ostree master has now landed
initial work for using composefs images for the root filesystem. The
initial work with minimal changes to ostree, but long term we are also
considering a complete rework of ostree based 100% on composefs.
Interested parties can follow this here:

 https://github.com/ostreedev/ostree/issues/2867

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Sat, Jun 10, 2023 at 6:02 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Fri, Jun 9, 2023 at 3:03 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > > > source lowerdata inode matches it (according to the verity options
> > > > > > enabled).
> > > > >
> > > > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > > > first opened to when the inode is evicted from the inode cache.
> > > > >
> > > > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > > > arbitrarily changed.
> > > > >
> > > > > Given that, does this verification happen in the right place?  I would have
> > > > > expected it to happen whenever the file is opened, but it seems you do it when
> > > > > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > > > > explanation.
> > > >
> > > > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > > > the inode overlay is looked up and fsverity is verified on lookup.
> > > > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > > > but verity=on and nfs_export=on are conflicting options.
> > > >
> > > > However, I agree that doing verify check on lookup is a bit too early, as
> > > > ls -lR will incur the overhead of verifying all file's data even
> > > > though their data
> > > > is not accessed in a non-lazy-lower-data scenario.
> > > >
> > > > The intuition of doing verity check before file is opened (or copied up)
> > > > when there is a realfile open is not wrong, it would have gotten rid of the
> > > > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > > > implement (not sure).
> > > >
> > > > My suggestion for Alexander:
> > > > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > > > - Implement ovl_maybe_validate_verity() similar to
> > > >   ovl_maybe_lookup_lowerdata()
> > > > - Implement a helper ovl_verify_lowerdata()
> > > >   that calls them both
> > > > - Replace the ovl_maybe_lookup_lowerdata() calls with
> > > >   ovl_verify_lowerdata() calls
> > > >
> > > > Then before opening (or copy up) a file, it could have either
> > > > lazy lower data lookup or lazy lower data validate or both (or none).
> > > >
> > > > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > > > just before it is needed and it is a bit easier to take ovl_inode_lock
> > > > unconditionally, in those call sites then deeper within copy_up, where
> > > > ovl_inode_lock is already taken.
> > > >
> > > > I *think* this is a good idea, but we won't know until you try it,
> > > > so please take my suggestion with a grain of salt.
> > >
> > > I'll have a look at it in a bit. It would make performance of
> > > verity=on in the non-lazy-lookup case better.
> > >
> >
> > Hi Alex,
> >
> > Now that lazy lookup is queued for next, I wanted to ask about the
> > status of your patches.
> >
> > Is the issue above the only thing you still need to look at?
> > No rush on my end, just wanted to be in sync.
>
> I spent some time on friday doing the initial work on this rework. In
> case you want to look at it  I just pushed it to:
>
> https://github.com/alexlarsson/linux/tree/overlay-verity
>
> The first 3 commits are the same as before, and the last one is the
> new approach. It's turned out pretty nice, simplifying things
> considerable.

It does look nicer :)
I gave you a small review comment on github, but overall looks good.

> But I really haven't had time to do a full re-review and
> test of it, so please don't merge it yet. I'll spend Monday ensuring
> it is in a good state for upstream.
>

When you are done testing, please post v3 patches.

Note that I pushed a new version of ovl-lazy-lowerdata branch to
fstests, with the minor :: syntax change.

Not sure if Miklos will have time to review them this cycle or wait
for the next one.

There are also the mount api changes from Christian that conflict
with the new verify option.

If we want to have all of these changes for this cycle, some collaboration
will be required.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Sun, Jun 11, 2023 at 1:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Jun 10, 2023 at 6:02 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Fri, Jun 9, 2023 at 3:03 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
> > > >
> > > > On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > > > > source lowerdata inode matches it (according to the verity options
> > > > > > > enabled).
> > > > > >
> > > > > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > > > > first opened to when the inode is evicted from the inode cache.
> > > > > >
> > > > > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > > > > arbitrarily changed.
> > > > > >
> > > > > > Given that, does this verification happen in the right place?  I would have
> > > > > > expected it to happen whenever the file is opened, but it seems you do it when
> > > > > > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > > > > > explanation.
> > > > >
> > > > > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > > > > the inode overlay is looked up and fsverity is verified on lookup.
> > > > > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > > > > but verity=on and nfs_export=on are conflicting options.
> > > > >
> > > > > However, I agree that doing verify check on lookup is a bit too early, as
> > > > > ls -lR will incur the overhead of verifying all file's data even
> > > > > though their data
> > > > > is not accessed in a non-lazy-lower-data scenario.
> > > > >
> > > > > The intuition of doing verity check before file is opened (or copied up)
> > > > > when there is a realfile open is not wrong, it would have gotten rid of the
> > > > > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > > > > implement (not sure).
> > > > >
> > > > > My suggestion for Alexander:
> > > > > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > > > > - Implement ovl_maybe_validate_verity() similar to
> > > > >   ovl_maybe_lookup_lowerdata()
> > > > > - Implement a helper ovl_verify_lowerdata()
> > > > >   that calls them both
> > > > > - Replace the ovl_maybe_lookup_lowerdata() calls with
> > > > >   ovl_verify_lowerdata() calls
> > > > >
> > > > > Then before opening (or copy up) a file, it could have either
> > > > > lazy lower data lookup or lazy lower data validate or both (or none).
> > > > >
> > > > > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > > > > just before it is needed and it is a bit easier to take ovl_inode_lock
> > > > > unconditionally, in those call sites then deeper within copy_up, where
> > > > > ovl_inode_lock is already taken.
> > > > >
> > > > > I *think* this is a good idea, but we won't know until you try it,
> > > > > so please take my suggestion with a grain of salt.
> > > >
> > > > I'll have a look at it in a bit. It would make performance of
> > > > verity=on in the non-lazy-lookup case better.
> > > >
> > >
> > > Hi Alex,
> > >
> > > Now that lazy lookup is queued for next, I wanted to ask about the
> > > status of your patches.
> > >
> > > Is the issue above the only thing you still need to look at?
> > > No rush on my end, just wanted to be in sync.
> >
> > I spent some time on friday doing the initial work on this rework. In
> > case you want to look at it  I just pushed it to:
> >
> > https://github.com/alexlarsson/linux/tree/overlay-verity
> >
> > The first 3 commits are the same as before, and the last one is the
> > new approach. It's turned out pretty nice, simplifying things
> > considerable.
>
> It does look nicer :)
> I gave you a small review comment on github, but overall looks good.
>
> > But I really haven't had time to do a full re-review and
> > test of it, so please don't merge it yet. I'll spend Monday ensuring
> > it is in a good state for upstream.
> >
>
> When you are done testing, please post v3 patches.
>
> Note that I pushed a new version of ovl-lazy-lowerdata branch to
> fstests, with the minor :: syntax change.

I posted the patches, and I also had to make some small changes to the
xfstest commit:

https://github.com/alexlarsson/xfstests/commits/verity-tests

> Not sure if Miklos will have time to review them this cycle or wait
> for the next one.
>
> There are also the mount api changes from Christian that conflict
> with the new verify option.
>
> If we want to have all of these changes for this cycle, some collaboration
> will be required.

I would really like to get the overlay.verity xattr support in
upstream pretty soon, because without it I can't release a stable
version of the composefs userspace code. I don't want to release
something that is using an xattr format that is not guaranteed to be
stable.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Mon, Jun 12, 2023 at 1:33 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Sun, Jun 11, 2023 at 1:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Sat, Jun 10, 2023 at 6:02 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Fri, Jun 9, 2023 at 3:03 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
> > > > >
> > > > > On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > > > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > > > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > > > > > source lowerdata inode matches it (according to the verity options
> > > > > > > > enabled).
> > > > > > >
> > > > > > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > > > > > first opened to when the inode is evicted from the inode cache.
> > > > > > >
> > > > > > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > > > > > arbitrarily changed.
> > > > > > >
> > > > > > > Given that, does this verification happen in the right place?  I would have
> > > > > > > expected it to happen whenever the file is opened, but it seems you do it when
> > > > > > > the dentry is looked up instead.  Maybe that works too, but I'd appreciate an
> > > > > > > explanation.
> > > > > >
> > > > > > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > > > > > the inode overlay is looked up and fsverity is verified on lookup.
> > > > > > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > > > > > but verity=on and nfs_export=on are conflicting options.
> > > > > >
> > > > > > However, I agree that doing verify check on lookup is a bit too early, as
> > > > > > ls -lR will incur the overhead of verifying all file's data even
> > > > > > though their data
> > > > > > is not accessed in a non-lazy-lower-data scenario.
> > > > > >
> > > > > > The intuition of doing verity check before file is opened (or copied up)
> > > > > > when there is a realfile open is not wrong, it would have gotten rid of the
> > > > > > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > > > > > implement (not sure).
> > > > > >
> > > > > > My suggestion for Alexander:
> > > > > > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > > > > > - Implement ovl_maybe_validate_verity() similar to
> > > > > >   ovl_maybe_lookup_lowerdata()
> > > > > > - Implement a helper ovl_verify_lowerdata()
> > > > > >   that calls them both
> > > > > > - Replace the ovl_maybe_lookup_lowerdata() calls with
> > > > > >   ovl_verify_lowerdata() calls
> > > > > >
> > > > > > Then before opening (or copy up) a file, it could have either
> > > > > > lazy lower data lookup or lazy lower data validate or both (or none).
> > > > > >
> > > > > > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > > > > > just before it is needed and it is a bit easier to take ovl_inode_lock
> > > > > > unconditionally, in those call sites then deeper within copy_up, where
> > > > > > ovl_inode_lock is already taken.
> > > > > >
> > > > > > I *think* this is a good idea, but we won't know until you try it,
> > > > > > so please take my suggestion with a grain of salt.
> > > > >
> > > > > I'll have a look at it in a bit. It would make performance of
> > > > > verity=on in the non-lazy-lookup case better.
> > > > >
> > > >
> > > > Hi Alex,
> > > >
> > > > Now that lazy lookup is queued for next, I wanted to ask about the
> > > > status of your patches.
> > > >
> > > > Is the issue above the only thing you still need to look at?
> > > > No rush on my end, just wanted to be in sync.
> > >
> > > I spent some time on friday doing the initial work on this rework. In
> > > case you want to look at it  I just pushed it to:
> > >
> > > https://github.com/alexlarsson/linux/tree/overlay-verity
> > >
> > > The first 3 commits are the same as before, and the last one is the
> > > new approach. It's turned out pretty nice, simplifying things
> > > considerable.
> >
> > It does look nicer :)
> > I gave you a small review comment on github, but overall looks good.
> >
> > > But I really haven't had time to do a full re-review and
> > > test of it, so please don't merge it yet. I'll spend Monday ensuring
> > > it is in a good state for upstream.
> > >
> >
> > When you are done testing, please post v3 patches.
> >
> > Note that I pushed a new version of ovl-lazy-lowerdata branch to
> > fstests, with the minor :: syntax change.
>
> I posted the patches, and I also had to make some small changes to the
> xfstest commit:
>
> https://github.com/alexlarsson/xfstests/commits/verity-tests
>
> > Not sure if Miklos will have time to review them this cycle or wait
> > for the next one.
> >
> > There are also the mount api changes from Christian that conflict
> > with the new verify option.
> >
> > If we want to have all of these changes for this cycle, some collaboration
> > will be required.
>
> I would really like to get the overlay.verity xattr support in
> upstream pretty soon, because without it I can't release a stable
> version of the composefs userspace code. I don't want to release
> something that is using an xattr format that is not guaranteed to be
> stable.
>

Alex,

Pondering about this last sentence.

The overlay.verity xattr format is already in its 3rd revision since
the beginning of development.

When it was bare digest, it might have made sense to have no
header that describes the format.

When the algo byte was added, that was already a very big hint
that a proper header was in order.

Now that you had to change the meaning of the byte, it is very hard
to argue that the xattr format is guaranteed to be stable - in the sense
that it can never change again.

Please add a minimal header to the overlay.verity xattr format,
similar to ovl_fb, that will allow composefs/overlayfs to be
maintained as the separate projects that they are.

Something like this?

/* On-disk format for "verity" xattr */
struct ovl_verity {
        u8 version;     /* 0 */
        u8 len;          /* size of this header + size of digest */
        u8 flags;
        u8 algo;        /* digest algo */
        u8 digest[];
};

I realize that digest size may be inferred from xattr size,
but it is better to state the stored digest size explicitly and verify
that it matches expected xattr size.

Eric,

Does the digest buffer passed to fsnotify helpers have any
memory alignment requirements?

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers]

On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > I would really like to get the overlay.verity xattr support in
> > upstream pretty soon, because without it I can't release a stable
> > version of the composefs userspace code. I don't want to release
> > something that is using an xattr format that is not guaranteed to be
> > stable.
> >
> 
> Alex,
> 
> Pondering about this last sentence.
> 
> The overlay.verity xattr format is already in its 3rd revision since
> the beginning of development.
> 
> When it was bare digest, it might have made sense to have no
> header that describes the format.
> 
> When the algo byte was added, that was already a very big hint
> that a proper header was in order.
> 
> Now that you had to change the meaning of the byte, it is very hard
> to argue that the xattr format is guaranteed to be stable - in the sense
> that it can never change again.
> 
> Please add a minimal header to the overlay.verity xattr format,
> similar to ovl_fb, that will allow composefs/overlayfs to be
> maintained as the separate projects that they are.
> 
> Something like this?
> 
> /* On-disk format for "verity" xattr */
> struct ovl_verity {
>         u8 version;     /* 0 */
>         u8 len;          /* size of this header + size of digest */
>         u8 flags;
>         u8 algo;        /* digest algo */
>         u8 digest[];
> };
> 
> I realize that digest size may be inferred from xattr size,
> but it is better to state the stored digest size explicitly and verify
> that it matches expected xattr size.

If it was up to me I think I'd keep it even more "minimal" just do:

        struct ovl_verity {
                u8 version;     /* 0 */
                u8 algo;        /* digest algo */
                u8 digest[];
        };

It's up to you, though.  Keep in mind, the definition of a fsverity digest as
algorithm ID + raw digest is pretty fundamental to fsverity.  It's not
especially prone to change.  The confusion we had was just over what type of
algorithm ID to use.

(There is some inconsistency about whether the algorithm ID is u8, u16, or u32.
However, it's u8 on-disk in the fsverity_descriptor.  So it's fine for the
overlayfs xattr to use u8.)

> 
> Does the digest buffer passed to fsnotify helpers have any
> memory alignment requirements?

I think you're asking about the raw_digest argument to fsverity_get_digest()?
No, there's no alignment requirement.

- Eric

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > I would really like to get the overlay.verity xattr support in
> > > upstream pretty soon, because without it I can't release a stable
> > > version of the composefs userspace code. I don't want to release
> > > something that is using an xattr format that is not guaranteed to be
> > > stable.
> > >
> >
> > Alex,
> >
> > Pondering about this last sentence.
> >
> > The overlay.verity xattr format is already in its 3rd revision since
> > the beginning of development.
> >
> > When it was bare digest, it might have made sense to have no
> > header that describes the format.
> >
> > When the algo byte was added, that was already a very big hint
> > that a proper header was in order.
> >
> > Now that you had to change the meaning of the byte, it is very hard
> > to argue that the xattr format is guaranteed to be stable - in the sense
> > that it can never change again.
> >
> > Please add a minimal header to the overlay.verity xattr format,
> > similar to ovl_fb, that will allow composefs/overlayfs to be
> > maintained as the separate projects that they are.
> >
> > Something like this?
> >
> > /* On-disk format for "verity" xattr */
> > struct ovl_verity {
> >         u8 version;     /* 0 */
> >         u8 len;          /* size of this header + size of digest */
> >         u8 flags;
> >         u8 algo;        /* digest algo */
> >         u8 digest[];
> > };
> >
> > I realize that digest size may be inferred from xattr size,
> > but it is better to state the stored digest size explicitly and verify
> > that it matches expected xattr size.
>
> If it was up to me I think I'd keep it even more "minimal" just do:
>
>         struct ovl_verity {
>                 u8 version;     /* 0 */
>                 u8 algo;        /* digest algo */
>                 u8 digest[];
>         };
>
> It's up to you, though.

Please keep len and flags.
Nothing to be gained from removing them.

> Keep in mind, the definition of a fsverity digest as
> algorithm ID + raw digest is pretty fundamental to fsverity.  It's not
> especially prone to change.  The confusion we had was just over what type of
> algorithm ID to use.
>
> (There is some inconsistency about whether the algorithm ID is u8, u16, or u32.
> However, it's u8 on-disk in the fsverity_descriptor.  So it's fine for the
> overlayfs xattr to use u8.)
>
> >
> > Does the digest buffer passed to fsnotify helpers have any
> > memory alignment requirements?
>
> I think you're asking about the raw_digest argument to fsverity_get_digest()?
> No, there's no alignment requirement.
>

Yes, that is what I meant.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Fri, Jun 16, 2023 at 7:55 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > > I would really like to get the overlay.verity xattr support in
> > > > upstream pretty soon, because without it I can't release a stable
> > > > version of the composefs userspace code. I don't want to release
> > > > something that is using an xattr format that is not guaranteed to be
> > > > stable.
> > > >
> > >
> > > Alex,
> > >
> > > Pondering about this last sentence.
> > >
> > > The overlay.verity xattr format is already in its 3rd revision since
> > > the beginning of development.
> > >
> > > When it was bare digest, it might have made sense to have no
> > > header that describes the format.
> > >
> > > When the algo byte was added, that was already a very big hint
> > > that a proper header was in order.
> > >
> > > Now that you had to change the meaning of the byte, it is very hard
> > > to argue that the xattr format is guaranteed to be stable - in the sense
> > > that it can never change again.
> > >
> > > Please add a minimal header to the overlay.verity xattr format,
> > > similar to ovl_fb, that will allow composefs/overlayfs to be
> > > maintained as the separate projects that they are.
> > >
> > > Something like this?
> > >
> > > /* On-disk format for "verity" xattr */
> > > struct ovl_verity {
> > >         u8 version;     /* 0 */
> > >         u8 len;          /* size of this header + size of digest */
> > >         u8 flags;
> > >         u8 algo;        /* digest algo */
> > >         u8 digest[];
> > > };
> > >
> > > I realize that digest size may be inferred from xattr size,
> > > but it is better to state the stored digest size explicitly and verify
> > > that it matches expected xattr size.
> >
> > If it was up to me I think I'd keep it even more "minimal" just do:
> >
> >         struct ovl_verity {
> >                 u8 version;     /* 0 */
> >                 u8 algo;        /* digest algo */
> >                 u8 digest[];
> >         };
> >
> > It's up to you, though.
>
> Please keep len and flags.
> Nothing to be gained from removing them.

Something is gained by removing them: smaller images.

I have a Centos 9 developer rootfs here to test with. The basic
composefs image size for this with the current format (1 byte for algo)
is 11M. Adding one more byte (version) adds 88k (0.8%) to it and adding 3
bytes (version+flags+len) adds 240k (2.1%).

These are not huge costs, but they are not really giving any huge advantages
either. I mean, the digest length can be computed already, both from
the xattr size
and the algo type. And flag-like features could be encoded in the
version byte, or,
if really needed, a version 2 header could add a flags field.

I don't believe this format will actually need to change much.
However, I do agree
with the general requirement for *some* ability to move forward with
this format,
so I'm gonna go with a single version byte.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Fri, Jun 16, 2023 at 10:50 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Fri, Jun 16, 2023 at 7:55 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > > > I would really like to get the overlay.verity xattr support in
> > > > > upstream pretty soon, because without it I can't release a stable
> > > > > version of the composefs userspace code. I don't want to release
> > > > > something that is using an xattr format that is not guaranteed to be
> > > > > stable.
> > > > >
> > > >
> > > > Alex,
> > > >
> > > > Pondering about this last sentence.
> > > >
> > > > The overlay.verity xattr format is already in its 3rd revision since
> > > > the beginning of development.
> > > >
> > > > When it was bare digest, it might have made sense to have no
> > > > header that describes the format.
> > > >
> > > > When the algo byte was added, that was already a very big hint
> > > > that a proper header was in order.
> > > >
> > > > Now that you had to change the meaning of the byte, it is very hard
> > > > to argue that the xattr format is guaranteed to be stable - in the sense
> > > > that it can never change again.
> > > >
> > > > Please add a minimal header to the overlay.verity xattr format,
> > > > similar to ovl_fb, that will allow composefs/overlayfs to be
> > > > maintained as the separate projects that they are.
> > > >
> > > > Something like this?
> > > >
> > > > /* On-disk format for "verity" xattr */
> > > > struct ovl_verity {
> > > >         u8 version;     /* 0 */
> > > >         u8 len;          /* size of this header + size of digest */
> > > >         u8 flags;
> > > >         u8 algo;        /* digest algo */
> > > >         u8 digest[];
> > > > };
> > > >
> > > > I realize that digest size may be inferred from xattr size,
> > > > but it is better to state the stored digest size explicitly and verify
> > > > that it matches expected xattr size.
> > >
> > > If it was up to me I think I'd keep it even more "minimal" just do:
> > >
> > >         struct ovl_verity {
> > >                 u8 version;     /* 0 */
> > >                 u8 algo;        /* digest algo */
> > >                 u8 digest[];
> > >         };
> > >
> > > It's up to you, though.
> >
> > Please keep len and flags.
> > Nothing to be gained from removing them.
>
> Something is gained by removing them: smaller images.
>
> I have a Centos 9 developer rootfs here to test with. The basic
> composefs image size for this with the current format (1 byte for algo)
> is 11M. Adding one more byte (version) adds 88k (0.8%) to it and adding 3
> bytes (version+flags+len) adds 240k (2.1%).
>

I am not impressed.
This is micro optimization IMO.
If you want to optimize xattrs for size, you could have erofs
compress all trusted.overlay.* names into byte special codes.
On local disk fs, this few bytes change in xattr size won't matter one bit.

> These are not huge costs, but they are not really giving any huge advantages
> either. I mean, the digest length can be computed already, both from
> the xattr size
> and the algo type. And flag-like features could be encoded in the
> version byte, or,
> if really needed, a version 2 header could add a flags field.
>

Yes it could at the cost of uglier code.
We will do that if we have to, but not because we failed
to reserve room for flags to begin with.
It would be a rookie on-disk format mistake to do that.

> I don't believe this format will actually need to change much.
> However, I do agree
> with the general requirement for *some* ability to move forward with
> this format,
> so I'm gonna go with a single version byte.
>

I disagree.
If you present a real life use case why that really matters
then I can reconsider.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 10:50 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 7:55 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > > > > I would really like to get the overlay.verity xattr support in
> > > > > > upstream pretty soon, because without it I can't release a stable
> > > > > > version of the composefs userspace code. I don't want to release
> > > > > > something that is using an xattr format that is not guaranteed to be
> > > > > > stable.
> > > > > >
> > > > >
> > > > > Alex,
> > > > >
> > > > > Pondering about this last sentence.
> > > > >
> > > > > The overlay.verity xattr format is already in its 3rd revision since
> > > > > the beginning of development.
> > > > >
> > > > > When it was bare digest, it might have made sense to have no
> > > > > header that describes the format.
> > > > >
> > > > > When the algo byte was added, that was already a very big hint
> > > > > that a proper header was in order.
> > > > >
> > > > > Now that you had to change the meaning of the byte, it is very hard
> > > > > to argue that the xattr format is guaranteed to be stable - in the sense
> > > > > that it can never change again.
> > > > >
> > > > > Please add a minimal header to the overlay.verity xattr format,
> > > > > similar to ovl_fb, that will allow composefs/overlayfs to be
> > > > > maintained as the separate projects that they are.
> > > > >
> > > > > Something like this?
> > > > >
> > > > > /* On-disk format for "verity" xattr */
> > > > > struct ovl_verity {
> > > > >         u8 version;     /* 0 */
> > > > >         u8 len;          /* size of this header + size of digest */
> > > > >         u8 flags;
> > > > >         u8 algo;        /* digest algo */
> > > > >         u8 digest[];
> > > > > };
> > > > >
> > > > > I realize that digest size may be inferred from xattr size,
> > > > > but it is better to state the stored digest size explicitly and verify
> > > > > that it matches expected xattr size.
> > > >
> > > > If it was up to me I think I'd keep it even more "minimal" just do:
> > > >
> > > >         struct ovl_verity {
> > > >                 u8 version;     /* 0 */
> > > >                 u8 algo;        /* digest algo */
> > > >                 u8 digest[];
> > > >         };
> > > >
> > > > It's up to you, though.
> > >
> > > Please keep len and flags.
> > > Nothing to be gained from removing them.
> >
> > Something is gained by removing them: smaller images.
> >
> > I have a Centos 9 developer rootfs here to test with. The basic
> > composefs image size for this with the current format (1 byte for algo)
> > is 11M. Adding one more byte (version) adds 88k (0.8%) to it and adding 3
> > bytes (version+flags+len) adds 240k (2.1%).
> >
>
> I am not impressed.
> This is micro optimization IMO.

Most optimization is a sequence of micro optimizations.

> If you want to optimize xattrs for size, you could have erofs
> compress all trusted.overlay.* names into byte special codes.

This is already supported as per:
https://lists.ozlabs.org/pipermail/linux-erofs/2023-April/008074.html
(Although I have not yet added it to composefs userspace)

I don't see why you would not want to try to minimize both keyname and
value size though.

> On local disk fs, this few bytes change in xattr size won't matter one bit.
>
> > These are not huge costs, but they are not really giving any huge advantages
> > either. I mean, the digest length can be computed already, both from
> > the xattr size
> > and the algo type. And flag-like features could be encoded in the
> > version byte, or,
> > if really needed, a version 2 header could add a flags field.
> >
>
> Yes it could at the cost of uglier code.
> We will do that if we have to, but not because we failed
> to reserve room for flags to begin with.
> It would be a rookie on-disk format mistake to do that.
>
> > I don't believe this format will actually need to change much.
> > However, I do agree
> > with the general requirement for *some* ability to move forward with
> > this format,
> > so I'm gonna go with a single version byte.
> >
>
> I disagree.
> If you present a real life use case why that really matters
> then I can reconsider.

I disagree, but I'll add them.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 10:50 AM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 7:55 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > > > > > I would really like to get the overlay.verity xattr support in
> > > > > > > upstream pretty soon, because without it I can't release a stable
> > > > > > > version of the composefs userspace code. I don't want to release
> > > > > > > something that is using an xattr format that is not guaranteed to be
> > > > > > > stable.
> > > > > > >
> > > > > >
> > > > > > Alex,
> > > > > >
> > > > > > Pondering about this last sentence.
> > > > > >
> > > > > > The overlay.verity xattr format is already in its 3rd revision since
> > > > > > the beginning of development.
> > > > > >
> > > > > > When it was bare digest, it might have made sense to have no
> > > > > > header that describes the format.
> > > > > >
> > > > > > When the algo byte was added, that was already a very big hint
> > > > > > that a proper header was in order.
> > > > > >
> > > > > > Now that you had to change the meaning of the byte, it is very hard
> > > > > > to argue that the xattr format is guaranteed to be stable - in the sense
> > > > > > that it can never change again.
> > > > > >
> > > > > > Please add a minimal header to the overlay.verity xattr format,
> > > > > > similar to ovl_fb, that will allow composefs/overlayfs to be
> > > > > > maintained as the separate projects that they are.
> > > > > >
> > > > > > Something like this?
> > > > > >
> > > > > > /* On-disk format for "verity" xattr */
> > > > > > struct ovl_verity {
> > > > > >         u8 version;     /* 0 */
> > > > > >         u8 len;          /* size of this header + size of digest */
> > > > > >         u8 flags;
> > > > > >         u8 algo;        /* digest algo */
> > > > > >         u8 digest[];
> > > > > > };
> > > > > >
> > > > > > I realize that digest size may be inferred from xattr size,
> > > > > > but it is better to state the stored digest size explicitly and verify
> > > > > > that it matches expected xattr size.
> > > > >
> > > > > If it was up to me I think I'd keep it even more "minimal" just do:
> > > > >
> > > > >         struct ovl_verity {
> > > > >                 u8 version;     /* 0 */
> > > > >                 u8 algo;        /* digest algo */
> > > > >                 u8 digest[];
> > > > >         };
> > > > >
> > > > > It's up to you, though.
> > > >
> > > > Please keep len and flags.
> > > > Nothing to be gained from removing them.
> > >
> > > Something is gained by removing them: smaller images.
> > >
> > > I have a Centos 9 developer rootfs here to test with. The basic
> > > composefs image size for this with the current format (1 byte for algo)
> > > is 11M. Adding one more byte (version) adds 88k (0.8%) to it and adding 3
> > > bytes (version+flags+len) adds 240k (2.1%).
> > >
> >
> > I am not impressed.
> > This is micro optimization IMO.
>
> Most optimization is a sequence of micro optimizations.
>
> > If you want to optimize xattrs for size, you could have erofs
> > compress all trusted.overlay.* names into byte special codes.
>
> This is already supported as per:
> https://lists.ozlabs.org/pipermail/linux-erofs/2023-April/008074.html
> (Although I have not yet added it to composefs userspace)
>
> I don't see why you would not want to try to minimize both keyname and
> value size though.

Nice.
didn't know that it was already in motion.

>
> > On local disk fs, this few bytes change in xattr size won't matter one bit.
> >
> > > These are not huge costs, but they are not really giving any huge advantages
> > > either. I mean, the digest length can be computed already, both from
> > > the xattr size
> > > and the algo type. And flag-like features could be encoded in the
> > > version byte, or,
> > > if really needed, a version 2 header could add a flags field.
> > >
> >
> > Yes it could at the cost of uglier code.
> > We will do that if we have to, but not because we failed
> > to reserve room for flags to begin with.
> > It would be a rookie on-disk format mistake to do that.
> >
> > > I don't believe this format will actually need to change much.
> > > However, I do agree
> > > with the general requirement for *some* ability to move forward with
> > > this format,
> > > so I'm gonna go with a single version byte.
> > >
> >
> > I disagree.
> > If you present a real life use case why that really matters
> > then I can reconsider.
>
> I disagree, but I'll add them.
>

Let's ask for a 3rd opinion.
don't add them for now, unless Miklos says that you should.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Fri, Jun 16, 2023 at 11:28 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > >
> > > > I don't believe this format will actually need to change much.
> > > > However, I do agree
> > > > with the general requirement for *some* ability to move forward with
> > > > this format,
> > > > so I'm gonna go with a single version byte.
> > > >
> > >
> > > I disagree.
> > > If you present a real life use case why that really matters
> > > then I can reconsider.
> >
> > I disagree, but I'll add them.
> >
>
> Let's ask for a 3rd opinion.
> don't add them for now, unless Miklos says that you should.

I added them to the branch anyway for now. However, if we're going
full header + flags anyway, I wonder if we really need the
"overlay.digest" xattr at all? We could just put the header + optional
digest in the "overlay.metacopy" xattr, and then just read/store one
xattr. Right now metacopy is zero size, but adding some data to it
would not break ovl_check_metacopy_xattr() in older kernels.

Basically, during the lookup we get the metacopy xattr anyway, and
when we do we could record in a flag that there is a digest in it,
then during open we don't have to look for a separate digest xattr,
just re-load the metacopy xattr if the flag is set. With this in place
we can also easily add other flags to overlay.metacopy, which imho
makes a ton more sense than adding flags to overlay.digest.

I'll have a look at this.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Fri, Jun 16, 2023 at 2:33 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Fri, Jun 16, 2023 at 11:28 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > >
> > > > > I don't believe this format will actually need to change much.
> > > > > However, I do agree
> > > > > with the general requirement for *some* ability to move forward with
> > > > > this format,
> > > > > so I'm gonna go with a single version byte.
> > > > >
> > > >
> > > > I disagree.
> > > > If you present a real life use case why that really matters
> > > > then I can reconsider.
> > >
> > > I disagree, but I'll add them.
> > >
> >
> > Let's ask for a 3rd opinion.
> > don't add them for now, unless Miklos says that you should.
>
> I added them to the branch anyway for now. However, if we're going
> full header + flags anyway, I wonder if we really need the
> "overlay.digest" xattr at all? We could just put the header + optional
> digest in the "overlay.metacopy" xattr, and then just read/store one
> xattr. Right now metacopy is zero size, but adding some data to it
> would not break ovl_check_metacopy_xattr() in older kernels.
>
> Basically, during the lookup we get the metacopy xattr anyway, and
> when we do we could record in a flag that there is a digest in it,
> then during open we don't have to look for a separate digest xattr,
> just re-load the metacopy xattr if the flag is set. With this in place
> we can also easily add other flags to overlay.metacopy, which imho
> makes a ton more sense than adding flags to overlay.digest.
>

I think that is a wonderful idea.
I like it a lot.

Thanks,
Amir.

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Miklos Szeredi]

On Fri, 16 Jun 2023 at 14:24, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 2:33 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Fri, Jun 16, 2023 at 11:28 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > >
> > > > > > I don't believe this format will actually need to change much.
> > > > > > However, I do agree
> > > > > > with the general requirement for *some* ability to move forward with
> > > > > > this format,
> > > > > > so I'm gonna go with a single version byte.
> > > > > >
> > > > >
> > > > > I disagree.
> > > > > If you present a real life use case why that really matters
> > > > > then I can reconsider.
> > > >
> > > > I disagree, but I'll add them.
> > > >
> > >
> > > Let's ask for a 3rd opinion.
> > > don't add them for now, unless Miklos says that you should.
> >
> > I added them to the branch anyway for now. However, if we're going
> > full header + flags anyway, I wonder if we really need the
> > "overlay.digest" xattr at all? We could just put the header + optional
> > digest in the "overlay.metacopy" xattr, and then just read/store one
> > xattr. Right now metacopy is zero size, but adding some data to it
> > would not break ovl_check_metacopy_xattr() in older kernels.
> >
> > Basically, during the lookup we get the metacopy xattr anyway, and
> > when we do we could record in a flag that there is a digest in it,
> > then during open we don't have to look for a separate digest xattr,
> > just re-load the metacopy xattr if the flag is set. With this in place
> > we can also easily add other flags to overlay.metacopy, which imho
> > makes a ton more sense than adding flags to overlay.digest.
> >
>
> I think that is a wonderful idea.
> I like it a lot.

Agreed.

Thanks,
Miklos

Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata

[Gao Xiang]

On 2023/6/16 19:33, Alexander Larsson wrote:
> On Fri, Jun 16, 2023 at 11:28 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@redhat.com> wrote:
>>>
>>> On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>>
>>>>> I don't believe this format will actually need to change much.
>>>>> However, I do agree
>>>>> with the general requirement for *some* ability to move forward with
>>>>> this format,
>>>>> so I'm gonna go with a single version byte.
>>>>>
>>>>
>>>> I disagree.
>>>> If you present a real life use case why that really matters
>>>> then I can reconsider.
>>>
>>> I disagree, but I'll add them.
>>>
>>
>> Let's ask for a 3rd opinion.
>> don't add them for now, unless Miklos says that you should.
> 
> I added them to the branch anyway for now. However, if we're going
> full header + flags anyway, I wonder if we really need the
> "overlay.digest" xattr at all? We could just put the header + optional
> digest in the "overlay.metacopy" xattr, and then just read/store one
> xattr. Right now metacopy is zero size, but adding some data to it
> would not break ovl_check_metacopy_xattr() in older kernels.
> 
> Basically, during the lookup we get the metacopy xattr anyway, and
> when we do we could record in a flag that there is a digest in it,
> then during open we don't have to look for a separate digest xattr,
> just re-load the metacopy xattr if the flag is set. With this in place
> we can also easily add other flags to overlay.metacopy, which imho
> makes a ton more sense than adding flags to overlay.digest.

My own slight concern about this is that:

Previously, all metacopy inodes shares a common EROFS shared xattr
which can be cached in memory once when the first read and other
inodes won't trigger any I/Os for this.

If "overlay.metacopy" xattr is not empty thus it cannot be shared,
I guess at least you could place it into an EROFS inline xattr, that
may be good as well if you also keep "header + flags".

But I'm not sure if it's a good idea to keep the full fsverity
digest in "overlay.metacopy" honestly, especially overlayfs
fsverity feature is off.  That will amplify I/Os (which could be
landed in EROFS shared xattrs as "overlay.digest" in the past and
now we need to read it immediately.)

Especially for the "ls -lR" workload, I guess you might need to
evaluate this way (only add flags vs flags + digest in
"overlay.metacopy") first.

Thanks,
Gao Xiang

> 
> I'll have a look at this.
>