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

[PATCH v3 0/4] 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.

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

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 v2:
 * Rebased on top of overlayfs-next
 * We now alway do verity verification the first time the file content
   is used, rather than doing it at lookup time for the non-lazy lookup
   case.

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://github.com/alexlarsson/xfstests/commits/verity-tests

Alexander Larsson (4):
  fsverity: Export fsverity_get_digest
  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                  |  33 +++++-
 fs/overlayfs/file.c                     |   8 +-
 fs/overlayfs/namei.c                    |  54 +++++++++-
 fs/overlayfs/overlayfs.h                |  12 ++-
 fs/overlayfs/ovl_entry.h                |   3 +
 fs/overlayfs/super.c                    |  79 +++++++++++++-
 fs/overlayfs/util.c                     | 133 ++++++++++++++++++++++++
 fs/verity/measure.c                     |   1 +
 9 files changed, 340 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH v3 1/4] 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.40.1

[PATCH v3 2/4] ovl: Add framework for verity support

[Alexander Larsson]

This adds the scaffolding (docs, config, mount options) for supporting
the 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 xattr is handled.

If you enable verity ("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 same 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 4f36b8919f7c..9497988557b9 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -406,6 +406,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 d9be5d318e1b..f3b51fe59f68 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.40.1

[PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

When accessing file data the first time 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). If the verity check passes we store this info in
the inode flags as OVL_VERIFIED, so that we can avoid doing
it again if the inode remains in memory.

The verification is done in ovl_maybe_validate_verity() which needs to
be called in the same places as ovl_maybe_lookup_lowerdata(), so there
is a new ovl_verify_lowerdata() helper that calls these in the right
order, and all current callers of ovl_maybe_lookup_lowerdata() are
changed to call it instead.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/file.c      |  8 ++--
 fs/overlayfs/namei.c     | 54 +++++++++++++++++++++-
 fs/overlayfs/overlayfs.h |  9 +++-
 fs/overlayfs/super.c     |  5 ++-
 fs/overlayfs/util.c      | 96 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 568f743a5584..68f01fd7f211 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1078,7 +1078,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	 * not very important to optimize this case, so do lazy lowerdata lookup
 	 * before any copy up, so we can do it before taking ovl_inode_lock().
 	 */
-	err = ovl_maybe_lookup_lowerdata(dentry);
+	err = ovl_verify_lowerdata(dentry);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 39737c2aaa84..6583d08fdb7a 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -115,8 +115,8 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 	if (allow_meta) {
 		ovl_path_real(dentry, &realpath);
 	} else {
-		/* lazy lookup of lowerdata */
-		err = ovl_maybe_lookup_lowerdata(dentry);
+		/* lazy lookup and verify of lowerdata */
+		err = ovl_verify_lowerdata(dentry);
 		if (err)
 			return err;
 
@@ -159,8 +159,8 @@ static int ovl_open(struct inode *inode, struct file *file)
 	struct path realpath;
 	int err;
 
-	/* lazy lookup of lowerdata */
-	err = ovl_maybe_lookup_lowerdata(dentry);
+	/* lazy lookup and verify lowerdata */
+	err = ovl_verify_lowerdata(dentry);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 292b8a948f1a..d91650f71fab 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -889,8 +889,49 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	return err;
 }
 
+static int ovl_maybe_validate_verity(struct dentry *dentry)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct inode *inode = d_inode(dentry);
+	struct path datapath, metapath;
+	int err;
+
+	if (!ofs->config.verity ||
+	    !ovl_is_metacopy_dentry(dentry) ||
+	    ovl_test_flag(OVL_VERIFIED, inode))
+		return 0;
+
+	ovl_path_lowerdata(dentry, &datapath);
+	if (!datapath.dentry)
+		return -EIO;
+
+	ovl_path_real(dentry, &metapath);
+	if (!metapath.dentry)
+		return -EIO;
+
+	err = ovl_inode_lock_interruptible(inode);
+	if (err)
+		return err;
+
+	if (!ovl_test_flag(OVL_VERIFIED, inode)) {
+		const struct cred *old_cred;
+
+		old_cred = ovl_override_creds(dentry->d_sb);
+
+		err = ovl_validate_verity(ofs, &metapath, &datapath);
+		if (err == 0)
+			ovl_set_flag(OVL_VERIFIED, inode);
+
+		revert_creds(old_cred);
+	}
+
+	ovl_inode_unlock(inode);
+
+	return err;
+}
+
 /* Lazy lookup of lowerdata */
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
+static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 	const char *redirect = ovl_lowerdata_redirect(inode);
@@ -935,6 +976,17 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	goto out;
 }
 
+int ovl_verify_lowerdata(struct dentry *dentry)
+{
+	int err;
+
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
+	return ovl_maybe_validate_verity(dentry);
+}
+
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index fcac4e2c56ab..66e3f79ed6d0 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 {
@@ -49,6 +50,7 @@ enum ovl_inode_flag {
 	OVL_UPPERDATA,
 	/* Inode number will remain constant over copy up. */
 	OVL_CONST_INO,
+	OVL_VERIFIED,
 };
 
 enum ovl_entry_flag {
@@ -460,6 +462,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)
@@ -559,7 +566,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
 struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 				struct dentry *origin, bool verify);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
+int ovl_verify_lowerdata(struct dentry *dentry);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f3b51fe59f68..698f97622030 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -63,6 +63,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode)
 {
 	struct dentry *real = NULL, *lower;
+	int err;
 
 	/* It's an overlay file */
 	if (inode && d_inode(dentry) == inode)
@@ -89,7 +90,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	 * uprobes on offset within the file, so lowerdata should be available
 	 * when setting the uprobe.
 	 */
-	ovl_maybe_lookup_lowerdata(dentry);
+	err = ovl_verify_lowerdata(dentry);
+	if (err)
+		goto bug;
 	lower = ovl_dentry_lowerdata(dentry);
 	if (!lower)
 		goto bug;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 007ad7e5ba5b..a4666ba3d5a3 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>
@@ -706,6 +708,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, \
@@ -720,6 +723,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,
@@ -1152,6 +1156,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.40.1

[PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

When accessing file data the first time 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). If the verity check passes we store this info in
the inode flags as OVL_VERIFIED, so that we can avoid doing
it again if the inode remains in memory.

The verification is done in ovl_maybe_validate_verity() which needs to
be called in the same places as ovl_maybe_lookup_lowerdata(), so there
is a new ovl_verify_lowerdata() helper that calls these in the right
order, and all current callers of ovl_maybe_lookup_lowerdata() are
changed to call it instead.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/file.c      |  8 ++--
 fs/overlayfs/namei.c     | 54 +++++++++++++++++++++-
 fs/overlayfs/overlayfs.h |  9 +++-
 fs/overlayfs/super.c     |  5 ++-
 fs/overlayfs/util.c      | 96 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 568f743a5584..68f01fd7f211 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1078,7 +1078,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	 * not very important to optimize this case, so do lazy lowerdata lookup
 	 * before any copy up, so we can do it before taking ovl_inode_lock().
 	 */
-	err = ovl_maybe_lookup_lowerdata(dentry);
+	err = ovl_verify_lowerdata(dentry);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 39737c2aaa84..6583d08fdb7a 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -115,8 +115,8 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 	if (allow_meta) {
 		ovl_path_real(dentry, &realpath);
 	} else {
-		/* lazy lookup of lowerdata */
-		err = ovl_maybe_lookup_lowerdata(dentry);
+		/* lazy lookup and verify of lowerdata */
+		err = ovl_verify_lowerdata(dentry);
 		if (err)
 			return err;
 
@@ -159,8 +159,8 @@ static int ovl_open(struct inode *inode, struct file *file)
 	struct path realpath;
 	int err;
 
-	/* lazy lookup of lowerdata */
-	err = ovl_maybe_lookup_lowerdata(dentry);
+	/* lazy lookup and verify lowerdata */
+	err = ovl_verify_lowerdata(dentry);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 292b8a948f1a..d91650f71fab 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -889,8 +889,49 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	return err;
 }
 
+static int ovl_maybe_validate_verity(struct dentry *dentry)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct inode *inode = d_inode(dentry);
+	struct path datapath, metapath;
+	int err;
+
+	if (!ofs->config.verity ||
+	    !ovl_is_metacopy_dentry(dentry) ||
+	    ovl_test_flag(OVL_VERIFIED, inode))
+		return 0;
+
+	ovl_path_lowerdata(dentry, &datapath);
+	if (!datapath.dentry)
+		return -EIO;
+
+	ovl_path_real(dentry, &metapath);
+	if (!metapath.dentry)
+		return -EIO;
+
+	err = ovl_inode_lock_interruptible(inode);
+	if (err)
+		return err;
+
+	if (!ovl_test_flag(OVL_VERIFIED, inode)) {
+		const struct cred *old_cred;
+
+		old_cred = ovl_override_creds(dentry->d_sb);
+
+		err = ovl_validate_verity(ofs, &metapath, &datapath);
+		if (err == 0)
+			ovl_set_flag(OVL_VERIFIED, inode);
+
+		revert_creds(old_cred);
+	}
+
+	ovl_inode_unlock(inode);
+
+	return err;
+}
+
 /* Lazy lookup of lowerdata */
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
+static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 	const char *redirect = ovl_lowerdata_redirect(inode);
@@ -935,6 +976,17 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	goto out;
 }
 
+int ovl_verify_lowerdata(struct dentry *dentry)
+{
+	int err;
+
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
+	return ovl_maybe_validate_verity(dentry);
+}
+
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index fcac4e2c56ab..66e3f79ed6d0 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 {
@@ -49,6 +50,7 @@ enum ovl_inode_flag {
 	OVL_UPPERDATA,
 	/* Inode number will remain constant over copy up. */
 	OVL_CONST_INO,
+	OVL_VERIFIED,
 };
 
 enum ovl_entry_flag {
@@ -460,6 +462,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)
@@ -559,7 +566,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
 struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 				struct dentry *origin, bool verify);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
+int ovl_verify_lowerdata(struct dentry *dentry);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f3b51fe59f68..698f97622030 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -63,6 +63,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode)
 {
 	struct dentry *real = NULL, *lower;
+	int err;
 
 	/* It's an overlay file */
 	if (inode && d_inode(dentry) == inode)
@@ -89,7 +90,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	 * uprobes on offset within the file, so lowerdata should be available
 	 * when setting the uprobe.
 	 */
-	ovl_maybe_lookup_lowerdata(dentry);
+	err = ovl_verify_lowerdata(dentry);
+	if (err)
+		goto bug;
 	lower = ovl_dentry_lowerdata(dentry);
 	if (!lower)
 		goto bug;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 007ad7e5ba5b..a4666ba3d5a3 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>
@@ -706,6 +708,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, \
@@ -720,6 +723,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,
@@ -1152,6 +1156,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.40.1

[PATCH v3 4/4] 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 68f01fd7f211..67c4f14c694c 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)
@@ -643,6 +644,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;
 	}
@@ -918,6 +931,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;
 }
 
@@ -984,6 +1010,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 66e3f79ed6d0..472bef93cb0b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -462,11 +462,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 a4666ba3d5a3..cef907ff66bc 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1174,7 +1174,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;
@@ -1248,6 +1248,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.40.1

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

[Amir Goldstein]

On Mon, Jun 12, 2023 at 1:29 PM 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>
> ---
>  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 68f01fd7f211..67c4f14c694c 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)
> @@ -643,6 +644,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;
>         }
> @@ -918,6 +931,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;
>  }
>
> @@ -984,6 +1010,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)

-EOPNOTSUPP is also not a failure, although you could do:
if (!ofs->noxattr) ...

to avoid this error

> +               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 66e3f79ed6d0..472bef93cb0b 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -462,11 +462,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 a4666ba3d5a3..cef907ff66bc 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1174,7 +1174,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;
> @@ -1248,6 +1248,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.40.1
>

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

[Amir Goldstein]

On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
>
> I have also added some tests for this feature to xfstests[1].

I can't remember if there is a good reason why your test does
not include verify in a data-only layer.

I think this test coverage needs to be added.

>
> 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 v2:
>  * Rebased on top of overlayfs-next
>  * We now alway do verity verification the first time the file content
>    is used, rather than doing it at lookup time for the non-lazy lookup
>    case.

Aparat from the minor comment:

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

for the series.

Thanks,
Amir.

>
> 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://github.com/alexlarsson/xfstests/commits/verity-tests
>
> Alexander Larsson (4):
>   fsverity: Export fsverity_get_digest
>   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                  |  33 +++++-
>  fs/overlayfs/file.c                     |   8 +-
>  fs/overlayfs/namei.c                    |  54 +++++++++-
>  fs/overlayfs/overlayfs.h                |  12 ++-
>  fs/overlayfs/ovl_entry.h                |   3 +
>  fs/overlayfs/super.c                    |  79 +++++++++++++-
>  fs/overlayfs/util.c                     | 133 ++++++++++++++++++++++++
>  fs/verity/measure.c                     |   1 +
>  9 files changed, 340 insertions(+), 10 deletions(-)
>
> --
> 2.40.1
>

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

[Alexander Larsson]

On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> >
> > I have also added some tests for this feature to xfstests[1].
>
> I can't remember if there is a good reason why your test does
> not include verify in a data-only layer.
>
> I think this test coverage needs to be added.

Yeah. I'll add that.

> >
> > 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 v2:
> >  * Rebased on top of overlayfs-next
> >  * We now alway do verity verification the first time the file content
> >    is used, rather than doing it at lookup time for the non-lazy lookup
> >    case.
>
> Aparat from the minor comment:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>
> for the series.
>
> Thanks,
> Amir.
>
> >
> > 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://github.com/alexlarsson/xfstests/commits/verity-tests
> >
> > Alexander Larsson (4):
> >   fsverity: Export fsverity_get_digest
> >   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                  |  33 +++++-
> >  fs/overlayfs/file.c                     |   8 +-
> >  fs/overlayfs/namei.c                    |  54 +++++++++-
> >  fs/overlayfs/overlayfs.h                |  12 ++-
> >  fs/overlayfs/ovl_entry.h                |   3 +
> >  fs/overlayfs/super.c                    |  79 +++++++++++++-
> >  fs/overlayfs/util.c                     | 133 ++++++++++++++++++++++++
> >  fs/verity/measure.c                     |   1 +
> >  9 files changed, 340 insertions(+), 10 deletions(-)
> >
> > --
> > 2.40.1
> >
>


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

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

[Alexander Larsson]

On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > >
> > > I have also added some tests for this feature to xfstests[1].
> >
> > I can't remember if there is a good reason why your test does
> > not include verify in a data-only layer.
> >
> > I think this test coverage needs to be added.
>
> Yeah. I'll add that.

Updated the git branch with some lowerdata tests.

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

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

[Amir Goldstein]

On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > >
> > > > I have also added some tests for this feature to xfstests[1].
> > >
> > > I can't remember if there is a good reason why your test does
> > > not include verify in a data-only layer.
> > >
> > > I think this test coverage needs to be added.
> >
> > Yeah. I'll add that.
>
> Updated the git branch with some lowerdata tests.

Nice!

Thanks,
Amir.

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Eric Biggers]

On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote:
> +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.

It looks like my request for improved documentation was not taken, which is
unfortunate and makes this patchset difficult to review.

- Eric

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers]

On Mon, Jun 12, 2023 at 12:28:17PM +0200, Alexander Larsson wrote:
> +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;
> +}

This means the overlayfs verity xattr contains the algorithm ID of the fsverity
digest as a HASH_ALGO_* value.

That works, but I think HASH_ALGO_* is somewhat of an IMA-ism.  fsverity
actually uses FS_VERITY_HASH_ALG_* everywhere else, including in the UAPI and in
fsverity-utils which includes libfsverity
(https://git.kernel.org/pub/scm/fs/fsverity/fsverity-utils.git/tree/include/libfsverity.h).

I'm guessing that you used HASH_ALGO_* not because you really wanted to, but
rather just because it's what fsverity_get_digest() happens to return, as IMA is
currently its only user.

Can you consider
https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
make fsverity_get_digest() support both types of IDs?  Then you can use
FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Amir Goldstein]

On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote:
> > +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.
>
> It looks like my request for improved documentation was not taken, which is
> unfortunate and makes this patchset difficult to review.
>

Which one?
IIRC, you had two requests.
One very broad to get the overlayfs.rst document up-to-date:
[1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/

But I assume you mean the specific request about this sentence:
[2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/

I must say, I re-read this paragraph and the email thread and I don't
understand what it is that you are asking for.

I am not trying to dismiss your request, I am trying to understand.
I understand that overlayfs.rst is not a high standard spec and that
"metacopy files" is not well defined and I am not saying that we cannot
improve the documentation.

But since you already spent the extra time understanding the inaccuracies
of overlayfs.rst during v2 review and we already tried to explain the
details of metacopy in v2 review, please elaborate what information
is actually missing that made the review of v3 harder for you, so
that we can try to improve those parts.

Let me try to tell this story in a different way

Imagine a feature:
   send/recv email attachments by reference
   instead of attaching files, drop them in a network share
   and send/recv a URL to that share with optional verity sig.
verity=off:
   do not include verity sig when sending emails
   do not verify verity sig when receiving emails
verity=on:
   include verity sig when sending emails (if available)
   verify verity sig if it is included in received email
verity=require:
   same as verity=on, but if verity sig is not included
   in received email, do not follow URL.
   if verity sig not available when sending, attach the
   file itself instead of the URL -
   that is what "otherwise a full copy-up" means in the
   context above

To me this sounds exactly like the documentation above.
Is this story clear?
If it is, what in the doc above is missing to make it also clear?

Thanks,
Amir.

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Eric Biggers]

On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote:
> On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote:
> > > +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.
> >
> > It looks like my request for improved documentation was not taken, which is
> > unfortunate and makes this patchset difficult to review.
> >
> 
> Which one?
> IIRC, you had two requests.
> One very broad to get the overlayfs.rst document up-to-date:
> [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/

That isn't an accurate summary of what I said.  I actually pointed out two
specific things that are confusing specifically in the context of this feature.

> But I assume you mean the specific request about this sentence:
> [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/

And that was a third specific thing.  I got a detailed response back
(https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com),
which was helpful.  Unfortunately, the information in that response hasn't yet
found its way into the documentation that is being proposed.

In general the proposed documentation reads like the audience is overlayfs
developers.  It doesn't describe the motivation for the feature or how to use it
in each of the two use cases.  Maybe that is intended, but it's not what I had
expected to see.

Side note: the use of the passive voice, e.g. "the xattr may be set" and "the
xattr may then be used to verify", should be avoided since it makes it unclear
who/what is doing these actions.

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Alexander Larsson]

On Tue, Jun 13, 2023 at 8:37 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote:
> > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote:
> > > > +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.
> > >
> > > It looks like my request for improved documentation was not taken, which is
> > > unfortunate and makes this patchset difficult to review.
> > >
> >
> > Which one?
> > IIRC, you had two requests.
> > One very broad to get the overlayfs.rst document up-to-date:
> > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/
>
> That isn't an accurate summary of what I said.  I actually pointed out two
> specific things that are confusing specifically in the context of this feature.
>
> > But I assume you mean the specific request about this sentence:
> > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/
>
> And that was a third specific thing.  I got a detailed response back
> (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com),
> which was helpful.  Unfortunately, the information in that response hasn't yet
> found its way into the documentation that is being proposed.
>
> In general the proposed documentation reads like the audience is overlayfs
> developers.  It doesn't describe the motivation for the feature or how to use it
> in each of the two use cases.  Maybe that is intended, but it's not what I had
> expected to see.

I think your complaint is mostly related to the scope/goal of the
overlayfs.rst file. The way it is currently written, and how the patch
adds to it, it describes purely how overlayfs works "natively".

For example, when you change just the permissions of a file from the
lower layer, then overlayfs generates a metacopy file with some
special xattrs in the upper dir (rather than a full copy). It can also
do things like set a redirect xattr on the metacopy file if the file
changes name in the upper (due to a rename for example).

With the patch it may (depending on options) also set a verity xattr
on the metacopy file so that later uses of the upper layer can ensure
that the lower layer content file hasn't changed. This allows you to
improve the robustness of overlayfs layers, such that if at a later
time the lower directories accidentally change we will detect this
when using the metacopy file rather than delivering the wrong data.

These kinds of uses are completely standalone, you just mount overlay
with the right options and the kernel will do the right thing. There
is no need to know the internal details of the xattrs.

The problem is that I also have a different usecase with composefs.
This involves a completely different way of using overlayfs where you
construct manually a filesystem with the xattrs that overlayfs reads,
and then you mount this in a very special way (using a read-only
filesystem on loopback). A combination of overlayfs features will give
certain properties that are useful.

I feel like you expect that the overlayfs.rst document should describe
the details of the composefs setup, but instead it (both the patch and
the existing document) mainly describes the other kind of use.

In other words, if you are interested in using overlayfs with verity
to increase the robustness against layer changes, then the document is
probably enough. However, If you're interested in using overlayfs to
implement more complex features like composefs, then the document is
very weak in all sorts of details, not limited to the fs-verity part.

So, I don't think the current document is bad for what it does. But it
should perhaps be amended with a more detailed description of the
internals of the xattrs and their behaviour so that advanced users
(i.e. developers) don't have to read the source code for such details.
That seems like a general critique though, and not necessarily related
to this particular patchset.

> Side note: the use of the passive voice, e.g. "the xattr may be set" and "the
xattr may then be used to verify", should be avoided since it makes it unclear
who/what is doing these actions.

I'm not a native english speaker so I'm sure this can be written in a
better way, but when it says:

  When metadata copy up is used for a file, then the xattr
"trusted.overlay.verity" may be set on the metacopy file.

It really means:

  In certain circumstances, depending on mount options and other
details, it may be the case that when a metacopy file is being created
by overlayfs (as a side effect of a filesystem operation) the xattr
"trusted.overlay.verity" will be set on the new metacopy file.

So, this is not really using "may" in the passive voice, but I see
that it can be read that way.  I'll try to reword this in a way that
is more precise.

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

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Amir Goldstein]

On Tue, Jun 13, 2023 at 9:37 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote:
> > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote:
> > > > +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.
> > >
> > > It looks like my request for improved documentation was not taken, which is
> > > unfortunate and makes this patchset difficult to review.
> > >
> >
> > Which one?
> > IIRC, you had two requests.
> > One very broad to get the overlayfs.rst document up-to-date:
> > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/
>
> That isn't an accurate summary of what I said.  I actually pointed out two
> specific things that are confusing specifically in the context of this feature.
>
> > But I assume you mean the specific request about this sentence:
> > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/
>
> And that was a third specific thing.  I got a detailed response back
> (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com),
> which was helpful.  Unfortunately, the information in that response hasn't yet
> found its way into the documentation that is being proposed.

This response is mostly about composefs and as Alex wrote
these details have no place in overlayfs.rst and not related to the
proposed feature, which stands alone even without composefs.

>
> In general the proposed documentation reads like the audience is overlayfs
> developers.

I never considered that overlayfs.rst is for an audience other than
overlayfs developers or people that want to become overlayfs
developers. It is not a user guide. If it were, it would have been a
very bad one.

> It doesn't describe the motivation for the feature or how to use it
> in each of the two use cases.  Maybe that is intended, but it's not what I had
> expected to see.
>

Yeh, that's a valid point.
That is what I wanted to know - what exactly is missing.
I guess this is the documented motivation:

"This may then be used to verify the content of the source file at the time
the file is opened"

but it does not tell a complete chain of trust story.

How about something along the lines of:

"In the case that the upper layer can be trusted not to be tampered
with while overlayfs is offline and some of the lower layers cannot
be trusted not to be tampered with, the "verity" feature can protect
against offline modification to lower files, whose metadata has been
copied up to the upper layer (a.k.a "metacopy" files) ...."

It's generic language that what the patches do, regardless of the
trust model of composefs and how it composes an overlayfs layers.

> Side note: the use of the passive voice, e.g. "the xattr may be set" and "the
> xattr may then be used to verify", should be avoided since it makes it unclear
> who/what is doing these actions.
>

I am not a native English speaker and bad at documentation in general,
so the only thing I can say is the passive-aggressive "patches welcome" ;-)

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Mon, Jun 12, 2023 at 9:09 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Jun 12, 2023 at 12:28:17PM +0200, Alexander Larsson wrote:
> > +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;
> > +}
>
> This means the overlayfs verity xattr contains the algorithm ID of the fsverity
> digest as a HASH_ALGO_* value.
>
> That works, but I think HASH_ALGO_* is somewhat of an IMA-ism.  fsverity
> actually uses FS_VERITY_HASH_ALG_* everywhere else, including in the UAPI and in
> fsverity-utils which includes libfsverity
> (https://git.kernel.org/pub/scm/fs/fsverity/fsverity-utils.git/tree/include/libfsverity.h).
>
> I'm guessing that you used HASH_ALGO_* not because you really wanted to, but
> rather just because it's what fsverity_get_digest() happens to return, as IMA is
> currently its only user.

Yeah, that is exactly why.

> Can you consider
> https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
> make fsverity_get_digest() support both types of IDs?  Then you can use
> FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.

Sounds very good to me. I'll rebase the patchset on top of it. Not
sure how to best land this though, are you ok with this landing via
overlayfs-next?

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

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

[Alexander Larsson]

On Mon, Jun 12, 2023 at 12:27 PM Alexander Larsson <alexl@redhat.com> 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.
>
> I have also added some tests for this feature to xfstests[1].
>
> 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 v2:
>  * Rebased on top of overlayfs-next
>  * We now alway do verity verification the first time the file content
>    is used, rather than doing it at lookup time for the non-lazy lookup
>    case.
>
> 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://github.com/alexlarsson/xfstests/commits/verity-tests

I pushed a new version of this branch with the following changes:

 * Includes and uses the new fsverity_get_digest() rework from Eric
 * The above means we now use the FS_VERITY_ALG_* enum values in the xattr
 * Made the overlayfs.rst document change a bit more explicit on what
happens and by whom
 * Ignore EOPNOTSUPP failure from removexattrs as pointed out by Amir

The previous patchset is available as the overlay-verity-v3 tag so you
can compare the differences.

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers]

On Tue, Jun 13, 2023 at 01:41:34PM +0200, Alexander Larsson wrote:
> > Can you consider
> > https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
> > make fsverity_get_digest() support both types of IDs?  Then you can use
> > FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.
> 
> Sounds very good to me. I'll rebase the patchset on top of it. Not
> sure how to best land this though, are you ok with this landing via
> overlayfs-next?

If you're confident that this series will land in v6.4, then sure, you can apply
my patch "fsverity: rework fsverity_get_digest() again" to overlayfs-next,
instead of me taking it through fsverity/for-next.  (Hopefully the IMA
maintainer will ack it as well, as it touches security/integrity/.)

Just be careful about being overly-optimistic about features landing in the next
release.  I've had experience with cases like this before, where I didn't apply
something for a reason like this, but then the series didn't make it in right
away so it was worse than me just taking the patch in the first place.

I do see that the other prerequisites were just applied to overlayfs-next, so
maybe this is good to go now.  It's up to the other overlayfs folks.

- Eric

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

[Eric Biggers]

On Tue, Jun 13, 2023 at 03:57:48PM +0200, Alexander Larsson wrote:
> 
> I pushed a new version of this branch with the following changes:
> 
>  * Includes and uses the new fsverity_get_digest() rework from Eric
>  * The above means we now use the FS_VERITY_ALG_* enum values in the xattr
>  * Made the overlayfs.rst document change a bit more explicit on what
> happens and by whom
>  * Ignore EOPNOTSUPP failure from removexattrs as pointed out by Amir
> 
> The previous patchset is available as the overlay-verity-v3 tag so you
> can compare the differences.
> 

Where can I find the new version?

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Eric Biggers]

On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote:
> >
> > In general the proposed documentation reads like the audience is overlayfs
> > developers.
> 
> I never considered that overlayfs.rst is for an audience other than
> overlayfs developers or people that want to become overlayfs
> developers. It is not a user guide. If it were, it would have been a
> very bad one.
> 
> > It doesn't describe the motivation for the feature or how to use it
> > in each of the two use cases.  Maybe that is intended, but it's not what I had
> > expected to see.
> >
> 
> Yeh, that's a valid point.
> That is what I wanted to know - what exactly is missing.
> I guess this is the documented motivation:

Sure, but even if the document is just for kernel developers, it should still
describe motivation and use cases, as those are important for userstanding.

> "This may then be used to verify the content of the source file at the time
> the file is opened"
> 
> but it does not tell a complete chain of trust story.
> 
> How about something along the lines of:
> 
> "In the case that the upper layer can be trusted not to be tampered
> with while overlayfs is offline

So *online* tampering of the upper layer is fine?

> and some of the lower layers cannot
> be trusted not to be tampered with, the "verity" feature can protect
> against offline modification to lower files

Data of lower files, not simply "lower files", right?

Are *online* modifications to lower files indeed not in scope?

If the feature "can protect", then under what circumstances does it protect, and
under what circumstances what does it not protect?

It would also be helpful to explain what specifically is meant by "protect".
Does it mean that overlayfs prevents modifications to lower file data, or does
it mean that overlayfs detects modifications to lower file data after they
happen?  If the latter, what happens when overlayfs detects a modification?
What do userspace programs experience?

> , whose metadata has been
> copied up to the upper layer (a.k.a "metacopy" files) ...."
> 
> It's generic language that what the patches do, regardless of the
> trust model of composefs and how it composes an overlayfs layers.

It's better, but it could use some more detail.  See my comments above.

- Eric

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers]

On Tue, Jun 13, 2023 at 10:57:59AM -0700, Eric Biggers wrote:
> On Tue, Jun 13, 2023 at 01:41:34PM +0200, Alexander Larsson wrote:
> > > Can you consider
> > > https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
> > > make fsverity_get_digest() support both types of IDs?  Then you can use
> > > FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.
> > 
> > Sounds very good to me. I'll rebase the patchset on top of it. Not
> > sure how to best land this though, are you ok with this landing via
> > overlayfs-next?
> 
> If you're confident that this series will land in v6.4, then sure, you can apply
> my patch "fsverity: rework fsverity_get_digest() again" to overlayfs-next,
> instead of me taking it through fsverity/for-next.  (Hopefully the IMA
> maintainer will ack it as well, as it touches security/integrity/.)
> 
> Just be careful about being overly-optimistic about features landing in the next
> release.  I've had experience with cases like this before, where I didn't apply
> something for a reason like this, but then the series didn't make it in right
> away so it was worse than me just taking the patch in the first place.
> 
> I do see that the other prerequisites were just applied to overlayfs-next, so
> maybe this is good to go now.  It's up to the other overlayfs folks.

I meant to say 6.5, not 6.4.

Anyway, just let me know if I should apply it or not, before it gets too late.

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Amir Goldstein]

On Tue, Jun 13, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote:
> > >
> > > In general the proposed documentation reads like the audience is overlayfs
> > > developers.
> >
> > I never considered that overlayfs.rst is for an audience other than
> > overlayfs developers or people that want to become overlayfs
> > developers. It is not a user guide. If it were, it would have been a
> > very bad one.
> >
> > > It doesn't describe the motivation for the feature or how to use it
> > > in each of the two use cases.  Maybe that is intended, but it's not what I had
> > > expected to see.
> > >
> >
> > Yeh, that's a valid point.
> > That is what I wanted to know - what exactly is missing.
> > I guess this is the documented motivation:
>
> Sure, but even if the document is just for kernel developers, it should still
> describe motivation and use cases, as those are important for userstanding.
>
> > "This may then be used to verify the content of the source file at the time
> > the file is opened"
> >
> > but it does not tell a complete chain of trust story.
> >
> > How about something along the lines of:
> >
> > "In the case that the upper layer can be trusted not to be tampered
> > with while overlayfs is offline
>
> So *online* tampering of the upper layer is fine?

No, for the sake of this section, it would be easier to say
that upper is completely trusted and completely under our
control and that the feature only hardens overlayfs when
lower is not under our full control.

In the case of composefs it's actually two lower layers, one trusted
and one not trusted (and an optional trusted upper), but it does not
matter IMO for the sake of explaining the basic feature.

>
> > and some of the lower layers cannot
> > be trusted not to be tampered with, the "verity" feature can protect
> > against offline modification to lower files
>
> Data of lower files, not simply "lower files", right?

Right.

>
> Are *online* modifications to lower files indeed not in scope?

They are. doc should not mention online.

>
> If the feature "can protect", then under what circumstances does it protect, and
> under what circumstances what does it not protect?
>
> It would also be helpful to explain what specifically is meant by "protect".
> Does it mean that overlayfs prevents modifications to lower file data, or does
> it mean that overlayfs detects modifications to lower file data after they
> happen?  If the latter, what happens when overlayfs detects a modification?
> What do userspace programs experience?
>
> > , whose metadata has been
> > copied up to the upper layer (a.k.a "metacopy" files) ...."
> >
> > It's generic language that what the patches do, regardless of the
> > trust model of composefs and how it composes an overlayfs layers.
>
> It's better, but it could use some more detail.  See my comments above.
>

Fair enough.

Alex,

Please incorporate Eric's feedback above to come up with a more
detailed description than:

+This can be useful as a way to validate that a lower directory matches
+the correct upper when it is re-mounted later. In case you can
+guarantee that a overlayfs directory with verity xattrs is not
+tampered with (for example using dm-verity or fs-verity) this can even
+be used to guarantee the validity of an untrusted lower directory.
+

On the specific points that Eric mentioned.

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Amir Goldstein]

On Wed, Jun 14, 2023 at 6:28 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 10:57:59AM -0700, Eric Biggers wrote:
> > On Tue, Jun 13, 2023 at 01:41:34PM +0200, Alexander Larsson wrote:
> > > > Can you consider
> > > > https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
> > > > make fsverity_get_digest() support both types of IDs?  Then you can use
> > > > FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.
> > >
> > > Sounds very good to me. I'll rebase the patchset on top of it. Not
> > > sure how to best land this though, are you ok with this landing via
> > > overlayfs-next?
> >
> > If you're confident that this series will land in v6.4, then sure, you can apply
> > my patch "fsverity: rework fsverity_get_digest() again" to overlayfs-next,
> > instead of me taking it through fsverity/for-next.  (Hopefully the IMA
> > maintainer will ack it as well, as it touches security/integrity/.)

Mimi,

Can you please take a look:

https://lore.kernel.org/linux-integrity/20230612190047.59755-1-ebiggers@kernel.org/

> >
> > Just be careful about being overly-optimistic about features landing in the next
> > release.  I've had experience with cases like this before, where I didn't apply
> > something for a reason like this, but then the series didn't make it in right
> > away so it was worse than me just taking the patch in the first place.
> >
> > I do see that the other prerequisites were just applied to overlayfs-next, so
> > maybe this is good to go now.  It's up to the other overlayfs folks.
>
> I meant to say 6.5, not 6.4.
>
> Anyway, just let me know if I should apply it or not, before it gets too late.

If you want to make sure that your patch lands in 6.5, you'd better take
it through your tree.

If you provide a stable branch with the patch, Miklos will be able to merge
your branch if he wishes to take Alex's patches for 6.5.

Thanks,
Amir.

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

[Amir Goldstein]

On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > >
> > > > I have also added some tests for this feature to xfstests[1].
> > >
> > > I can't remember if there is a good reason why your test does
> > > not include verify in a data-only layer.
> > >
> > > I think this test coverage needs to be added.
> >
> > Yeah. I'll add that.
>
> Updated the git branch with some lowerdata tests.
>

What do I need to do in order to enable verity on ext4 besides
enabling FS_VERITY in the kernel?

I'm getting these on verity tests on ext4 in the default 4k config.
_require_scratch_verity() doesn't mention any requirement other
that 4K blocks and extent format files.

Thanks,
Amir.

BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
DEVICE: /dev/vdb
EXT_MKFS_OPTIONS: -b 4096
EXT_MOUNT_OPTIONS: -o block_validity
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 kvm-xfstests
6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
Jun 13 18:16:59 IDT 2023
MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc

generic/572        [06:04:42] [06:04:47] [not run]
generic/572 -- ext4 verity isn't usable by default with these mkfs options
...

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

[Eric Biggers]

On Wed, Jun 14, 2023 at 09:14:10AM +0300, Amir Goldstein wrote:
> What do I need to do in order to enable verity on ext4 besides
> enabling FS_VERITY in the kernel?
> 
> I'm getting these on verity tests on ext4 in the default 4k config.
> _require_scratch_verity() doesn't mention any requirement other
> that 4K blocks and extent format files.
> 
> Thanks,
> Amir.
> 
> BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> DEVICE: /dev/vdb
> EXT_MKFS_OPTIONS: -b 4096
> EXT_MOUNT_OPTIONS: -o block_validity
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 kvm-xfstests
> 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> Jun 13 18:16:59 IDT 2023
> MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> 
> generic/572        [06:04:42] [06:04:47] [not run]
> generic/572 -- ext4 verity isn't usable by default with these mkfs options
> ...

I don't know why it's not working for you.  Is there anything helpful in
/results/ext4/results-4k/generic/572.full?

- Eric

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

[Alexander Larsson]

On Tue, Jun 13, 2023 at 7:59 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 03:57:48PM +0200, Alexander Larsson wrote:
> >
> > I pushed a new version of this branch with the following changes:
> >
> >  * Includes and uses the new fsverity_get_digest() rework from Eric
> >  * The above means we now use the FS_VERITY_ALG_* enum values in the xattr
> >  * Made the overlayfs.rst document change a bit more explicit on what
> > happens and by whom
> >  * Ignore EOPNOTSUPP failure from removexattrs as pointed out by Amir
> >
> > The previous patchset is available as the overlay-verity-v3 tag so you
> > can compare the differences.
> >
>
> Where can I find the new version?

Sorry, its at https://github.com/alexlarsson/linux/commits/overlay-verity

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

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

[Alexander Larsson]

On Wed, Jun 14, 2023 at 8:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > > >
> > > > > I have also added some tests for this feature to xfstests[1].
> > > >
> > > > I can't remember if there is a good reason why your test does
> > > > not include verify in a data-only layer.
> > > >
> > > > I think this test coverage needs to be added.
> > >
> > > Yeah. I'll add that.
> >
> > Updated the git branch with some lowerdata tests.
> >
>
> What do I need to do in order to enable verity on ext4 besides
> enabling FS_VERITY in the kernel?
>
> I'm getting these on verity tests on ext4 in the default 4k config.
> _require_scratch_verity() doesn't mention any requirement other
> that 4K blocks and extent format files.
>
> Thanks,
> Amir.
>
> BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> DEVICE: /dev/vdb
> EXT_MKFS_OPTIONS: -b 4096
> EXT_MOUNT_OPTIONS: -o block_validity
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 kvm-xfstests
> 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> Jun 13 18:16:59 IDT 2023
> MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
>
> generic/572        [06:04:42] [06:04:47] [not run]
> generic/572 -- ext4 verity isn't usable by default with these mkfs options
> ...

You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).

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

Re: [PATCH v3 3/4] ovl: Validate verity xattr when resolving lowerdata

[Alexander Larsson]

On Wed, Jun 14, 2023 at 5:28 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 13, 2023 at 10:57:59AM -0700, Eric Biggers wrote:
> > On Tue, Jun 13, 2023 at 01:41:34PM +0200, Alexander Larsson wrote:
> > > > Can you consider
> > > > https://lore.kernel.org/r/20230612190047.59755-1-ebiggers@kernel.org which would
> > > > make fsverity_get_digest() support both types of IDs?  Then you can use
> > > > FS_VERITY_HASH_ALG_*, which I think would make things slightly easier for you.
> > >
> > > Sounds very good to me. I'll rebase the patchset on top of it. Not
> > > sure how to best land this though, are you ok with this landing via
> > > overlayfs-next?
> >
> > If you're confident that this series will land in v6.4, then sure, you can apply
> > my patch "fsverity: rework fsverity_get_digest() again" to overlayfs-next,
> > instead of me taking it through fsverity/for-next.  (Hopefully the IMA
> > maintainer will ack it as well, as it touches security/integrity/.)
> >
> > Just be careful about being overly-optimistic about features landing in the next
> > release.  I've had experience with cases like this before, where I didn't apply
> > something for a reason like this, but then the series didn't make it in right
> > away so it was worse than me just taking the patch in the first place.
> >
> > I do see that the other prerequisites were just applied to overlayfs-next, so
> > maybe this is good to go now.  It's up to the other overlayfs folks.
>
> I meant to say 6.5, not 6.4.
>
> Anyway, just let me know if I should apply it or not, before it gets too late.

Honestly, I have no idea about the timescale here. That is all up to
miklos really. Maybe best to do as amir say and take it through your
tree but on from a branch that milklos can merge into overlafs-next if
he wants to take it this cycle.

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

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Alexander Larsson]

On Wed, Jun 14, 2023 at 7:24 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jun 13, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote:
> > > >
> > > > In general the proposed documentation reads like the audience is overlayfs
> > > > developers.
> > >
> > > I never considered that overlayfs.rst is for an audience other than
> > > overlayfs developers or people that want to become overlayfs
> > > developers. It is not a user guide. If it were, it would have been a
> > > very bad one.
> > >
> > > > It doesn't describe the motivation for the feature or how to use it
> > > > in each of the two use cases.  Maybe that is intended, but it's not what I had
> > > > expected to see.
> > > >
> > >
> > > Yeh, that's a valid point.
> > > That is what I wanted to know - what exactly is missing.
> > > I guess this is the documented motivation:
> >
> > Sure, but even if the document is just for kernel developers, it should still
> > describe motivation and use cases, as those are important for userstanding.
> >
> > > "This may then be used to verify the content of the source file at the time
> > > the file is opened"
> > >
> > > but it does not tell a complete chain of trust story.
> > >
> > > How about something along the lines of:
> > >
> > > "In the case that the upper layer can be trusted not to be tampered
> > > with while overlayfs is offline
> >
> > So *online* tampering of the upper layer is fine?
>
> No, for the sake of this section, it would be easier to say
> that upper is completely trusted and completely under our
> control and that the feature only hardens overlayfs when
> lower is not under our full control.
>
> In the case of composefs it's actually two lower layers, one trusted
> and one not trusted (and an optional trusted upper), but it does not
> matter IMO for the sake of explaining the basic feature.
>
> >
> > > and some of the lower layers cannot
> > > be trusted not to be tampered with, the "verity" feature can protect
> > > against offline modification to lower files
> >
> > Data of lower files, not simply "lower files", right?
>
> Right.
>
> >
> > Are *online* modifications to lower files indeed not in scope?
>
> They are. doc should not mention online.
>
> >
> > If the feature "can protect", then under what circumstances does it protect, and
> > under what circumstances what does it not protect?
> >
> > It would also be helpful to explain what specifically is meant by "protect".
> > Does it mean that overlayfs prevents modifications to lower file data, or does
> > it mean that overlayfs detects modifications to lower file data after they
> > happen?  If the latter, what happens when overlayfs detects a modification?
> > What do userspace programs experience?
> >
> > > , whose metadata has been
> > > copied up to the upper layer (a.k.a "metacopy" files) ...."
> > >
> > > It's generic language that what the patches do, regardless of the
> > > trust model of composefs and how it composes an overlayfs layers.
> >
> > It's better, but it could use some more detail.  See my comments above.
> >
>
> Fair enough.
>
> Alex,
>
> Please incorporate Eric's feedback above to come up with a more
> detailed description than:
>
> +This can be useful as a way to validate that a lower directory matches
> +the correct upper when it is re-mounted later. In case you can
> +guarantee that a overlayfs directory with verity xattrs is not
> +tampered with (for example using dm-verity or fs-verity) this can even
> +be used to guarantee the validity of an untrusted lower directory.
> +
>
> On the specific points that Eric mentioned.

I think maybe it's best we hash this out here first.
What do you think about this version:

fs-verity support
----------------------

During metadata copy up of a lower file, if the source file has
fs-verity enabled and overlay verity support is enabled, then the
"trusted.overlay.verity" xattr is set on the new metacopy file. This
specifies the expected fs-verity digest of the lowerdata file, which is
used to verify the content of the lower file at the time the metacopy
file is opened.

When a layer containing verity xattrs is used, it means that any
such metacopy file in the upper layer is guaranteed to match the
content that was in the lower at the time of the copy-up. If at any time
(during a mount, after a remount, etc) such a file in the lower is
replaced or modified in any way, then opening the corresponding file on
overlayfs will result in EIO and a detailed error printed to the kernel logs.
(Actually, due to caching the overlayfs mount might still see the previous
file contents after a lower file is replaced under an active mount, but
it will never see the wrong data.)

Verity can be used as a general robustness check to detect accidental
changes in the overlayfs directories in use. But, with additional care
it can also give more powerful guarantees. For example, if the upper layer
is fully trusted (by using dm-verity or something similar), then an untrusted
lower layer can be used to supply validated file content for all metacopy files.
If additionally the untrusted lower directories are specified as "Data-only",
then they can only supply such file content, and the entire mount can be
trusted to match the upper layer.

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

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Eric Biggers]

On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote:
> When a layer containing verity xattrs is used, it means that any
> such metacopy file in the upper layer is guaranteed to match the
> content that was in the lower at the time of the copy-up. If at any time
> (during a mount, after a remount, etc) such a file in the lower is
> replaced or modified in any way, then opening the corresponding file on
> overlayfs will result in EIO and a detailed error printed to the kernel logs.
> (Actually, due to caching the overlayfs mount might still see the previous
> file contents after a lower file is replaced under an active mount, but
> it will never see the wrong data.)

Well, the key point of fsverity is that data is not verified until it is
actually read.  At open time, the fsverity file digest is made available in
constant time, and overlayfs will verify that.  However, invalid data blocks are
not reported until the data is actually read.  The error that applications get
is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at
https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files

So overlayfs might report EIO at open time, or it might not report an error
until the modified data is read.  And in the latter case, presumably the error
seen by the application matches the one for using fsverity natively?

You can link to the fsverity documentation somewhere if it would be helpful, but
I'd still like the semantics of how this works on overlayfs to be documented.

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Alexander Larsson]

On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote:
> > When a layer containing verity xattrs is used, it means that any
> > such metacopy file in the upper layer is guaranteed to match the
> > content that was in the lower at the time of the copy-up. If at any time
> > (during a mount, after a remount, etc) such a file in the lower is
> > replaced or modified in any way, then opening the corresponding file on
> > overlayfs will result in EIO and a detailed error printed to the kernel logs.
> > (Actually, due to caching the overlayfs mount might still see the previous
> > file contents after a lower file is replaced under an active mount, but
> > it will never see the wrong data.)
>
> Well, the key point of fsverity is that data is not verified until it is
> actually read.  At open time, the fsverity file digest is made available in
> constant time, and overlayfs will verify that.  However, invalid data blocks are
> not reported until the data is actually read.  The error that applications get
> is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at
> https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files
>
> So overlayfs might report EIO at open time, or it might not report an error
> until the modified data is read.  And in the latter case, presumably the error
> seen by the application matches the one for using fsverity natively?

Yes, I'm aware of that, but do we need to describe this in the
overlayfs documentation?
The text I wrote is describing the behaviour that overlayfs adds to
the mix, and I sort of
assumed the late validation from fs-verity itself would be known about
if the file already
has fs-verity enabled.

> You can link to the fsverity documentation somewhere if it would be helpful, but
> I'd still like the semantics of how this works on overlayfs to be documented.

I guess just adding a link to that is not that bad. What about:

----
When a layer containing verity xattrs is used, it means that any such
metacopy file in the upper layer is guaranteed to match the content
that was in the lower at the time of the copy-up. If at any time
(during a mount, after a remount, etc) such a file in the lower is
replaced or modified in any way, then opening the corresponding file
on overlayfs will result in EIO and a detailed error printed to the
kernel logs.  (Actually, due to caching the overlayfs mount might
still see the previous file contents after a lower file is replaced
under an active mount, but it will never see the wrong data.)  In
addition fs-verity will do late validation of the file content, as
described in :ref:`Documentation/filesystems/fsverity.rst
<accessing_verity_files>`.
---

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

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Eric Biggers]

On Fri, Jun 16, 2023 at 10:11:06AM +0200, Alexander Larsson wrote:
> On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote:
> > > When a layer containing verity xattrs is used, it means that any
> > > such metacopy file in the upper layer is guaranteed to match the
> > > content that was in the lower at the time of the copy-up. If at any time
> > > (during a mount, after a remount, etc) such a file in the lower is
> > > replaced or modified in any way, then opening the corresponding file on
> > > overlayfs will result in EIO and a detailed error printed to the kernel logs.
> > > (Actually, due to caching the overlayfs mount might still see the previous
> > > file contents after a lower file is replaced under an active mount, but
> > > it will never see the wrong data.)
> >
> > Well, the key point of fsverity is that data is not verified until it is
> > actually read.  At open time, the fsverity file digest is made available in
> > constant time, and overlayfs will verify that.  However, invalid data blocks are
> > not reported until the data is actually read.  The error that applications get
> > is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at
> > https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files
> >
> > So overlayfs might report EIO at open time, or it might not report an error
> > until the modified data is read.  And in the latter case, presumably the error
> > seen by the application matches the one for using fsverity natively?
> 
> Yes, I'm aware of that, but do we need to describe this in the
> overlayfs documentation?
> The text I wrote is describing the behaviour that overlayfs adds to
> the mix, and I sort of
> assumed the late validation from fs-verity itself would be known about
> if the file already
> has fs-verity enabled.
> 
> > You can link to the fsverity documentation somewhere if it would be helpful, but
> > I'd still like the semantics of how this works on overlayfs to be documented.
> 
> I guess just adding a link to that is not that bad. What about:
> 
> ----
> When a layer containing verity xattrs is used, it means that any such
> metacopy file in the upper layer is guaranteed to match the content
> that was in the lower at the time of the copy-up. If at any time
> (during a mount, after a remount, etc) such a file in the lower is
> replaced or modified in any way, then opening the corresponding file
> on overlayfs will result in EIO and a detailed error printed to the
> kernel logs.  (Actually, due to caching the overlayfs mount might
> still see the previous file contents after a lower file is replaced
> under an active mount, but it will never see the wrong data.)  In
> addition fs-verity will do late validation of the file content, as
> described in :ref:`Documentation/filesystems/fsverity.rst
> <accessing_verity_files>`.

That still has the incorrect statement "If at any time (during a mount, after a
remount, etc) such a file in the lower is replaced or modified in any way, then
opening the corresponding file on overlayfs will result in EIO and a detailed
error printed to the kernel logs."  See my last email where I explained why that
statement is not correct.

- Eric

Re: [PATCH v3 2/4] ovl: Add framework for verity support

[Alexander Larsson]

On Sat, Jun 17, 2023 at 9:47 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 16, 2023 at 10:11:06AM +0200, Alexander Larsson wrote:
> > On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote:
> > > > When a layer containing verity xattrs is used, it means that any
> > > > such metacopy file in the upper layer is guaranteed to match the
> > > > content that was in the lower at the time of the copy-up. If at any time
> > > > (during a mount, after a remount, etc) such a file in the lower is
> > > > replaced or modified in any way, then opening the corresponding file on
> > > > overlayfs will result in EIO and a detailed error printed to the kernel logs.
> > > > (Actually, due to caching the overlayfs mount might still see the previous
> > > > file contents after a lower file is replaced under an active mount, but
> > > > it will never see the wrong data.)
> > >
> > > Well, the key point of fsverity is that data is not verified until it is
> > > actually read.  At open time, the fsverity file digest is made available in
> > > constant time, and overlayfs will verify that.  However, invalid data blocks are
> > > not reported until the data is actually read.  The error that applications get
> > > is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at
> > > https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files
> > >
> > > So overlayfs might report EIO at open time, or it might not report an error
> > > until the modified data is read.  And in the latter case, presumably the error
> > > seen by the application matches the one for using fsverity natively?
> >
> > Yes, I'm aware of that, but do we need to describe this in the
> > overlayfs documentation?
> > The text I wrote is describing the behaviour that overlayfs adds to
> > the mix, and I sort of
> > assumed the late validation from fs-verity itself would be known about
> > if the file already
> > has fs-verity enabled.
> >
> > > You can link to the fsverity documentation somewhere if it would be helpful, but
> > > I'd still like the semantics of how this works on overlayfs to be documented.
> >
> > I guess just adding a link to that is not that bad. What about:
> >
> > ----
> > When a layer containing verity xattrs is used, it means that any such
> > metacopy file in the upper layer is guaranteed to match the content
> > that was in the lower at the time of the copy-up. If at any time
> > (during a mount, after a remount, etc) such a file in the lower is
> > replaced or modified in any way, then opening the corresponding file
> > on overlayfs will result in EIO and a detailed error printed to the
> > kernel logs.  (Actually, due to caching the overlayfs mount might
> > still see the previous file contents after a lower file is replaced
> > under an active mount, but it will never see the wrong data.)  In
> > addition fs-verity will do late validation of the file content, as
> > described in :ref:`Documentation/filesystems/fsverity.rst
> > <accessing_verity_files>`.
>
> That still has the incorrect statement "If at any time (during a mount, after a
> remount, etc) such a file in the lower is replaced or modified in any way, then
> opening the corresponding file on overlayfs will result in EIO and a detailed
> error printed to the kernel logs."  See my last email where I explained why that
> statement is not correct.

If the modification of the file happens via the kernel vfs API, then
the new backing file will have either have no, or the wrong fs-verity
digest, which will be reported by overlayfs on open. It is true that a
modification on the block level which the vfs is unaware of will be
reported at read time, by fs-verity (i.e. independent of overlayfs).
This is what I meant by the "in addition .." part, but maybe that is
not clear. I'll try to rephrase it.


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

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

[Amir Goldstein]

On Wed, Jun 14, 2023 at 10:17 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Wed, Jun 14, 2023 at 8:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> > > >
> > > > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > > > >
> > > > > > I have also added some tests for this feature to xfstests[1].
> > > > >
> > > > > I can't remember if there is a good reason why your test does
> > > > > not include verify in a data-only layer.
> > > > >
> > > > > I think this test coverage needs to be added.
> > > >
> > > > Yeah. I'll add that.
> > >
> > > Updated the git branch with some lowerdata tests.
> > >
> >
> > What do I need to do in order to enable verity on ext4 besides
> > enabling FS_VERITY in the kernel?
> >
> > I'm getting these on verity tests on ext4 in the default 4k config.
> > _require_scratch_verity() doesn't mention any requirement other
> > that 4K blocks and extent format files.
> >
> > Thanks,
> > Amir.
> >
> > BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> > DEVICE: /dev/vdb
> > EXT_MKFS_OPTIONS: -b 4096
> > EXT_MOUNT_OPTIONS: -o block_validity
> > FSTYP         -- ext4
> > PLATFORM      -- Linux/x86_64 kvm-xfstests
> > 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> > Jun 13 18:16:59 IDT 2023
> > MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> > MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> >
> > generic/572        [06:04:42] [06:04:47] [not run]
> > generic/572 -- ext4 verity isn't usable by default with these mkfs options
> > ...
>
> You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).
>

That was indeed missing in my setup, but it did not fix the problem.

Turned out that I had a very old version of fsverity installed in my
kvm-xfstest test VM, where there is no --block-size option to
fsverity enable would always fail.

Eric,

You may consider adding a check for minimal version of
fsverity or check for support of --block-size option to make
this error reason more clear for testers.

Ted,

FYI, FSVERITY_GIT in fstests-bld/config points to an out of date URL

How come there is no ext4/cfg/verity in fstests-bld?
Are you guys not testing fsverity with fstests-bld?
Should we just add fsverity config or add verity to ext4/cfg/encrypt
instead to avoid growing the test matrix?

I can send patches for fstests-bld fixing the above if you like.

Alex,

Verified that your verity-tests2 work as expected with v5 patches.

Thanks,
Amir.

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

[Alexander Larsson]

On Thu, Jun 22, 2023 at 11:37 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 14, 2023 at 10:17 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Wed, Jun 14, 2023 at 8:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
> > > >
> > > > On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > > > > >
> > > > > > > I have also added some tests for this feature to xfstests[1].
> > > > > >
> > > > > > I can't remember if there is a good reason why your test does
> > > > > > not include verify in a data-only layer.
> > > > > >
> > > > > > I think this test coverage needs to be added.
> > > > >
> > > > > Yeah. I'll add that.
> > > >
> > > > Updated the git branch with some lowerdata tests.
> > > >
> > >
> > > What do I need to do in order to enable verity on ext4 besides
> > > enabling FS_VERITY in the kernel?
> > >
> > > I'm getting these on verity tests on ext4 in the default 4k config.
> > > _require_scratch_verity() doesn't mention any requirement other
> > > that 4K blocks and extent format files.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> > > DEVICE: /dev/vdb
> > > EXT_MKFS_OPTIONS: -b 4096
> > > EXT_MOUNT_OPTIONS: -o block_validity
> > > FSTYP         -- ext4
> > > PLATFORM      -- Linux/x86_64 kvm-xfstests
> > > 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> > > Jun 13 18:16:59 IDT 2023
> > > MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> > > MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> > >
> > > generic/572        [06:04:42] [06:04:47] [not run]
> > > generic/572 -- ext4 verity isn't usable by default with these mkfs options
> > > ...
> >
> > You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).
> >
>
> That was indeed missing in my setup, but it did not fix the problem.
>
> Turned out that I had a very old version of fsverity installed in my
> kvm-xfstest test VM, where there is no --block-size option to
> fsverity enable would always fail.
>
> Eric,
>
> You may consider adding a check for minimal version of
> fsverity or check for support of --block-size option to make
> this error reason more clear for testers.
>
> Ted,
>
> FYI, FSVERITY_GIT in fstests-bld/config points to an out of date URL
>
> How come there is no ext4/cfg/verity in fstests-bld?
> Are you guys not testing fsverity with fstests-bld?
> Should we just add fsverity config or add verity to ext4/cfg/encrypt
> instead to avoid growing the test matrix?
>
> I can send patches for fstests-bld fixing the above if you like.
>
> Alex,
>
> Verified that your verity-tests2 work as expected with v5 patches.

To be honest I have not validated that my changes to the shared verity
code still works with the non-overlayfs tests. If you have a setup for
it it would be great if you could try the regular ext4 w/ fs-veriy
tests on top of the verity-test2 branch.

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

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

[Amir Goldstein]

On Thu, Jun 22, 2023 at 12:52 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 11:37 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Jun 14, 2023 at 10:17 AM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Wed, Jun 14, 2023 at 8:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 12, 2023 at 5:54 PM Alexander Larsson <alexl@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jun 12, 2023 at 1:09 PM Alexander Larsson <alexl@redhat.com> wrote:
> > > > > >
> > > > > > On Mon, Jun 12, 2023 at 12:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 12, 2023 at 1:27 PM Alexander Larsson <alexl@redhat.com> 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.
> > > > > > > >
> > > > > > > > I have also added some tests for this feature to xfstests[1].
> > > > > > >
> > > > > > > I can't remember if there is a good reason why your test does
> > > > > > > not include verify in a data-only layer.
> > > > > > >
> > > > > > > I think this test coverage needs to be added.
> > > > > >
> > > > > > Yeah. I'll add that.
> > > > >
> > > > > Updated the git branch with some lowerdata tests.
> > > > >
> > > >
> > > > What do I need to do in order to enable verity on ext4 besides
> > > > enabling FS_VERITY in the kernel?
> > > >
> > > > I'm getting these on verity tests on ext4 in the default 4k config.
> > > > _require_scratch_verity() doesn't mention any requirement other
> > > > that 4K blocks and extent format files.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > > > BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> > > > DEVICE: /dev/vdb
> > > > EXT_MKFS_OPTIONS: -b 4096
> > > > EXT_MOUNT_OPTIONS: -o block_validity
> > > > FSTYP         -- ext4
> > > > PLATFORM      -- Linux/x86_64 kvm-xfstests
> > > > 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> > > > Jun 13 18:16:59 IDT 2023
> > > > MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> > > > MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> > > >
> > > > generic/572        [06:04:42] [06:04:47] [not run]
> > > > generic/572 -- ext4 verity isn't usable by default with these mkfs options
> > > > ...
> > >
> > > You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).
> > >
> >
> > That was indeed missing in my setup, but it did not fix the problem.
> >
> > Turned out that I had a very old version of fsverity installed in my
> > kvm-xfstest test VM, where there is no --block-size option to
> > fsverity enable would always fail.
> >
> > Eric,
> >
> > You may consider adding a check for minimal version of
> > fsverity or check for support of --block-size option to make
> > this error reason more clear for testers.
> >
> > Ted,
> >
> > FYI, FSVERITY_GIT in fstests-bld/config points to an out of date URL
> >
> > How come there is no ext4/cfg/verity in fstests-bld?
> > Are you guys not testing fsverity with fstests-bld?
> > Should we just add fsverity config or add verity to ext4/cfg/encrypt
> > instead to avoid growing the test matrix?
> >
> > I can send patches for fstests-bld fixing the above if you like.
> >
> > Alex,
> >
> > Verified that your verity-tests2 work as expected with v5 patches.
>
> To be honest I have not validated that my changes to the shared verity
> code still works with the non-overlayfs tests. If you have a setup for
> it it would be great if you could try the regular ext4 w/ fs-veriy
> tests on top of the verity-test2 branch.
>

There is no problem with "./check -g verity" on ext4
those tests pass.

However, "./check -overlay -g generic/verity" fails several test:
Failures: generic/572 generic/573 generic/574 generic/575 generic/577
because _require_scratch_verity falsely claims that overlay (over ext4)
supports verify, but then FS_IOC_ENABLE_VERITY actually fails
during the test.

Instead of changing _require_scratch_verity() as you did,
you should consider passing optional arguments, e.g.:
  local fstyp=${1:-$FSTYP}
and calling it from _require_scratch_overlay_verity() with the
$OVL_BASE_* values.

Thanks,
Amir.

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

[Eric Biggers]

Hi Amir,

On Thu, Jun 22, 2023 at 12:36:59PM +0300, Amir Goldstein wrote:
> > > What do I need to do in order to enable verity on ext4 besides
> > > enabling FS_VERITY in the kernel?
> > >
> > > I'm getting these on verity tests on ext4 in the default 4k config.
> > > _require_scratch_verity() doesn't mention any requirement other
> > > that 4K blocks and extent format files.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> > > DEVICE: /dev/vdb
> > > EXT_MKFS_OPTIONS: -b 4096
> > > EXT_MOUNT_OPTIONS: -o block_validity
> > > FSTYP         -- ext4
> > > PLATFORM      -- Linux/x86_64 kvm-xfstests
> > > 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> > > Jun 13 18:16:59 IDT 2023
> > > MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> > > MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> > >
> > > generic/572        [06:04:42] [06:04:47] [not run]
> > > generic/572 -- ext4 verity isn't usable by default with these mkfs options
> > > ...
> >
> > You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).
> >
> 
> That was indeed missing in my setup, but it did not fix the problem.
> 
> Turned out that I had a very old version of fsverity installed in my
> kvm-xfstest test VM, where there is no --block-size option to
> fsverity enable would always fail.

That would do it.  So the tests were in fact skipping themselves as expected;
just the skip message was not clear.

> 
> Eric,
> 
> You may consider adding a check for minimal version of
> fsverity or check for support of --block-size option to make
> this error reason more clear for testers.

I'm thinking it wouldn't be worth bothering, as the --block-size option was in
the first actual release of fsverity-utils (v1.0).  You must have pulled down a
work-in-progress version of fsverity-utils back in 2018 or early 2019, well
before the kernel support for fsverity was upstreamed, and then never updated
it.  I expect this issue affects very few people.

> Ted,
> 
> FYI, FSVERITY_GIT in fstests-bld/config points to an out of date URL

It was already updated several months ago.  Please run 'git pull'.

> 
> How come there is no ext4/cfg/verity in fstests-bld?

xfstests has a verity test *group*, which contains the tests that were written
specifically to test verity.

An xfstests-bld test config is something that applies to all tests.  Features
like "encrypt" have an xfstests-bld test config since there is a way to
enable/use those features in all tests.  In the case of encrypt that means
mounting the filesystem with "-o test_dummy_encryption".

In the case of verity, there is no way to enable/use verity in all tests.  (It
would be possible to always enable "-O verity" on the filesystem, but that does
nothing to make verity actually be used/tested.)  Hence, it doesn't have an
xfstests-bld test config.

> Are you guys not testing fsverity with fstests-bld?

We are.  The documented way to test fsverity is to use kvm-xfstests:
https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#tests

> Should we just add fsverity config or add verity to ext4/cfg/encrypt
> instead to avoid growing the test matrix?
> 
> I can send patches for fstests-bld fixing the above if you like.

As per the above, I don't think there is anything to fix.

- Eric

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

[Amir Goldstein]

On Thu, Jun 22, 2023 at 7:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi Amir,
>
> On Thu, Jun 22, 2023 at 12:36:59PM +0300, Amir Goldstein wrote:
> > > > What do I need to do in order to enable verity on ext4 besides
> > > > enabling FS_VERITY in the kernel?
> > > >
> > > > I'm getting these on verity tests on ext4 in the default 4k config.
> > > > _require_scratch_verity() doesn't mention any requirement other
> > > > that 4K blocks and extent format files.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > > > BEGIN TEST 4k (10 tests): Ext4 4k block Wed Jun 14 06:04:25 UTC 2023
> > > > DEVICE: /dev/vdb
> > > > EXT_MKFS_OPTIONS: -b 4096
> > > > EXT_MOUNT_OPTIONS: -o block_validity
> > > > FSTYP         -- ext4
> > > > PLATFORM      -- Linux/x86_64 kvm-xfstests
> > > > 6.4.0-rc2-xfstests-00026-g35774ba7f07c #1596 SMP PREEMPT_DYNAMIC Tue
> > > > Jun 13 18:16:59 IDT 2023
> > > > MKFS_OPTIONS  -- -F -q -b 4096 /dev/vdc
> > > > MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> > > >
> > > > generic/572        [06:04:42] [06:04:47] [not run]
> > > > generic/572 -- ext4 verity isn't usable by default with these mkfs options
> > > > ...
> > >
> > > You need to "tune2fs -O verity" (or pass -O verity to mkfs.ext4).
> > >
> >
> > That was indeed missing in my setup, but it did not fix the problem.
> >
> > Turned out that I had a very old version of fsverity installed in my
> > kvm-xfstest test VM, where there is no --block-size option to
> > fsverity enable would always fail.
>
> That would do it.  So the tests were in fact skipping themselves as expected;
> just the skip message was not clear.
>

Right.

> >
> > Eric,
> >
> > You may consider adding a check for minimal version of
> > fsverity or check for support of --block-size option to make
> > this error reason more clear for testers.
>
> I'm thinking it wouldn't be worth bothering, as the --block-size option was in
> the first actual release of fsverity-utils (v1.0).  You must have pulled down a
> work-in-progress version of fsverity-utils back in 2018 or early 2019, well
> before the kernel support for fsverity was upstreamed, and then never updated
> it.  I expect this issue affects very few people.
>

Agree.

> > Ted,
> >
> > FYI, FSVERITY_GIT in fstests-bld/config points to an out of date URL
>
> It was already updated several months ago.  Please run 'git pull'.
>

I thought I did. My bad.
Sorry for the noise.

> >
> > How come there is no ext4/cfg/verity in fstests-bld?
>
> xfstests has a verity test *group*, which contains the tests that were written
> specifically to test verity.
>
> An xfstests-bld test config is something that applies to all tests.  Features
> like "encrypt" have an xfstests-bld test config since there is a way to
> enable/use those features in all tests.  In the case of encrypt that means
> mounting the filesystem with "-o test_dummy_encryption".
>
> In the case of verity, there is no way to enable/use verity in all tests.  (It
> would be possible to always enable "-O verity" on the filesystem, but that does
> nothing to make verity actually be used/tested.)  Hence, it doesn't have an
> xfstests-bld test config.
>

Got it.

In case of overlayfs over ext4 (i.e. -c ext4:overlay/small) it is needed
to format ext4 in the beginning with -O verity because overlayfs
tests do not currently have support for formatting the base fs differently
per test.

This is the patch I intend to post.
Shout it you disagree:

commit 28d65d957c25eab85eb80b06724b215770a734a1
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Thu Jun 22 11:30:06 2023 +0300

    test-appliance: enable verity for testing overlay over ext4

    Add -O verity for ext4 formatted for overlay tests, so that overlay
    verity feature could be tested.

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

diff --git a/test-appliance/files/root/fs/overlay/config
b/test-appliance/files/root/fs/overlay/config
index 7c50b19..f252a70 100644
--- a/test-appliance/files/root/fs/overlay/config
+++ b/test-appliance/files/root/fs/overlay/config
@@ -55,7 +55,7 @@ function __mkfs()

        case "$BASE_FSTYPE" in
            ext4)
-               /sbin/mke2fs -F -q -t ext4 "$dev"
+               /sbin/mke2fs -F -q -t ext4 -O verity "$dev"
                ;;
            xfs)
                mkfs.xfs -f -m rmapbt=1,reflink=1 "$dev"


> > Are you guys not testing fsverity with fstests-bld?
>
> We are.  The documented way to test fsverity is to use kvm-xfstests:
> https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#tests
>
> > Should we just add fsverity config or add verity to ext4/cfg/encrypt
> > instead to avoid growing the test matrix?
> >
> > I can send patches for fstests-bld fixing the above if you like.
>
> As per the above, I don't think there is anything to fix.
>

Right.
Expect for the patch above which is needed for running Alex's
-g overlay/verity test

Thanks,
Amir.

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

[Amir Goldstein]

On Thu, Jun 22, 2023 at 2:45 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Jun 22, 2023 at 12:52 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 11:37 AM Amir Goldstein <amir73il@gmail.com> wrote:
...
> > > Alex,
> > >
> > > Verified that your verity-tests2 work as expected with v5 patches.
> >
> > To be honest I have not validated that my changes to the shared verity
> > code still works with the non-overlayfs tests. If you have a setup for
> > it it would be great if you could try the regular ext4 w/ fs-veriy
> > tests on top of the verity-test2 branch.
> >
>
> There is no problem with "./check -g verity" on ext4
> those tests pass.
>
> However, "./check -overlay -g generic/verity" fails several test:
> Failures: generic/572 generic/573 generic/574 generic/575 generic/577
> because _require_scratch_verity falsely claims that overlay (over ext4)
> supports verify, but then FS_IOC_ENABLE_VERITY actually fails
> during the test.
>
> Instead of changing _require_scratch_verity() as you did,
> you should consider passing optional arguments, e.g.:
>   local fstyp=${1:-$FSTYP}
> and calling it from _require_scratch_overlay_verity() with the
> $OVL_BASE_* values.
>

FWIW, I pushed this solution to
https://github.com/amir73il/xfstests/commits/verity-tests

It's ugly, but it works.

Thanks,
Amir.

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

[Alexander Larsson]

Cool, I wanted to look at this, but was on PTO last week.
It looks good to me, and I synced this to:
  https://github.com/alexlarsson/xfstests/commits/verity-tests
To avoid drift.

On Mon, Jun 26, 2023 at 3:14 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Jun 22, 2023 at 2:45 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 12:52 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Thu, Jun 22, 2023 at 11:37 AM Amir Goldstein <amir73il@gmail.com> wrote:
> ...
> > > > Alex,
> > > >
> > > > Verified that your verity-tests2 work as expected with v5 patches.
> > >
> > > To be honest I have not validated that my changes to the shared verity
> > > code still works with the non-overlayfs tests. If you have a setup for
> > > it it would be great if you could try the regular ext4 w/ fs-veriy
> > > tests on top of the verity-test2 branch.
> > >
> >
> > There is no problem with "./check -g verity" on ext4
> > those tests pass.
> >
> > However, "./check -overlay -g generic/verity" fails several test:
> > Failures: generic/572 generic/573 generic/574 generic/575 generic/577
> > because _require_scratch_verity falsely claims that overlay (over ext4)
> > supports verify, but then FS_IOC_ENABLE_VERITY actually fails
> > during the test.
> >
> > Instead of changing _require_scratch_verity() as you did,
> > you should consider passing optional arguments, e.g.:
> >   local fstyp=${1:-$FSTYP}
> > and calling it from _require_scratch_overlay_verity() with the
> > $OVL_BASE_* values.
> >
>
> FWIW, I pushed this solution to
> https://github.com/amir73il/xfstests/commits/verity-tests
>
> It's ugly, but it works.
>
> Thanks,
> Amir.
>


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

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

[Amir Goldstein]

On Mon, Jul 3, 2023 at 11:11 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> Cool, I wanted to look at this, but was on PTO last week.
> It looks good to me, and I synced this to:
>   https://github.com/alexlarsson/xfstests/commits/verity-tests
> To avoid drift.
>

I pushed the overlay-verity series to overlayfs-next, so you may
expect "finishing touches" bug reports from bots in the near future.
As long as they are minor fixes, you can fix your branch and let me know
and I will update overlayfs-next.

Miklos may yet have feedback on the final version, but I think all his
comments were addressed including the ACK from Eric (thanks!).

Eric,

There was no posting of v5 that addressed your v4 review comments,
so we do not have your RVB/ACK for patches 2-4.
Let me know if you want me to add that to the patches.

Alex,

wrt overlay verity-tests, please submit those tests along with the lowerdata
tests to fstests anytime between now and the 6.6 merge window.
The tests are properly equipped to check for the feature and testers can
use them to test linux-next.

For test overlay/080 you may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
For test overlay/079 you may retain my authorship or assume authorship
I don't mind as it was co-authored and you took it to the finish line.

Thanks,
Amir.

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

[Alexander Larsson]

On Thu, Jul 6, 2023 at 9:10 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jul 3, 2023 at 11:11 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > Cool, I wanted to look at this, but was on PTO last week.
> > It looks good to me, and I synced this to:
> >   https://github.com/alexlarsson/xfstests/commits/verity-tests
> > To avoid drift.
> >
>
> I pushed the overlay-verity series to overlayfs-next, so you may
> expect "finishing touches" bug reports from bots in the near future.
> As long as they are minor fixes, you can fix your branch and let me know
> and I will update overlayfs-next.
>
> Miklos may yet have feedback on the final version, but I think all his
> comments were addressed including the ACK from Eric (thanks!).

Nice!

> Eric,
>
> There was no posting of v5 that addressed your v4 review comments,
> so we do not have your RVB/ACK for patches 2-4.
> Let me know if you want me to add that to the patches.
>
> Alex,
>
> wrt overlay verity-tests, please submit those tests along with the lowerdata
> tests to fstests anytime between now and the 6.6 merge window.
> The tests are properly equipped to check for the feature and testers can
> use them to test linux-next.

I'll have a look at this soon.

> For test overlay/080 you may add:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> For test overlay/079 you may retain my authorship or assume authorship
> I don't mind as it was co-authored and you took it to the finish line.
>
> Thanks,
> Amir.
>


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