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

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

[Alexander Larsson]

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

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

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

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

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

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

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

 Documentation/filesystems/overlayfs.rst |  33 +++++
 fs/overlayfs/Kconfig                    |  14 ++
 fs/overlayfs/copy_up.c                  |  27 ++++
 fs/overlayfs/namei.c                    |  34 +++++
 fs/overlayfs/overlayfs.h                |  11 ++
 fs/overlayfs/ovl_entry.h                |   4 +
 fs/overlayfs/super.c                    |  49 +++++++
 fs/overlayfs/util.c                     | 167 ++++++++++++++++++++++--
 fs/verity/measure.c                     |   1 +
 9 files changed, 332 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 1/6] fsverity: Export fsverity_get_digest

[Alexander Larsson]

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

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

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

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

[Alexander Larsson]

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

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

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4e327665c316..477008186d18 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -395,6 +395,8 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 void ovl_i_path_real(struct inode *inode, struct path *path);
+void ovl_entry_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
+			 struct dentry *upperdentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 9a042768013e..77c954591daa 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -351,19 +351,30 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
 	return ovl_upperdentry_dereference(OVL_I(inode));
 }
 
-void ovl_i_path_real(struct inode *inode, struct path *path)
-{
-	struct ovl_path *lowerstack = ovl_lowerstack(OVL_I_E(inode));
+void ovl_entry_path_real(struct ovl_fs *ofs,
+			 struct ovl_entry *oe,
+			 struct dentry *upperdentry,
+			 struct path *path)
+{
+	if (upperdentry) {
+		path->dentry = upperdentry;
+		path->mnt = ovl_upper_mnt(ofs);
+	} else {
+		struct ovl_path *lowerstack = ovl_lowerstack(oe);
 
-	path->dentry = ovl_i_dentry_upper(inode);
-	if (!path->dentry) {
 		path->dentry = lowerstack->dentry;
 		path->mnt = lowerstack->layer->mnt;
-	} else {
-		path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
 	}
 }
 
+void ovl_i_path_real(struct inode *inode, struct path *path)
+{
+	ovl_entry_path_real(OVL_FS(inode->i_sb),
+			    OVL_I_E(inode),
+			    ovl_i_dentry_upper(inode),
+			    path);
+}
+
 struct inode *ovl_inode_upper(struct inode *inode)
 {
 	struct dentry *upperdentry = ovl_i_dentry_upper(inode);
-- 
2.39.2

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

[Alexander Larsson]

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

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 477008186d18..3d14770dc711 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -395,6 +395,7 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 void ovl_i_path_real(struct inode *inode, struct path *path);
+void ovl_entry_path_lowerdata(struct ovl_entry *oe, struct path *path);
 void ovl_entry_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
 			 struct dentry *upperdentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 77c954591daa..17eff3e31239 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -242,9 +242,9 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 	}
 }
 
-void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
+void ovl_entry_path_lowerdata(struct ovl_entry *oe,
+			      struct path *path)
 {
-	struct ovl_entry *oe = OVL_E(dentry);
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
 	struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
 
@@ -262,6 +262,13 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 	}
 }
 
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
+{
+	struct ovl_entry *oe = OVL_E(dentry);
+
+	return ovl_entry_path_lowerdata(oe, path);
+}
+
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path)
 {
 	enum ovl_path_type type = ovl_path_type(dentry);
-- 
2.39.2

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

[Alexander Larsson]

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

Unless you explicitly disable it ("verity=off") all existing xattrs
are validated before use. This is all that happens by default
("verity=validate"), but, if you turn on verity ("verity=on") then
during metacopy we generate verity xattr in the upper metacopy file if
the source file has verity enabled. This means later accesses can
guarantee that the correct data is used.

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

Actual implementation follows in a separate commit.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 Documentation/filesystems/overlayfs.rst | 33 +++++++++++++++++
 fs/overlayfs/Kconfig                    | 14 +++++++
 fs/overlayfs/ovl_entry.h                |  4 ++
 fs/overlayfs/super.c                    | 49 +++++++++++++++++++++++++
 4 files changed, 100 insertions(+)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index c8e04a4f0e21..66895bf71cd1 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -403,6 +403,39 @@ 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. If enabled, overlayfs can also set this xattr
+during metadata copy up.
+
+This is controlled by the "verity" mount option, which supports
+these values:
+
+- "off":
+    The verity xattr is never used.
+- "validate":
+    Whenever a metacopy files specifies an expected digest, the
+    corresponding data file must match the specified digest.
+- "on":
+    Same as validate, but additionally, 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. Additionally metadata copy up will only be used if
+    the data file has fs-verity enabled, otherwise a full copy-up is
+    used.
+
+There are two ways to tune the default behaviour. The kernel config
+option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
+either of these are enabled, then verity mode is "on" by default,
+otherwise it is "validate".
+
 Sharing and copying layers
 --------------------------
 
diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 6708e54b0e30..98d6b1a7baf5 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -124,3 +124,17 @@ config OVERLAY_FS_METACOPY
 	  that doesn't support this feature will have unexpected results.
 
 	  If unsure, say N.
+
+config OVERLAY_FS_VERITY
+	bool "Overlayfs: turn on verity feature by default"
+	depends on OVERLAY_FS
+	depends on OVERLAY_FS_METACOPY
+	help
+	  If this config option is enabled then overlay filesystems will
+	  try to copy fs-verity digests from the lower file into the
+	  metacopy file at metadata copy-up time. It is still possible
+	  to turn off this feature globally with the "verity=off"
+	  module option or on a filesystem instance basis with the
+	  "verity=off" or "verity=validate" mount option.
+
+	  If unsure, say N.
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index a7b1006c5321..f759e476dfc7 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -13,6 +13,10 @@ struct ovl_config {
 	bool redirect_dir;
 	bool redirect_follow;
 	const char *redirect_mode;
+	bool verity_validate;
+	bool verity_generate;
+	bool verity_require;
+	const char *verity_mode;
 	bool index;
 	bool uuid;
 	bool nfs_export;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ef78abc21998..953d76f6a1e3 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -59,6 +59,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
 MODULE_PARM_DESC(metacopy,
 		 "Default to on or off for the metadata only copy up feature");
 
+static bool ovl_verity_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERITY);
+module_param_named(verity, ovl_verity_def, bool, 0644);
+MODULE_PARM_DESC(verity,
+		 "Default to on or validate for the metadata only copy up feature");
+
 static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode)
 {
@@ -235,6 +240,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);
@@ -325,6 +331,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 ovl_verity_def ? "on" : "validate";
+}
+
 static const char * const ovl_xino_str[] = {
 	"off",
 	"auto",
@@ -374,6 +385,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;
 }
 
@@ -429,6 +442,7 @@ enum {
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
 	OPT_VOLATILE,
+	OPT_VERITY,
 	OPT_ERR,
 };
 
@@ -451,6 +465,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}
 };
 
@@ -500,6 +515,25 @@ 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, "validate") == 0) {
+		config->verity_validate = true;
+	} else if (strcmp(mode, "on") == 0) {
+		config->verity_validate = true;
+		config->verity_generate = true;
+	} else if (strcmp(mode, "require") == 0) {
+		config->verity_validate = true;
+		config->verity_generate = true;
+		config->verity_require = 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;
@@ -511,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];
@@ -611,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);
@@ -642,6 +687,10 @@ 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;
+
 	/*
 	 * This is to make the logic below simpler.  It doesn't make any other
 	 * difference, since config->redirect_dir is only used for upper.
-- 
2.39.2

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

[Alexander Larsson]

When resolving lowerdata (lazily or non-lazily) we chech 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).

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/namei.c     | 34 ++++++++++++++
 fs/overlayfs/overlayfs.h |  6 +++
 fs/overlayfs/util.c      | 97 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ba2b156162ca..49f3715c582d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 /* Lazy lookup of lowerdata */
 int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 {
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct inode *inode = d_inode(dentry);
 	const char *redirect = ovl_lowerdata_redirect(inode);
 	struct ovl_path datapath = {};
@@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	if (err)
 		goto out_err;
 
+	if (ofs->config.verity_validate) {
+		struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, };
+		struct path metapath = {};
+
+		ovl_path_real(dentry, &metapath);
+		if (!metapath.dentry) {
+			err = -EIO;
+			goto out_err;
+		}
+
+		err = ovl_validate_verity(ofs, &metapath, &data);
+		if (err)
+			goto out_err;
+	}
+
 	err = ovl_dentry_set_lowerdata(dentry, &datapath);
 	if (err)
 		goto out_err;
@@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto out_put;
 
+	/* Validate verity of lower-data */
+	if (ofs->config.verity_validate &&
+	    !d.is_dir && (uppermetacopy || ctr > 1)) {
+		struct path datapath;
+
+		ovl_entry_path_lowerdata(&oe, &datapath);
+
+		/* Is NULL for lazy lookup, will be verified later */
+		if (datapath.dentry) {
+			struct path metapath;
+
+			ovl_entry_path_real(ofs, &oe, upperdentry, &metapath);
+			err = ovl_validate_verity(ofs, &metapath, &datapath);
+			if (err < 0)
+				goto out_free_oe;
+		}
+	}
+
 	if (upperopaque)
 		ovl_dentry_set_opaque(dentry);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3d14770dc711..b1d639ccd5ac 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 {
@@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
 int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
 bool ovl_is_metacopy_dentry(struct dentry *dentry);
 char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
+int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
+			 u8 *digest_buf, int *buf_length);
+int ovl_validate_verity(struct ovl_fs *ofs,
+			struct path *metapath,
+			struct path *datapath);
 int ovl_sync_status(struct ovl_fs *ofs);
 
 static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 17eff3e31239..55e90aa0978a 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>
@@ -742,6 +744,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, \
@@ -756,6 +759,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,
@@ -1188,6 +1192,99 @@ 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;
+}
+
+static int ovl_ensure_verity_loaded(struct ovl_fs *ofs,
+				    struct path *datapath)
+{
+	struct inode *inode = d_inode(datapath->dentry);
+	const struct fsverity_info *vi;
+	const struct cred *old_cred;
+	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.
+		 */
+		old_cred = override_creds(ofs->creator_cred);
+		filp = dentry_open(datapath, O_RDONLY, current_cred());
+		revert_creds(old_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 required_digest[FS_VERITY_MAX_DIGEST_SIZE];
+	u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
+	enum hash_algo verity_algo;
+	int digest_len;
+	int err;
+
+	if (!ofs->config.verity_validate ||
+	    /* Verity only works on regular files */
+	    !S_ISREG(d_inode(metapath->dentry)->i_mode))
+		return 0;
+
+	digest_len = sizeof(required_digest);
+	err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len);
+	if (err == -ENODATA) {
+		if (ofs->config.verity_require) {
+			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(ofs, 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 (digest_len != hash_digest_size[verity_algo] ||
+	    memcmp(required_digest, actual_digest, digest_len) != 0) {
+		pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
+				    datapath->dentry);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 /*
  * ovl_sync_status() - Check fs sync status for volatile mounts
  *
-- 
2.39.2

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

[Alexander Larsson]

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

During real data copy up, remove any old overlay.verity xattr.

If verity is required, and lowerdata does not have fs-verity enabled,
fall back to full copy-up (or the generated metacopy would not validate).

Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
 fs/overlayfs/copy_up.c   | 27 +++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/util.c      | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index eb266fb68730..a5c3862911d1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -19,6 +19,7 @@
 #include <linux/fdtable.h>
 #include <linux/ratelimit.h>
 #include <linux/exportfs.h>
+#include <linux/fsverity.h>
 #include "overlayfs.h"
 
 #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -644,6 +645,18 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	if (c->metacopy) {
 		err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
 					 NULL, 0, -EOPNOTSUPP);
+
+		/* Copy the verity digest if any so we can validate the copy-up later */
+		if (!err) {
+			struct path lowerdatapath;
+
+			ovl_path_lowerdata(c->dentry, &lowerdatapath);
+			if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
+				err = -EIO;
+			else
+				err = ovl_set_verity_xattr_from(ofs, temp, &lowerdatapath);
+		}
+
 		if (err)
 			return err;
 	}
@@ -919,6 +932,15 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 	if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
 		return false;
 
+	/* Fall back to full copy if no fsverity on source data and we require verity */
+	if (ofs->config.verity_require) {
+		struct dentry *lowerdata = ovl_dentry_lowerdata(dentry);
+
+		if (WARN_ON_ONCE(lowerdata == NULL) ||
+		    !fsverity_get_info(d_inode(lowerdata)))
+			return false;
+	}
+
 	return true;
 }
 
@@ -985,6 +1007,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 b1d639ccd5ac..710dd816518f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -473,6 +473,8 @@ int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
 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 55e90aa0978a..2bd9c9e68bf4 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1285,6 +1285,42 @@ 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[FS_VERITY_MAX_DIGEST_SIZE];
+	enum hash_algo verity_algo;
+
+	if (!ofs->config.verity_generate || !S_ISREG(d_inode(dst)->i_mode))
+		return 0;
+
+	err = -EIO;
+	if (src) {
+		err = ovl_ensure_verity_loaded(ofs, 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, &verity_algo);
+	}
+	if (err == -ENODATA) {
+		if (ofs->config.verity_require) {
+			pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
+					    src->dentry);
+			return -EIO;
+		}
+		return 0;
+	}
+	if (err < 0)
+		return err;
+
+	return ovl_check_setxattr(ofs, dst, OVL_XATTR_VERITY,
+				  src_digest, hash_digest_size[verity_algo], -EOPNOTSUPP);
+}
+
 /*
  * ovl_sync_status() - Check fs sync status for volatile mounts
  *
-- 
2.39.2

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

[Amir Goldstein]

On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> This adds the scaffolding (docs, config, mount options) for supporting
> for a new overlay xattr "overlay.verity", which contains a fs-verity
> digest. This is used for metacopy files, and the actual fs-verity
> digest of the lowerdata file needs to match it. The mount option
> "verity" specifies how this xattrs is handled.
>
> Unless you explicitly disable it ("verity=off") all existing xattrs
> are validated before use. This is all that happens by default
> ("verity=validate"), but, if you turn on verity ("verity=on") then
> during metacopy we generate verity xattr in the upper metacopy file if
> the source file has verity enabled. This means later accesses can
> guarantee that the correct data is used.
>
> Additionally you can use "verity=require". In this mode all metacopy
> files must have a valid verity xattr. For this to work metadata
> copy-up must be able to create a verity xattr (so that later accesses
> are validated). Therefore, in this mode, if the lower data file
> doesn't have fs-verity enabled we fall back to a full copy rather than
> a metacopy.
>
> Actual implementation follows in a separate commit.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst | 33 +++++++++++++++++
>  fs/overlayfs/Kconfig                    | 14 +++++++
>  fs/overlayfs/ovl_entry.h                |  4 ++
>  fs/overlayfs/super.c                    | 49 +++++++++++++++++++++++++
>  4 files changed, 100 insertions(+)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index c8e04a4f0e21..66895bf71cd1 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -403,6 +403,39 @@ 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. If enabled, overlayfs can also set this xattr
> +during metadata copy up.
> +
> +This is controlled by the "verity" mount option, which supports
> +these values:
> +
> +- "off":
> +    The verity xattr is never used.
> +- "validate":
> +    Whenever a metacopy files specifies an expected digest, the
> +    corresponding data file must match the specified digest.
> +- "on":
> +    Same as validate, but additionally, 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. Additionally metadata copy up will only be used if
> +    the data file has fs-verity enabled, otherwise a full copy-up is
> +    used.
> +
> +There are two ways to tune the default behaviour. The kernel config
> +option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
> +either of these are enabled, then verity mode is "on" by default,
> +otherwise it is "validate".
> +
>  Sharing and copying layers
>  --------------------------
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 6708e54b0e30..98d6b1a7baf5 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -124,3 +124,17 @@ config OVERLAY_FS_METACOPY
>           that doesn't support this feature will have unexpected results.
>
>           If unsure, say N.
> +
> +config OVERLAY_FS_VERITY
> +       bool "Overlayfs: turn on verity feature by default"
> +       depends on OVERLAY_FS
> +       depends on OVERLAY_FS_METACOPY
> +       help
> +         If this config option is enabled then overlay filesystems will
> +         try to copy fs-verity digests from the lower file into the
> +         metacopy file at metadata copy-up time. It is still possible
> +         to turn off this feature globally with the "verity=off"
> +         module option or on a filesystem instance basis with the
> +         "verity=off" or "verity=validate" mount option.
> +
> +         If unsure, say N.
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index a7b1006c5321..f759e476dfc7 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -13,6 +13,10 @@ struct ovl_config {
>         bool redirect_dir;
>         bool redirect_follow;
>         const char *redirect_mode;
> +       bool verity_validate;
> +       bool verity_generate;
> +       bool verity_require;
> +       const char *verity_mode;
>         bool index;
>         bool uuid;
>         bool nfs_export;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ef78abc21998..953d76f6a1e3 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -59,6 +59,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
>  MODULE_PARM_DESC(metacopy,
>                  "Default to on or off for the metadata only copy up feature");
>
> +static bool ovl_verity_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERITY);
> +module_param_named(verity, ovl_verity_def, bool, 0644);
> +MODULE_PARM_DESC(verity,
> +                "Default to on or validate for the metadata only copy up feature");
> +
>  static struct dentry *ovl_d_real(struct dentry *dentry,
>                                  const struct inode *inode)
>  {
> @@ -235,6 +240,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);
> @@ -325,6 +331,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 ovl_verity_def ? "on" : "validate";
> +}
> +
>  static const char * const ovl_xino_str[] = {
>         "off",
>         "auto",
> @@ -374,6 +385,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;
>  }
>
> @@ -429,6 +442,7 @@ enum {
>         OPT_METACOPY_ON,
>         OPT_METACOPY_OFF,
>         OPT_VOLATILE,
> +       OPT_VERITY,
>         OPT_ERR,
>  };
>
> @@ -451,6 +465,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}
>  };
>
> @@ -500,6 +515,25 @@ 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, "validate") == 0) {
> +               config->verity_validate = true;
> +       } else if (strcmp(mode, "on") == 0) {
> +               config->verity_validate = true;
> +               config->verity_generate = true;
> +       } else if (strcmp(mode, "require") == 0) {
> +               config->verity_validate = true;
> +               config->verity_generate = true;
> +               config->verity_require = 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;
> @@ -511,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];
> @@ -611,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);
> @@ -642,6 +687,10 @@ 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;
> +
>         /*
>          * This is to make the logic below simpler.  It doesn't make any other
>          * difference, since config->redirect_dir is only used for upper.
>

Need to add code to resolve verity -> metacopy dependency
same as for metacopy -> redirect_dir dependency.

However, note that the nfs_export,userxattr -> !metacopy dependencies
also imply nfs_export,userxattr -> !verity.

But unlike metacopy, I am not sure of we can auto disable verity (?)
I guess it is fine to set veritfy=off *along with* metacopy=off because
trying to follow a metacopy would result in EIO anyway when metacopy
is disabled, so the verity mode is meaningless.

Thanks,
Amir.

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

[Amir Goldstein]

ovl_maybe_lookup_lowerdata

On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> When resolving lowerdata (lazily or non-lazily) we chech 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).
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
>  fs/overlayfs/namei.c     | 34 ++++++++++++++
>  fs/overlayfs/overlayfs.h |  6 +++
>  fs/overlayfs/util.c      | 97 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index ba2b156162ca..49f3715c582d 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
>  /* Lazy lookup of lowerdata */
>  int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
>  {
> +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>         struct inode *inode = d_inode(dentry);
>         const char *redirect = ovl_lowerdata_redirect(inode);
>         struct ovl_path datapath = {};
> @@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
>         if (err)
>                 goto out_err;
>
> +       if (ofs->config.verity_validate) {
> +               struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, };
> +               struct path metapath = {};
> +
> +               ovl_path_real(dentry, &metapath);
> +               if (!metapath.dentry) {
> +                       err = -EIO;
> +                       goto out_err;
> +               }
> +
> +               err = ovl_validate_verity(ofs, &metapath, &data);
> +               if (err)
> +                       goto out_err;
> +       }
> +
>         err = ovl_dentry_set_lowerdata(dentry, &datapath);
>         if (err)
>                 goto out_err;
> @@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         if (err)
>                 goto out_put;
>
> +       /* Validate verity of lower-data */
> +       if (ofs->config.verity_validate &&
> +           !d.is_dir && (uppermetacopy || ctr > 1)) {
> +               struct path datapath;
> +
> +               ovl_entry_path_lowerdata(&oe, &datapath);
> +
> +               /* Is NULL for lazy lookup, will be verified later */
> +               if (datapath.dentry) {
> +                       struct path metapath;
> +
> +                       ovl_entry_path_real(ofs, &oe, upperdentry, &metapath);
> +                       err = ovl_validate_verity(ofs, &metapath, &datapath);
> +                       if (err < 0)
> +                               goto out_free_oe;
> +               }
> +       }
> +
>         if (upperopaque)
>                 ovl_dentry_set_opaque(dentry);
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 3d14770dc711..b1d639ccd5ac 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 {
> @@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
>  bool ovl_is_metacopy_dentry(struct dentry *dentry);
>  char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
> +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> +                        u8 *digest_buf, int *buf_length);
> +int ovl_validate_verity(struct ovl_fs *ofs,
> +                       struct path *metapath,
> +                       struct path *datapath);
>  int ovl_sync_status(struct ovl_fs *ofs);
>
>  static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 17eff3e31239..55e90aa0978a 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>
> @@ -742,6 +744,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, \
> @@ -756,6 +759,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,
> @@ -1188,6 +1192,99 @@ 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;
> +}
> +
> +static int ovl_ensure_verity_loaded(struct ovl_fs *ofs,
> +                                   struct path *datapath)
> +{
> +       struct inode *inode = d_inode(datapath->dentry);
> +       const struct fsverity_info *vi;
> +       const struct cred *old_cred;
> +       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.
> +                */
> +               old_cred = override_creds(ofs->creator_cred);

Even though it may work, that's the wrong place for override_creds(),
because you are calling this helper from within ovl_lookup() with
mounter creds already.

Better to move revert_creds() in ovl_maybe_lookup_lowerdata()
to out_revert_creds: goto label and call ovl_validate_verity() with
mounter creds from all call sites.

> +               filp = dentry_open(datapath, O_RDONLY, current_cred());

You could use open_with_fake_path() here to avoid ENFILE
not sure if this is critical.

> +               revert_creds(old_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 required_digest[FS_VERITY_MAX_DIGEST_SIZE];
> +       u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
> +       enum hash_algo verity_algo;
> +       int digest_len;
> +       int err;
> +
> +       if (!ofs->config.verity_validate ||
> +           /* Verity only works on regular files */
> +           !S_ISREG(d_inode(metapath->dentry)->i_mode))
> +               return 0;
> +
> +       digest_len = sizeof(required_digest);
> +       err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len);
> +       if (err == -ENODATA) {
> +               if (ofs->config.verity_require) {
> +                       pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n",
> +                                           metapath->dentry);
> +                       return -EIO;
> +               }
> +               return 0;
> +       }

Thinking out loud: feels to me that verity xattr is a property that is
always "coupled" with metacopy xattr.

I wonder if we should check and store them together during lookup.

On one hand that means using a bit more memory per inode
before we need it.

OTOH, getxattr on metapath during lazy lookup may incur extra
IO to the metapath inode xattr block that will have been amortized
if done during lookup.

I have no strong feelings one way or the other.

Thanks,
Amir.

> +       if (err < 0)
> +               return err;
> +
> +       err = ovl_ensure_verity_loaded(ofs, 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 (digest_len != hash_digest_size[verity_algo] ||
> +           memcmp(required_digest, actual_digest, digest_len) != 0) {
> +               pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> +                                   datapath->dentry);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * ovl_sync_status() - Check fs sync status for volatile mounts
>   *
> --
> 2.39.2
>

Re: [PATCH 2/6] ovl: Break out ovl_entry_path_real() from ovl_i_path_real()

[Amir Goldstein]

On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> This allows us to get the real path from the ovl_entry in ovl_lookup()
> before having finished setting up the resulting inode.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
>  fs/overlayfs/overlayfs.h |  2 ++
>  fs/overlayfs/util.c      | 25 ++++++++++++++++++-------
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4e327665c316..477008186d18 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -395,6 +395,8 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
>  void ovl_path_lower(struct dentry *dentry, struct path *path);
>  void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>  void ovl_i_path_real(struct inode *inode, struct path *path);
> +void ovl_entry_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
> +                        struct dentry *upperdentry, struct path *path);
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>  enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 9a042768013e..77c954591daa 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -351,19 +351,30 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
>         return ovl_upperdentry_dereference(OVL_I(inode));
>  }
>
> -void ovl_i_path_real(struct inode *inode, struct path *path)
> -{
> -       struct ovl_path *lowerstack = ovl_lowerstack(OVL_I_E(inode));
> +void ovl_entry_path_real(struct ovl_fs *ofs,

Nit: I would use ovl_e_ prefix. Not critical.

Thanks,
Amir.

Re: [PATCH 3/6] ovl: Break out ovl_entry_path_lowerdata() from ovl_path_lowerdata()

[Amir Goldstein]

On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> This will be needed later when getting the lowerdata path from the
> ovl_entry in ovl_lookup() before the dentry is set up.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/util.c      | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 477008186d18..3d14770dc711 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -395,6 +395,7 @@ void ovl_path_upper(struct dentry *dentry, struct path *path);
>  void ovl_path_lower(struct dentry *dentry, struct path *path);
>  void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>  void ovl_i_path_real(struct inode *inode, struct path *path);
> +void ovl_entry_path_lowerdata(struct ovl_entry *oe, struct path *path);
>  void ovl_entry_path_real(struct ovl_fs *ofs, struct ovl_entry *oe,
>                          struct dentry *upperdentry, struct path *path);
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 77c954591daa..17eff3e31239 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -242,9 +242,9 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
>         }
>  }
>
> -void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
> +void ovl_entry_path_lowerdata(struct ovl_entry *oe,

Nit: I would use ovl_e_ prefix. Not critical.

> +                             struct path *path)
>  {
> -       struct ovl_entry *oe = OVL_E(dentry);
>         struct ovl_path *lowerdata = ovl_lowerdata(oe);
>         struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
>
> @@ -262,6 +262,13 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
>         }
>  }
>
> +void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
> +{
> +       struct ovl_entry *oe = OVL_E(dentry);
> +
> +       return ovl_entry_path_lowerdata(oe, path);

Nit: I wouldn't use a helper var here.

Otherwise you may add (also to the previous helper patch):

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

Thanks,
Amir.

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

[Amir Goldstein]

On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> During regular metacopy, if lowerdata file has fs-verity enabled,
> set the new overlay.verity xattr (if enabled).
>
> During real data copy up, remove any old overlay.verity xattr.
>
> If verity is required, and lowerdata does not have fs-verity enabled,
> fall back to full copy-up (or the generated metacopy would not validate).
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>

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

> ---
>  fs/overlayfs/copy_up.c   | 27 +++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  2 ++
>  fs/overlayfs/util.c      | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index eb266fb68730..a5c3862911d1 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -19,6 +19,7 @@
>  #include <linux/fdtable.h>
>  #include <linux/ratelimit.h>
>  #include <linux/exportfs.h>
> +#include <linux/fsverity.h>
>  #include "overlayfs.h"
>
>  #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
> @@ -644,6 +645,18 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
>         if (c->metacopy) {
>                 err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
>                                          NULL, 0, -EOPNOTSUPP);
> +
> +               /* Copy the verity digest if any so we can validate the copy-up later */
> +               if (!err) {
> +                       struct path lowerdatapath;
> +
> +                       ovl_path_lowerdata(c->dentry, &lowerdatapath);
> +                       if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> +                               err = -EIO;
> +                       else
> +                               err = ovl_set_verity_xattr_from(ofs, temp, &lowerdatapath);
> +               }
> +
>                 if (err)
>                         return err;
>         }
> @@ -919,6 +932,15 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
>         if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
>                 return false;
>
> +       /* Fall back to full copy if no fsverity on source data and we require verity */
> +       if (ofs->config.verity_require) {
> +               struct dentry *lowerdata = ovl_dentry_lowerdata(dentry);
> +
> +               if (WARN_ON_ONCE(lowerdata == NULL) ||
> +                   !fsverity_get_info(d_inode(lowerdata)))
> +                       return false;
> +       }
> +
>         return true;
>  }
>
> @@ -985,6 +1007,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 b1d639ccd5ac..710dd816518f 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -473,6 +473,8 @@ int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
>  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 55e90aa0978a..2bd9c9e68bf4 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1285,6 +1285,42 @@ 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[FS_VERITY_MAX_DIGEST_SIZE];
> +       enum hash_algo verity_algo;
> +
> +       if (!ofs->config.verity_generate || !S_ISREG(d_inode(dst)->i_mode))
> +               return 0;
> +
> +       err = -EIO;
> +       if (src) {
> +               err = ovl_ensure_verity_loaded(ofs, 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, &verity_algo);
> +       }
> +       if (err == -ENODATA) {
> +               if (ofs->config.verity_require) {
> +                       pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
> +                                           src->dentry);
> +                       return -EIO;
> +               }
> +               return 0;
> +       }
> +       if (err < 0)
> +               return err;
> +
> +       return ovl_check_setxattr(ofs, dst, OVL_XATTR_VERITY,
> +                                 src_digest, hash_digest_size[verity_algo], -EOPNOTSUPP);
> +}
> +
>  /*
>   * ovl_sync_status() - Check fs sync status for volatile mounts
>   *
> --
> 2.39.2
>

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

[Miklos Szeredi]

On Thu, 20 Apr 2023 at 09:44, Alexander Larsson <alexl@redhat.com> wrote:
>
> This adds the scaffolding (docs, config, mount options) for supporting
> for a new overlay xattr "overlay.verity", which contains a fs-verity
> digest. This is used for metacopy files, and the actual fs-verity
> digest of the lowerdata file needs to match it. The mount option
> "verity" specifies how this xattrs is handled.
>
> Unless you explicitly disable it ("verity=off") all existing xattrs
> are validated before use. This is all that happens by default
> ("verity=validate"), but, if you turn on verity ("verity=on") then
> during metacopy we generate verity xattr in the upper metacopy file if
> the source file has verity enabled. This means later accesses can
> guarantee that the correct data is used.
>
> Additionally you can use "verity=require". In this mode all metacopy
> files must have a valid verity xattr. For this to work metadata
> copy-up must be able to create a verity xattr (so that later accesses
> are validated). Therefore, in this mode, if the lower data file
> doesn't have fs-verity enabled we fall back to a full copy rather than
> a metacopy.

Maybe we can reduce the number of modes.  Which mode does your use case need?

>
> Actual implementation follows in a separate commit.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst | 33 +++++++++++++++++
>  fs/overlayfs/Kconfig                    | 14 +++++++
>  fs/overlayfs/ovl_entry.h                |  4 ++
>  fs/overlayfs/super.c                    | 49 +++++++++++++++++++++++++
>  4 files changed, 100 insertions(+)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index c8e04a4f0e21..66895bf71cd1 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -403,6 +403,39 @@ 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. If enabled, overlayfs can also set this xattr
> +during metadata copy up.
> +
> +This is controlled by the "verity" mount option, which supports
> +these values:
> +
> +- "off":
> +    The verity xattr is never used.
> +- "validate":
> +    Whenever a metacopy files specifies an expected digest, the
> +    corresponding data file must match the specified digest.
> +- "on":
> +    Same as validate, but additionally, 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. Additionally metadata copy up will only be used if
> +    the data file has fs-verity enabled, otherwise a full copy-up is
> +    used.
> +
> +There are two ways to tune the default behaviour. The kernel config
> +option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
> +either of these are enabled, then verity mode is "on" by default,
> +otherwise it is "validate".

I'm not sure that enabling verity by default is safe.  E.g. a script
mounts overalyfs but doesn't set the verity mount, since it's on by
default.  Then the script is moved to a different system where the
default is off, which will result in verity not being enabled, even
though that was not intended.  Is there an advantage to allowing to
change the default?  I know it's done for most of the overlayfs
options, but I think this is different.

> +
>  Sharing and copying layers
>  --------------------------
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 6708e54b0e30..98d6b1a7baf5 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -124,3 +124,17 @@ config OVERLAY_FS_METACOPY
>           that doesn't support this feature will have unexpected results.
>
>           If unsure, say N.
> +
> +config OVERLAY_FS_VERITY
> +       bool "Overlayfs: turn on verity feature by default"
> +       depends on OVERLAY_FS
> +       depends on OVERLAY_FS_METACOPY
> +       help
> +         If this config option is enabled then overlay filesystems will
> +         try to copy fs-verity digests from the lower file into the
> +         metacopy file at metadata copy-up time. It is still possible
> +         to turn off this feature globally with the "verity=off"
> +         module option or on a filesystem instance basis with the
> +         "verity=off" or "verity=validate" mount option.
> +
> +         If unsure, say N.
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index a7b1006c5321..f759e476dfc7 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -13,6 +13,10 @@ struct ovl_config {
>         bool redirect_dir;
>         bool redirect_follow;
>         const char *redirect_mode;
> +       bool verity_validate;
> +       bool verity_generate;
> +       bool verity_require;
> +       const char *verity_mode;
>         bool index;
>         bool uuid;
>         bool nfs_export;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ef78abc21998..953d76f6a1e3 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -59,6 +59,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
>  MODULE_PARM_DESC(metacopy,
>                  "Default to on or off for the metadata only copy up feature");
>
> +static bool ovl_verity_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERITY);
> +module_param_named(verity, ovl_verity_def, bool, 0644);
> +MODULE_PARM_DESC(verity,
> +                "Default to on or validate for the metadata only copy up feature");
> +
>  static struct dentry *ovl_d_real(struct dentry *dentry,
>                                  const struct inode *inode)
>  {
> @@ -235,6 +240,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);
> @@ -325,6 +331,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 ovl_verity_def ? "on" : "validate";
> +}
> +
>  static const char * const ovl_xino_str[] = {
>         "off",
>         "auto",
> @@ -374,6 +385,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;
>  }
>
> @@ -429,6 +442,7 @@ enum {
>         OPT_METACOPY_ON,
>         OPT_METACOPY_OFF,
>         OPT_VOLATILE,
> +       OPT_VERITY,
>         OPT_ERR,
>  };
>
> @@ -451,6 +465,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}
>  };
>
> @@ -500,6 +515,25 @@ 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, "validate") == 0) {
> +               config->verity_validate = true;
> +       } else if (strcmp(mode, "on") == 0) {
> +               config->verity_validate = true;
> +               config->verity_generate = true;
> +       } else if (strcmp(mode, "require") == 0) {
> +               config->verity_validate = true;
> +               config->verity_generate = true;
> +               config->verity_require = 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;
> @@ -511,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];
> @@ -611,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);
> @@ -642,6 +687,10 @@ 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;
> +
>         /*
>          * This is to make the logic below simpler.  It doesn't make any other
>          * difference, since config->redirect_dir is only used for upper.
> --
> 2.39.2
>

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

[Alexander Larsson]

On Tue, Apr 25, 2023 at 1:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 20 Apr 2023 at 09:44, Alexander Larsson <alexl@redhat.com> wrote:
> >
> > This adds the scaffolding (docs, config, mount options) for supporting
> > for a new overlay xattr "overlay.verity", which contains a fs-verity
> > digest. This is used for metacopy files, and the actual fs-verity
> > digest of the lowerdata file needs to match it. The mount option
> > "verity" specifies how this xattrs is handled.
> >
> > Unless you explicitly disable it ("verity=off") all existing xattrs
> > are validated before use. This is all that happens by default
> > ("verity=validate"), but, if you turn on verity ("verity=on") then
> > during metacopy we generate verity xattr in the upper metacopy file if
> > the source file has verity enabled. This means later accesses can
> > guarantee that the correct data is used.
> >
> > Additionally you can use "verity=require". In this mode all metacopy
> > files must have a valid verity xattr. For this to work metadata
> > copy-up must be able to create a verity xattr (so that later accesses
> > are validated). Therefore, in this mode, if the lower data file
> > doesn't have fs-verity enabled we fall back to a full copy rather than
> > a metacopy.
>
> Maybe we can reduce the number of modes.  Which mode does your use case need?

For composefs I typically always create images with full verity info
so they *can* be used with verity, but I want to allow using these
even on systems that don't support verity. So, at the very minimum
composefs needs "verity=off" (don't even try to verify anything) and
"verity=require" (ensure all redirects have a verity xattr and it is
valid).

These are kind of extremes though, and I think it makes sense to have
something in between where not everything has to be verified.
Currently we have both "validate" and "on". But maybe those could be
consolidated.

> >
> > Actual implementation follows in a separate commit.
> >
> > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > ---
> >  Documentation/filesystems/overlayfs.rst | 33 +++++++++++++++++
> >  fs/overlayfs/Kconfig                    | 14 +++++++
> >  fs/overlayfs/ovl_entry.h                |  4 ++
> >  fs/overlayfs/super.c                    | 49 +++++++++++++++++++++++++
> >  4 files changed, 100 insertions(+)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > index c8e04a4f0e21..66895bf71cd1 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -403,6 +403,39 @@ 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. If enabled, overlayfs can also set this xattr
> > +during metadata copy up.
> > +
> > +This is controlled by the "verity" mount option, which supports
> > +these values:
> > +
> > +- "off":
> > +    The verity xattr is never used.
> > +- "validate":
> > +    Whenever a metacopy files specifies an expected digest, the
> > +    corresponding data file must match the specified digest.
> > +- "on":
> > +    Same as validate, but additionally, 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. Additionally metadata copy up will only be used if
> > +    the data file has fs-verity enabled, otherwise a full copy-up is
> > +    used.
> > +
> > +There are two ways to tune the default behaviour. The kernel config
> > +option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
> > +either of these are enabled, then verity mode is "on" by default,
> > +otherwise it is "validate".
>
> I'm not sure that enabling verity by default is safe.  E.g. a script
> mounts overalyfs but doesn't set the verity mount, since it's on by
> default.  Then the script is moved to a different system where the
> default is off, which will result in verity not being enabled, even
> though that was not intended.  Is there an advantage to allowing to
> change the default?  I know it's done for most of the overlayfs
> options, but I think this is different.

I sort of agree, in particular because many filesystems still don't
support verity, or need it to be specifically enabled.
So, what about dropping "validate" and go with modes: "off, on,
require", where "off" is the default?

> > +
> >  Sharing and copying layers
> >  --------------------------
> >
> > diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> > index 6708e54b0e30..98d6b1a7baf5 100644
> > --- a/fs/overlayfs/Kconfig
> > +++ b/fs/overlayfs/Kconfig
> > @@ -124,3 +124,17 @@ config OVERLAY_FS_METACOPY
> >           that doesn't support this feature will have unexpected results.
> >
> >           If unsure, say N.
> > +
> > +config OVERLAY_FS_VERITY
> > +       bool "Overlayfs: turn on verity feature by default"
> > +       depends on OVERLAY_FS
> > +       depends on OVERLAY_FS_METACOPY
> > +       help
> > +         If this config option is enabled then overlay filesystems will
> > +         try to copy fs-verity digests from the lower file into the
> > +         metacopy file at metadata copy-up time. It is still possible
> > +         to turn off this feature globally with the "verity=off"
> > +         module option or on a filesystem instance basis with the
> > +         "verity=off" or "verity=validate" mount option.
> > +
> > +         If unsure, say N.
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index a7b1006c5321..f759e476dfc7 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -13,6 +13,10 @@ struct ovl_config {
> >         bool redirect_dir;
> >         bool redirect_follow;
> >         const char *redirect_mode;
> > +       bool verity_validate;
> > +       bool verity_generate;
> > +       bool verity_require;
> > +       const char *verity_mode;
> >         bool index;
> >         bool uuid;
> >         bool nfs_export;
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index ef78abc21998..953d76f6a1e3 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -59,6 +59,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
> >  MODULE_PARM_DESC(metacopy,
> >                  "Default to on or off for the metadata only copy up feature");
> >
> > +static bool ovl_verity_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERITY);
> > +module_param_named(verity, ovl_verity_def, bool, 0644);
> > +MODULE_PARM_DESC(verity,
> > +                "Default to on or validate for the metadata only copy up feature");
> > +
> >  static struct dentry *ovl_d_real(struct dentry *dentry,
> >                                  const struct inode *inode)
> >  {
> > @@ -235,6 +240,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);
> > @@ -325,6 +331,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 ovl_verity_def ? "on" : "validate";
> > +}
> > +
> >  static const char * const ovl_xino_str[] = {
> >         "off",
> >         "auto",
> > @@ -374,6 +385,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;
> >  }
> >
> > @@ -429,6 +442,7 @@ enum {
> >         OPT_METACOPY_ON,
> >         OPT_METACOPY_OFF,
> >         OPT_VOLATILE,
> > +       OPT_VERITY,
> >         OPT_ERR,
> >  };
> >
> > @@ -451,6 +465,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}
> >  };
> >
> > @@ -500,6 +515,25 @@ 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, "validate") == 0) {
> > +               config->verity_validate = true;
> > +       } else if (strcmp(mode, "on") == 0) {
> > +               config->verity_validate = true;
> > +               config->verity_generate = true;
> > +       } else if (strcmp(mode, "require") == 0) {
> > +               config->verity_validate = true;
> > +               config->verity_generate = true;
> > +               config->verity_require = 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;
> > @@ -511,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];
> > @@ -611,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);
> > @@ -642,6 +687,10 @@ 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;
> > +
> >         /*
> >          * This is to make the logic below simpler.  It doesn't make any other
> >          * difference, since config->redirect_dir is only used for upper.
> > --
> > 2.39.2
> >
>


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

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

[Miklos Szeredi]

On Tue, 25 Apr 2023 at 15:33, Alexander Larsson <alexl@redhat.com> wrote:
>
> On Tue, Apr 25, 2023 at 1:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, 20 Apr 2023 at 09:44, Alexander Larsson <alexl@redhat.com> wrote:

> > > +There are two ways to tune the default behaviour. The kernel config
> > > +option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
> > > +either of these are enabled, then verity mode is "on" by default,
> > > +otherwise it is "validate".
> >
> > I'm not sure that enabling verity by default is safe.  E.g. a script
> > mounts overalyfs but doesn't set the verity mount, since it's on by
> > default.  Then the script is moved to a different system where the
> > default is off, which will result in verity not being enabled, even
> > though that was not intended.  Is there an advantage to allowing to
> > change the default?  I know it's done for most of the overlayfs
> > options, but I think this is different.
>
> I sort of agree, in particular because many filesystems still don't
> support verity, or need it to be specifically enabled.
> So, what about dropping "validate" and go with modes: "off, on,
> require", where "off" is the default?

Okay.

Thanks,
Miklos

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

[Eric Biggers]

On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote:
> +	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 (digest_len != hash_digest_size[verity_algo] ||
> +	    memcmp(required_digest, actual_digest, digest_len) != 0) {
> +		pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> +				    datapath->dentry);
> +		return -EIO;
> +	}
> +
> +	return 0;

This is incorrect because the digest algorithm is not being compared.

- Eric

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

[Alexander Larsson]

On Wed, Apr 26, 2023 at 11:47 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote:
> > +     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 (digest_len != hash_digest_size[verity_algo] ||
> > +         memcmp(required_digest, actual_digest, digest_len) != 0) {
> > +             pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> > +                                 datapath->dentry);
> > +             return -EIO;
> > +     }
> > +
> > +     return 0;
>
> This is incorrect because the digest algorithm is not being compared.

This is actually an interesting question. How much are things weakened
by comparing the digest size, but not comparing the digest type. Like,
suppose the xattr has a sha256 digest (32 bytes), how likely is there
to be another new supported verity algorithm of the same digest size
where you can force it to produce matching digests?

I ask because ideally we want to minimize the size of the xattrs,
since they are stored for each file, and not having to specify the
type for each saves space. Currently the only two supported algorithms
(sha256 and sha512) are different sizes, so we essentially compare
type by comparing the size.

I see three options here:
1) Only compare digest + size (like now)
2) Assume size 32 means sha256, and 64 means sha512 and validate that
3) Use more space in the xattr to store an algorithm type

Maybe alternative 2 is the best option on balance, less extensible,
but safe and uses least space.
Opinions?

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

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

[Alexander Larsson]

On Thu, Apr 20, 2023 at 2:06 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> ovl_maybe_lookup_lowerdata
>
> On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > When resolving lowerdata (lazily or non-lazily) we chech 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).
> >
> > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > ---
> >  fs/overlayfs/namei.c     | 34 ++++++++++++++
> >  fs/overlayfs/overlayfs.h |  6 +++
> >  fs/overlayfs/util.c      | 97 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 137 insertions(+)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index ba2b156162ca..49f3715c582d 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
> >  /* Lazy lookup of lowerdata */
> >  int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> >  {
> > +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> >         struct inode *inode = d_inode(dentry);
> >         const char *redirect = ovl_lowerdata_redirect(inode);
> >         struct ovl_path datapath = {};
> > @@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> >         if (err)
> >                 goto out_err;
> >
> > +       if (ofs->config.verity_validate) {
> > +               struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, };
> > +               struct path metapath = {};
> > +
> > +               ovl_path_real(dentry, &metapath);
> > +               if (!metapath.dentry) {
> > +                       err = -EIO;
> > +                       goto out_err;
> > +               }
> > +
> > +               err = ovl_validate_verity(ofs, &metapath, &data);
> > +               if (err)
> > +                       goto out_err;
> > +       }
> > +
> >         err = ovl_dentry_set_lowerdata(dentry, &datapath);
> >         if (err)
> >                 goto out_err;
> > @@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         if (err)
> >                 goto out_put;
> >
> > +       /* Validate verity of lower-data */
> > +       if (ofs->config.verity_validate &&
> > +           !d.is_dir && (uppermetacopy || ctr > 1)) {
> > +               struct path datapath;
> > +
> > +               ovl_entry_path_lowerdata(&oe, &datapath);
> > +
> > +               /* Is NULL for lazy lookup, will be verified later */
> > +               if (datapath.dentry) {
> > +                       struct path metapath;
> > +
> > +                       ovl_entry_path_real(ofs, &oe, upperdentry, &metapath);
> > +                       err = ovl_validate_verity(ofs, &metapath, &datapath);
> > +                       if (err < 0)
> > +                               goto out_free_oe;
> > +               }
> > +       }
> > +
> >         if (upperopaque)
> >                 ovl_dentry_set_opaque(dentry);
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 3d14770dc711..b1d639ccd5ac 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 {
> > @@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
> >  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
> >  bool ovl_is_metacopy_dentry(struct dentry *dentry);
> >  char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
> > +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> > +                        u8 *digest_buf, int *buf_length);
> > +int ovl_validate_verity(struct ovl_fs *ofs,
> > +                       struct path *metapath,
> > +                       struct path *datapath);
> >  int ovl_sync_status(struct ovl_fs *ofs);
> >
> >  static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 17eff3e31239..55e90aa0978a 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>
> > @@ -742,6 +744,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, \
> > @@ -756,6 +759,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,
> > @@ -1188,6 +1192,99 @@ 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;
> > +}
> > +
> > +static int ovl_ensure_verity_loaded(struct ovl_fs *ofs,
> > +                                   struct path *datapath)
> > +{
> > +       struct inode *inode = d_inode(datapath->dentry);
> > +       const struct fsverity_info *vi;
> > +       const struct cred *old_cred;
> > +       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.
> > +                */
> > +               old_cred = override_creds(ofs->creator_cred);
>
> Even though it may work, that's the wrong place for override_creds(),
> because you are calling this helper from within ovl_lookup() with
> mounter creds already.
>
> Better to move revert_creds() in ovl_maybe_lookup_lowerdata()
> to out_revert_creds: goto label and call ovl_validate_verity() with
> mounter creds from all call sites.

I'll do that. Although In practice I wonder how much this matters
anyway, because we're not using the result of the open, other than the
side affect of the first open loading the fs-verity info into the
inode.

> > +               filp = dentry_open(datapath, O_RDONLY, current_cred());
>
> You could use open_with_fake_path() here to avoid ENFILE
> not sure if this is critical.
>
> > +               revert_creds(old_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 required_digest[FS_VERITY_MAX_DIGEST_SIZE];
> > +       u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
> > +       enum hash_algo verity_algo;
> > +       int digest_len;
> > +       int err;
> > +
> > +       if (!ofs->config.verity_validate ||
> > +           /* Verity only works on regular files */
> > +           !S_ISREG(d_inode(metapath->dentry)->i_mode))
> > +               return 0;
> > +
> > +       digest_len = sizeof(required_digest);
> > +       err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len);
> > +       if (err == -ENODATA) {
> > +               if (ofs->config.verity_require) {
> > +                       pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n",
> > +                                           metapath->dentry);
> > +                       return -EIO;
> > +               }
> > +               return 0;
> > +       }
>
> Thinking out loud: feels to me that verity xattr is a property that is
> always "coupled" with metacopy xattr.
>
> I wonder if we should check and store them together during lookup.
>
> On one hand that means using a bit more memory per inode
> before we need it.
>
> OTOH, getxattr on metapath during lazy lookup may incur extra
> IO to the metapath inode xattr block that will have been amortized
> if done during lookup.
>
> I have no strong feelings one way or the other.

Currently the metacopy xattrs content is not used. We only check
whether it exists or not. In theory we could extend it to also store
the verity digest itself. Then you only need one lookup. Having
non-zero size metacopy attrs seems to be backwards compatible given
current code.

But, as you say, then we would need to store the digest (which is not
small) for later use. We would have to increase inode size by a
pointer, and then temporarily store the allocated space for the digest
until lazy lookup happens.

> Thanks,
> Amir.
>
> > +       if (err < 0)
> > +               return err;
> > +
> > +       err = ovl_ensure_verity_loaded(ofs, 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 (digest_len != hash_digest_size[verity_algo] ||
> > +           memcmp(required_digest, actual_digest, digest_len) != 0) {
> > +               pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> > +                                   datapath->dentry);
> > +               return -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /*
> >   * ovl_sync_status() - Check fs sync status for volatile mounts
> >   *
> > --
> > 2.39.2
> >
>


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

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

[Eric Biggers]

On Thu, Apr 27, 2023 at 09:22:57AM +0200, Alexander Larsson wrote:
> On Wed, Apr 26, 2023 at 11:47 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote:
> > > +     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 (digest_len != hash_digest_size[verity_algo] ||
> > > +         memcmp(required_digest, actual_digest, digest_len) != 0) {
> > > +             pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> > > +                                 datapath->dentry);
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     return 0;
> >
> > This is incorrect because the digest algorithm is not being compared.
> 
> This is actually an interesting question. How much are things weakened
> by comparing the digest size, but not comparing the digest type. Like,
> suppose the xattr has a sha256 digest (32 bytes), how likely is there
> to be another new supported verity algorithm of the same digest size
> where you can force it to produce matching digests?

It might actually be fairly likely, considering that whenever a system includes
a choice of cryptographic algorithm, it tends to grow to include many different
algorithms.  Some of the reasons for this include:

  - Algorithms can become outdated and broken, yet systems often have to
    continue to support such algorithms for backwards compatibility.

  - People sometimes insist on using "national pride" algorithms, e.g. due to
    government regulations.  For example, in China people can be required to use
    Chinese algorithms instead of the U.S. / NIST algorithms.  See e.g. the
    existing support for SM3, SM4, Streebog, and Aria in the kernel crypto API
    and various other kernel subsystems.

  - Non-cryptographic algorithms might be added for use cases that don't
    actually require cryptographic security, e.g. integrity-only.

I'd strongly discourage you from building something whose security critically
depends on every algorithm that may ever exist being cryptographically secure.

Also, two hash algorithms that are each secure individually are not necessarily
secure when used as alternatives sharing the same output space.  E.g. consider
algorithm1 = SHA-256(data) and algorithm2 = SHA-256(data with all bits flipped).

> 
> I ask because ideally we want to minimize the size of the xattrs,
> since they are stored for each file, and not having to specify the
> type for each saves space. Currently the only two supported algorithms
> (sha256 and sha512) are different sizes, so we essentially compare
> type by comparing the size.
> 
> I see three options here:
> 1) Only compare digest + size (like now)
> 2) Assume size 32 means sha256, and 64 means sha512 and validate that
> 3) Use more space in the xattr to store an algorithm type

Just store the algorithm alongside the digest.  It's just 1 extra byte.

- Eric