* [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata
@ 2023-06-21 11:18 Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-06-21 11:18 UTC (permalink / raw)
To: miklos
Cc: linux-unionfs, amir73il, ebiggers, tytso, fsverity,
Alexander Larsson
This patchset adds support for using fs-verity to validate lowerdata
files by specifying an overlay.verity xattr on the metacopy
files.
This is primarily motivated by the Composefs usecase, where there will
be a read-only EROFS layer that contains redirect into a base data
layer which has fs-verity enabled on all files. However, it is also
useful in general if you want to ensure that the lowerdata files
matches the expected content over time.
I have also added some tests for this feature to xfstests[1].
This series depends on commits from overlay-next, fs-verity-next and
vfs.all, so I based it on:
https://github.com/amir73il/linux/tree/next
Which contains all of these
This series is also available in git here:
https://github.com/alexlarsson/linux/tree/overlay-verity
Changes since v4:
* Rebased also on vfs.all
* Refactored patch series with the new overlay.metadata versioned
xattr header in its own patch.
* Some documentation updates
* Fixes for issues reported in review from Amir.
Changes since v3:
* Instead of using a overlay.digest xattr we extend the current
overlay.metacopy xattr with version, flags and digest. This makes
it flexible for later changes and allows us to use the existing
xattr lookup to know ahead of time whether a file needs to have
verity validated.
I've done some performance checks on this new layout, and the
results are essentially the same as before.
* This is rebased on top of the latest overlayfs-next, which includes
the changes to the new mount API, so that part has been redone.
* The documentation changes have been rewritten to try to be more
clear about the behaviour of i/o verification when verity is used.
Changes since v2:
* Rebased on top of overlayfs-next
* We now alway do verity verification the first time the file content
is used, rather than doing it at lookup time for the non-lazy lookup
case.
Changes since v1:
* Rebased on v2 lazy lowerdata series
* Dropped the "validate" mount option variant. We now only support
"off", "on" and "require", where "off" is the default.
* We now store the digest algorithm used in the overlay.verity xattr.
* Dropped ability to configure default verity options, as this could
cause problems moving layers between machines.
* We now properly resolve dependent mount options by automatically
enabling metacopy and redirect_dir if verity is on, or failing
if the specified options conflict.
* Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
* Renamed new helpers from ovl_entry_path_ to ovl_e_path_
[1] https://github.com/alexlarsson/xfstests/commits/verity-tests
Alexander Larsson (4):
ovl: Add framework for verity support
ovl: Add versioned header for overlay.metacopy xattr
ovl: Validate verity xattr when resolving lowerdata
ovl: Handle verity during copy-up
Documentation/filesystems/fsverity.rst | 2 +
Documentation/filesystems/overlayfs.rst | 47 +++++++
fs/overlayfs/copy_up.c | 47 ++++++-
fs/overlayfs/file.c | 8 +-
fs/overlayfs/namei.c | 82 +++++++++++-
fs/overlayfs/overlayfs.h | 44 ++++++-
fs/overlayfs/ovl_entry.h | 1 +
fs/overlayfs/super.c | 66 +++++++++-
fs/overlayfs/util.c | 158 +++++++++++++++++++++++-
9 files changed, 432 insertions(+), 23 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/4] ovl: Add framework for verity support
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
@ 2023-06-21 11:18 ` Alexander Larsson
2023-06-21 12:18 ` Amir Goldstein
2023-07-03 19:08 ` Eric Biggers
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-06-21 11:18 UTC (permalink / raw)
To: miklos
Cc: linux-unionfs, amir73il, ebiggers, tytso, fsverity,
Alexander Larsson
This adds the scaffolding (docs, config, mount options) for supporting
the new digest field in the metacopy xattr. This contains a fs-verity
digest that need to match the fs-verity digest of the lowerdata
file. The mount option "verity" specifies how this xattr is handled.
If you enable verity ("verity=on") all existing xattrs are validated
before use, and during metacopy we generate verity xattr in the upper
metacopy file (if the source file has verity enabled). This means
later accesses can guarantee that the same data is used.
Additionally you can use "verity=require". In this mode all metacopy
files must have a valid verity xattr. For this to work metadata
copy-up must be able to create a verity xattr (so that later accesses
are validated). Therefore, in this mode, if the lower data file
doesn't have fs-verity enabled we fall back to a full copy rather than
a metacopy.
Actual implementation follows in a separate commit.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
Documentation/filesystems/fsverity.rst | 2 +
Documentation/filesystems/overlayfs.rst | 47 +++++++++++++++++++
fs/overlayfs/overlayfs.h | 6 +++
fs/overlayfs/ovl_entry.h | 1 +
fs/overlayfs/super.c | 61 +++++++++++++++++++++++--
5 files changed, 114 insertions(+), 3 deletions(-)
diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index cb845e8e5435..13e4b18e5dbb 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -326,6 +326,8 @@ the file has fs-verity enabled. This can perform better than
FS_IOC_GETFLAGS and FS_IOC_MEASURE_VERITY because it doesn't require
opening the file, and opening verity files can be expensive.
+.. _accessing_verity_files:
+
Accessing verity files
======================
diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index eb7d2c88ddec..b63e0db03631 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -405,6 +405,53 @@ 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
+----------------------
+
+During metadata copy up of a lower file, if the source file has
+fs-verity enabled and overlay verity support is enabled, then the
+digest of the lower file is added to the "trusted.overlay.metacopy"
+xattr. This is then used to verify the content of the lower file
+each the time the metacopy file is opened.
+
+When a layer containing verity xattrs is used, it means that any such
+metacopy file in the upper layer is guaranteed to match the content
+that was in the lower at the time of the copy-up. If at any time
+(during a mount, after a remount, etc) such a file in the lower is
+replaced or modified in any way, access to the corresponding file in
+overlayfs will result in EIO errors (either on open, due to overlayfs
+digest check, or from a later read due to fs-verity) and a detailed
+error is printed to the kernel logs. For more details of how fs-verity
+file access works, see :ref:`Documentation/filesystems/fsverity.rst
+<accessing_verity_files>`.
+
+Verity can be used as a general robustness check to detect accidental
+changes in the overlayfs directories in use. But, with additional care
+it can also give more powerful guarantees. For example, if the upper
+layer is fully trusted (by using dm-verity or something similar), then
+an untrusted lower layer can be used to supply validated file content
+for all metacopy files. If additionally the untrusted lower
+directories are specified as "Data-only", then they can only supply
+such file content, and the entire mount can be trusted to match the
+upper layer.
+
+This feature is controlled by the "verity" mount option, which
+supports these values:
+
+- "off":
+ The metacopy digest is never generated or used. This is the
+ default if verity option is not specified.
+- "on":
+ Whenever a metacopy files specifies an expected digest, the
+ corresponding data file must match the specified digest. When
+ generating a metacopy file the verity digest will be set in it
+ based on the source file (if it has one).
+- "require":
+ Same as "on", but additionally all metacopy files must specify a
+ digest (or EIO is returned on open). This means metadata copy up
+ will only be used if the data file has fs-verity enabled,
+ otherwise a full copy-up is used.
+
Sharing and copying layers
--------------------------
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4142d1a457ff..cf92a9aaf934 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -70,6 +70,12 @@ enum {
OVL_XINO_ON,
};
+enum {
+ OVL_VERITY_OFF,
+ OVL_VERITY_ON,
+ OVL_VERITY_REQUIRE,
+};
+
/* The set of options that user requested explicitly via mount options */
struct ovl_opt_set {
bool metacopy;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 306e1ecdc96d..e999c73fb0c3 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -10,6 +10,7 @@ struct ovl_config {
char *workdir;
bool default_permissions;
int redirect_mode;
+ int verity_mode;
bool index;
bool uuid;
bool nfs_export;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c14c52560fd6..a4eb9abd4b52 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -366,6 +366,23 @@ static inline int ovl_xino_def(void)
return ovl_xino_auto_def ? OVL_XINO_AUTO : OVL_XINO_OFF;
}
+static const struct constant_table ovl_parameter_verity[] = {
+ { "off", OVL_VERITY_OFF },
+ { "on", OVL_VERITY_ON },
+ { "require", OVL_VERITY_REQUIRE },
+ {}
+};
+
+static const char *ovl_verity_mode(struct ovl_config *config)
+{
+ return ovl_parameter_verity[config->verity_mode].name;
+}
+
+static int ovl_verity_mode_def(void)
+{
+ return OVL_VERITY_OFF;
+}
+
/**
* ovl_show_options
* @m: the seq_file handle
@@ -414,6 +431,9 @@ 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 (ofs->config.verity_mode != ovl_verity_mode_def())
+ seq_printf(m, ",verity=%s",
+ ovl_verity_mode(&ofs->config));
return 0;
}
@@ -463,6 +483,7 @@ enum {
Opt_xino,
Opt_metacopy,
Opt_volatile,
+ Opt_verity,
};
static const struct constant_table ovl_parameter_bool[] = {
@@ -487,6 +508,7 @@ static const struct fs_parameter_spec ovl_parameter_spec[] = {
fsparam_enum("xino", Opt_xino, ovl_parameter_xino),
fsparam_enum("metacopy", Opt_metacopy, ovl_parameter_bool),
fsparam_flag("volatile", Opt_volatile),
+ fsparam_enum("verity", Opt_verity, ovl_parameter_verity),
{}
};
@@ -568,6 +590,9 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
case Opt_userxattr:
config->userxattr = true;
break;
+ case Opt_verity:
+ config->verity_mode = result.uint_32;
+ break;
default:
pr_err("unrecognized mount option \"%s\" or missing value\n",
param->key);
@@ -607,6 +632,18 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
config->ovl_volatile = false;
}
+ /* Resolve verity -> metacopy dependency */
+ if (config->verity_mode && !config->metacopy) {
+ /* Don't allow explicit specified conflicting combinations */
+ if (set.metacopy) {
+ pr_err("conflicting options: metacopy=off,verity=%s\n",
+ ovl_verity_mode(config));
+ return -EINVAL;
+ }
+ /* Otherwise automatically enable metacopy. */
+ config->metacopy = true;
+ }
+
/*
* This is to make the logic below simpler. It doesn't make any other
* difference, since redirect_dir=on is only used for upper.
@@ -614,13 +651,18 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
if (!config->upperdir && config->redirect_mode == OVL_REDIRECT_FOLLOW)
config->redirect_mode = OVL_REDIRECT_ON;
- /* Resolve metacopy -> redirect_dir dependency */
+ /* Resolve verity -> metacopy -> redirect_dir dependency */
if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
if (set.metacopy && set.redirect) {
pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
ovl_redirect_mode(config));
return -EINVAL;
}
+ if (config->verity_mode && set.redirect) {
+ pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
+ ovl_verity_mode(config), ovl_redirect_mode(config));
+ return -EINVAL;
+ }
if (set.redirect) {
/*
* There was an explicit redirect_dir=... that resulted
@@ -657,7 +699,7 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
}
}
- /* Resolve nfs_export -> !metacopy dependency */
+ /* Resolve nfs_export -> !metacopy && !verity dependency */
if (config->nfs_export && config->metacopy) {
if (set.nfs_export && set.metacopy) {
pr_err("conflicting options: nfs_export=on,metacopy=on\n");
@@ -670,6 +712,14 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
*/
pr_info("disabling nfs_export due to metacopy=on\n");
config->nfs_export = false;
+ } else if (config->verity_mode) {
+ /*
+ * There was an explicit verity=.. that resulted
+ * in this conflict.
+ */
+ pr_info("disabling nfs_export due to verity=%s\n",
+ ovl_verity_mode(config));
+ config->nfs_export = false;
} else {
/*
* There was an explicit nfs_export=on that resulted
@@ -681,7 +731,7 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
}
- /* Resolve userxattr -> !redirect && !metacopy dependency */
+ /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
if (config->userxattr) {
if (set.redirect &&
config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
@@ -693,6 +743,11 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
pr_err("conflicting options: userxattr,metacopy=on\n");
return -EINVAL;
}
+ if (config->verity_mode) {
+ pr_err("conflicting options: userxattr,verity=%s\n",
+ ovl_verity_mode(config));
+ return -EINVAL;
+ }
/*
* Silently disable default setting of redirect and metacopy.
* This shall be the default in the future as well: these
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
@ 2023-06-21 11:18 ` Alexander Larsson
2023-06-21 12:21 ` Amir Goldstein
2023-07-03 19:13 ` Eric Biggers
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
3 siblings, 2 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-06-21 11:18 UTC (permalink / raw)
To: miklos
Cc: linux-unionfs, amir73il, ebiggers, tytso, fsverity,
Alexander Larsson
Historically overlay.metacopy was a zero-size xattr, and it's
existence marked a metacopy file. This change adds a versioned header
with a flag field, a length and a digest. The initial use-case of this
will be for validating a fs-verity digest, but the flags field could
also be used later for other new features.
ovl_check_metacopy_xattr() now returns the size of the xattr,
emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
distinguish it from the no-xattr case.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
fs/overlayfs/namei.c | 10 +++++-----
fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
3 files changed, 61 insertions(+), 10 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 57adf911735f..3dd480253710 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -25,7 +25,7 @@ struct ovl_lookup_data {
bool stop;
bool last;
char *redirect;
- bool metacopy;
+ int metacopy;
/* Referring to last redirect xattr */
bool absolute_redirect;
};
@@ -270,7 +270,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
d->stop = true;
goto put_and_out;
}
- err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
+ err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
if (err < 0)
goto out_err;
@@ -963,7 +963,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
.stop = false,
.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
.redirect = NULL,
- .metacopy = false,
+ .metacopy = 0,
};
if (dentry->d_name.len > ofs->namelen)
@@ -1120,7 +1120,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
/* Defer lookup of lowerdata in data-only layers to first access */
if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
- d.metacopy = false;
+ d.metacopy = 0;
ctr++;
}
@@ -1211,7 +1211,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
upperredirect = NULL;
goto out_free_oe;
}
- err = ovl_check_metacopy_xattr(ofs, &upperpath);
+ err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
if (err < 0)
goto out_free_oe;
uppermetacopy = err;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index cf92a9aaf934..6d4e08df0dfe 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -7,6 +7,7 @@
#include <linux/kernel.h>
#include <linux/uuid.h>
#include <linux/fs.h>
+#include <linux/fsverity.h>
#include <linux/namei.h>
#include <linux/posix_acl.h>
#include <linux/posix_acl_xattr.h>
@@ -140,6 +141,26 @@ struct ovl_fh {
#define OVL_FH_FID_OFFSET (OVL_FH_WIRE_OFFSET + \
offsetof(struct ovl_fb, fid))
+/* On-disk format for "metacopy" xattr (if non-zero size) */
+struct ovl_metacopy {
+ u8 version; /* 0 */
+ u8 len; /* size of this header + used digest bytes */
+ u8 flags;
+ u8 digest_algo; /* FS_VERITY_HASH_ALG_* constant, 0 for no digest */
+ u8 digest[FS_VERITY_MAX_DIGEST_SIZE]; /* Only the used part on disk */
+} __packed;
+
+#define OVL_METACOPY_MAX_SIZE (sizeof(struct ovl_metacopy))
+#define OVL_METACOPY_MIN_SIZE (OVL_METACOPY_MAX_SIZE - FS_VERITY_MAX_DIGEST_SIZE)
+#define OVL_METACOPY_INIT { 0, OVL_METACOPY_MIN_SIZE }
+
+static inline int ovl_metadata_digest_size(const struct ovl_metacopy *metacopy)
+{
+ if (metacopy->len < OVL_METACOPY_MIN_SIZE)
+ return 0;
+ return (int)metacopy->len - OVL_METACOPY_MIN_SIZE;
+}
+
extern const char *const ovl_xattr_table[][2];
static inline const char *ovl_xattr(struct ovl_fs *ofs, enum ovl_xattr ox)
{
@@ -490,7 +511,8 @@ bool ovl_need_index(struct dentry *dentry);
int ovl_nlink_start(struct dentry *dentry);
void ovl_nlink_end(struct dentry *dentry);
int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
-int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
+int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
+ struct ovl_metacopy *data);
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_sync_status(struct ovl_fs *ofs);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7ef9e13c404a..921747223991 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1054,8 +1054,12 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
return -EIO;
}
-/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
-int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
+/*
+ * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
+ * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
+ */
+int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
+ struct ovl_metacopy *data)
{
int res;
@@ -1063,7 +1067,8 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
if (!S_ISREG(d_inode(path->dentry)->i_mode))
return 0;
- res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, NULL, 0);
+ res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY,
+ data, data ? OVL_METACOPY_MAX_SIZE : 0);
if (res < 0) {
if (res == -ENODATA || res == -EOPNOTSUPP)
return 0;
@@ -1077,7 +1082,31 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
goto out;
}
- return 1;
+ if (res == 0) {
+ /* Emulate empty data for zero size metacopy xattr */
+ res = OVL_METACOPY_MIN_SIZE;
+ if (data) {
+ memset(data, 0, res);
+ data->len = res;
+ }
+ } else if (res < OVL_METACOPY_MIN_SIZE) {
+ pr_warn_ratelimited("metacopy file '%pd' has too small xattr\n",
+ path->dentry);
+ return -EIO;
+ } else if (data) {
+ if (data->version != 0) {
+ pr_warn_ratelimited("metacopy file '%pd' has unsupported version\n",
+ path->dentry);
+ return -EIO;
+ }
+ if (res != data->len) {
+ pr_warn_ratelimited("metacopy file '%pd' has invalid xattr size\n",
+ path->dentry);
+ return -EIO;
+ }
+ }
+
+ return res;
out:
pr_warn_ratelimited("failed to get metacopy (%i)\n", res);
return res;
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
@ 2023-06-21 11:18 ` Alexander Larsson
2023-06-21 12:24 ` Amir Goldstein
2023-07-03 19:24 ` Eric Biggers
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
3 siblings, 2 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-06-21 11:18 UTC (permalink / raw)
To: miklos
Cc: linux-unionfs, amir73il, ebiggers, tytso, fsverity,
Alexander Larsson
The new digest field in the metacopy xattr is used during lookup to
record whether the header contained a digest in the OVL_HAS_DIGEST
flags.
When accessing file data the first time, if OVL_HAS_DIGEST is set, we
reload the metadata and check that the source lowerdata inode matches
the specified digest in it (according to the enabled verity
options). If the verity check passes we store this info in the inode
flags as OVL_VERIFIED_DIGEST, so that we can avoid doing it again if
the inode remains in memory.
The verification is done in ovl_maybe_validate_verity() which needs to
be called in the same places as ovl_maybe_lookup_lowerdata(), so there
is a new ovl_verify_lowerdata() helper that calls these in the right
order, and all current callers of ovl_maybe_lookup_lowerdata() are
changed to call it instead.
Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/file.c | 8 ++--
fs/overlayfs/namei.c | 72 +++++++++++++++++++++++++++++++-
fs/overlayfs/overlayfs.h | 11 ++++-
fs/overlayfs/super.c | 5 ++-
fs/overlayfs/util.c | 90 ++++++++++++++++++++++++++++++++++++++++
6 files changed, 180 insertions(+), 8 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 568f743a5584..68f01fd7f211 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1078,7 +1078,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
* not very important to optimize this case, so do lazy lowerdata lookup
* before any copy up, so we can do it before taking ovl_inode_lock().
*/
- err = ovl_maybe_lookup_lowerdata(dentry);
+ err = ovl_verify_lowerdata(dentry);
if (err)
return err;
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index cb53c84108fc..859a833c1035 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -115,8 +115,8 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
if (allow_meta) {
ovl_path_real(dentry, &realpath);
} else {
- /* lazy lookup of lowerdata */
- err = ovl_maybe_lookup_lowerdata(dentry);
+ /* lazy lookup and verify of lowerdata */
+ err = ovl_verify_lowerdata(dentry);
if (err)
return err;
@@ -159,8 +159,8 @@ static int ovl_open(struct inode *inode, struct file *file)
struct path realpath;
int err;
- /* lazy lookup of lowerdata */
- err = ovl_maybe_lookup_lowerdata(dentry);
+ /* lazy lookup and verify lowerdata */
+ err = ovl_verify_lowerdata(dentry);
if (err)
return err;
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 3dd480253710..d00ec43f2376 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -889,8 +889,58 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
return err;
}
+static int ovl_maybe_validate_verity(struct dentry *dentry)
+{
+ struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct inode *inode = d_inode(dentry);
+ struct path datapath, metapath;
+ int err;
+
+ if (!ofs->config.verity_mode ||
+ !ovl_is_metacopy_dentry(dentry) ||
+ ovl_test_flag(OVL_VERIFIED_DIGEST, inode))
+ return 0;
+
+ if (!ovl_test_flag(OVL_HAS_DIGEST, inode)) {
+ if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
+ pr_warn_ratelimited("metacopy file '%pd' has no digest specified\n",
+ dentry);
+ return -EIO;
+ }
+ return 0;
+ }
+
+ ovl_path_lowerdata(dentry, &datapath);
+ if (!datapath.dentry)
+ return -EIO;
+
+ ovl_path_real(dentry, &metapath);
+ if (!metapath.dentry)
+ return -EIO;
+
+ err = ovl_inode_lock_interruptible(inode);
+ if (err)
+ return err;
+
+ if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) {
+ const struct cred *old_cred;
+
+ old_cred = ovl_override_creds(dentry->d_sb);
+
+ err = ovl_validate_verity(ofs, &metapath, &datapath);
+ if (err == 0)
+ ovl_set_flag(OVL_VERIFIED_DIGEST, inode);
+
+ revert_creds(old_cred);
+ }
+
+ ovl_inode_unlock(inode);
+
+ return err;
+}
+
/* Lazy lookup of lowerdata */
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
+static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
const char *redirect = ovl_lowerdata_redirect(inode);
@@ -935,6 +985,17 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
goto out;
}
+int ovl_verify_lowerdata(struct dentry *dentry)
+{
+ int err;
+
+ err = ovl_maybe_lookup_lowerdata(dentry);
+ if (err)
+ return err;
+
+ return ovl_maybe_validate_verity(dentry);
+}
+
struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
unsigned int flags)
{
@@ -955,6 +1016,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
unsigned int i;
int err;
bool uppermetacopy = false;
+ int metacopy_size = 0;
struct ovl_lookup_data d = {
.sb = dentry->d_sb,
.name = dentry->d_name,
@@ -999,6 +1061,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
if (d.metacopy)
uppermetacopy = true;
+ metacopy_size = d.metacopy;
}
if (d.redirect) {
@@ -1076,6 +1139,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
origin = this;
}
+ if (!upperdentry && !d.is_dir && !ctr && d.metacopy)
+ metacopy_size = d.metacopy;
+
if (d.metacopy && ctr) {
/*
* Do not store intermediate metacopy dentries in
@@ -1215,6 +1281,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
if (err < 0)
goto out_free_oe;
uppermetacopy = err;
+ metacopy_size = err;
}
if (upperdentry || ctr) {
@@ -1236,6 +1303,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
goto out_free_oe;
if (upperdentry && !uppermetacopy)
ovl_set_flag(OVL_UPPERDATA, inode);
+
+ if (metacopy_size > OVL_METACOPY_MIN_SIZE)
+ ovl_set_flag(OVL_HAS_DIGEST, inode);
}
ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6d4e08df0dfe..9fbbc077643b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -50,6 +50,8 @@ enum ovl_inode_flag {
OVL_UPPERDATA,
/* Inode number will remain constant over copy up. */
OVL_CONST_INO,
+ OVL_HAS_DIGEST,
+ OVL_VERIFIED_DIGEST,
};
enum ovl_entry_flag {
@@ -513,8 +515,15 @@ void ovl_nlink_end(struct dentry *dentry);
int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
struct ovl_metacopy *data);
+int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
+ struct ovl_metacopy *metacopy);
bool ovl_is_metacopy_dentry(struct dentry *dentry);
char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
+int ovl_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)
@@ -634,7 +643,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
struct dentry *origin, bool verify);
int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
-int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
+int ovl_verify_lowerdata(struct dentry *dentry);
struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
unsigned int flags);
bool ovl_lower_positive(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index a4eb9abd4b52..457a5bc439cb 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -63,6 +63,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
const struct inode *inode)
{
struct dentry *real = NULL, *lower;
+ int err;
/* It's an overlay file */
if (inode && d_inode(dentry) == inode)
@@ -89,7 +90,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
* uprobes on offset within the file, so lowerdata should be available
* when setting the uprobe.
*/
- ovl_maybe_lookup_lowerdata(dentry);
+ err = ovl_verify_lowerdata(dentry);
+ if (err)
+ goto bug;
lower = ovl_dentry_lowerdata(dentry);
if (!lower)
goto bug;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 921747223991..927a1133859d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -10,6 +10,7 @@
#include <linux/cred.h>
#include <linux/xattr.h>
#include <linux/exportfs.h>
+#include <linux/file.h>
#include <linux/fileattr.h>
#include <linux/uuid.h>
#include <linux/namei.h>
@@ -1112,6 +1113,18 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
return res;
}
+int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d, struct ovl_metacopy *metacopy)
+{
+ size_t len = metacopy->len;
+
+ /* If no flags or digest fall back to empty metacopy file */
+ if (metacopy->version == 0 && metacopy->flags == 0 && metacopy->digest_algo == 0)
+ len = 0;
+
+ return ovl_check_setxattr(ofs, d, OVL_XATTR_METACOPY,
+ metacopy, len, -EOPNOTSUPP);
+}
+
bool ovl_is_metacopy_dentry(struct dentry *dentry)
{
struct ovl_entry *oe = OVL_E(dentry);
@@ -1174,6 +1187,83 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
return ERR_PTR(res);
}
+/* Call with mounter creds as it may open the file */
+static int ovl_ensure_verity_loaded(struct path *datapath)
+{
+ struct inode *inode = d_inode(datapath->dentry);
+ const struct fsverity_info *vi;
+ struct file *filp;
+
+ vi = fsverity_get_info(inode);
+ if (vi == NULL && IS_VERITY(inode)) {
+ /*
+ * If this inode was not yet opened, the verity info hasn't been
+ * loaded yet, so we need to do that here to force it into memory.
+ */
+ filp = kernel_file_open(datapath, O_RDONLY, inode, current_cred());
+ if (IS_ERR(filp))
+ return PTR_ERR(filp);
+ fput(filp);
+ }
+
+ return 0;
+}
+
+int ovl_validate_verity(struct ovl_fs *ofs,
+ struct path *metapath,
+ struct path *datapath)
+{
+ struct ovl_metacopy metacopy_data;
+ u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
+ int xattr_digest_size, digest_size;
+ int xattr_size, err;
+ u8 verity_algo;
+
+ if (!ofs->config.verity_mode ||
+ /* Verity only works on regular files */
+ !S_ISREG(d_inode(metapath->dentry)->i_mode))
+ return 0;
+
+ xattr_size = ovl_check_metacopy_xattr(ofs, metapath, &metacopy_data);
+ if (xattr_size < 0)
+ return xattr_size;
+
+ if (!xattr_size || !metacopy_data.digest_algo) {
+ if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
+ pr_warn_ratelimited("metacopy file '%pd' has no digest specified\n",
+ metapath->dentry);
+ return -EIO;
+ }
+ return 0;
+ }
+
+ xattr_digest_size = ovl_metadata_digest_size(&metacopy_data);
+
+ err = ovl_ensure_verity_loaded(datapath);
+ if (err < 0) {
+ pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
+ datapath->dentry);
+ return -EIO;
+ }
+
+ digest_size = fsverity_get_digest(d_inode(datapath->dentry), actual_digest,
+ &verity_algo, NULL);
+ if (digest_size == 0) {
+ pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry);
+ return -EIO;
+ }
+
+ if (xattr_digest_size != digest_size ||
+ metacopy_data.digest_algo != verity_algo ||
+ memcmp(metacopy_data.digest, actual_digest, xattr_digest_size) != 0) {
+ pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
+ datapath->dentry);
+ return -EIO;
+ }
+
+ return 0;
+}
+
/*
* ovl_sync_status() - Check fs sync status for volatile mounts
*
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 4/4] ovl: Handle verity during copy-up
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
` (2 preceding siblings ...)
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
@ 2023-06-21 11:18 ` Alexander Larsson
2023-06-21 12:26 ` Amir Goldstein
2023-07-03 19:29 ` Eric Biggers
3 siblings, 2 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-06-21 11:18 UTC (permalink / raw)
To: miklos
Cc: linux-unionfs, amir73il, ebiggers, tytso, fsverity,
Alexander Larsson
During regular metacopy, if lowerdata file has fs-verity enabled, and
the verity option is enabled, we add the digest to the metacopy xattr.
If verity is required, and lowerdata does not have fs-verity enabled,
fall back to full copy-up (or the generated metacopy would not
validate).
Signed-off-by: Alexander Larsson <alexl@redhat.com>
---
fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
fs/overlayfs/overlayfs.h | 3 +++
fs/overlayfs/util.c | 33 ++++++++++++++++++++++++++++-
3 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 68f01fd7f211..fce7d048673c 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -544,6 +544,7 @@ struct ovl_copy_up_ctx {
bool origin;
bool indexed;
bool metacopy;
+ bool metacopy_digest;
};
static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -641,8 +642,21 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
}
if (c->metacopy) {
- err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
- NULL, 0, -EOPNOTSUPP);
+ struct path lowerdatapath;
+ struct ovl_metacopy metacopy_data = OVL_METACOPY_INIT;
+
+ ovl_path_lowerdata(c->dentry, &lowerdatapath);
+ if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
+ err = -EIO;
+ else
+ err = ovl_set_verity_xattr_from(ofs, &lowerdatapath, &metacopy_data);
+
+ if (metacopy_data.digest_algo)
+ c->metacopy_digest = true;
+
+ if (!err)
+ err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
+
if (err)
return err;
}
@@ -751,6 +765,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
if (err)
goto cleanup;
+ if (c->metacopy_digest)
+ ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
+ else
+ ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
+ ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
+
if (!c->metacopy)
ovl_set_upperdata(d_inode(c->dentry));
inode = d_inode(c->dentry);
@@ -813,6 +833,12 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
if (err)
goto out_fput;
+ if (c->metacopy_digest)
+ ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
+ else
+ ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
+ ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
+
if (!c->metacopy)
ovl_set_upperdata(d_inode(c->dentry));
ovl_inode_update(d_inode(c->dentry), dget(temp));
@@ -918,6 +944,19 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
return false;
+ /* Fall back to full copy if no fsverity on source data and we require verity */
+ if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
+ struct path lowerdata;
+
+ ovl_path_lowerdata(dentry, &lowerdata);
+
+ if (WARN_ON_ONCE(lowerdata.dentry == NULL) ||
+ ovl_ensure_verity_loaded(&lowerdata) ||
+ !fsverity_get_info(d_inode(lowerdata.dentry))) {
+ return false;
+ }
+ }
+
return true;
}
@@ -984,6 +1023,8 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
if (err)
goto out_free;
+ ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
+ ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
ovl_set_upperdata(d_inode(c->dentry));
out_free:
kfree(capability);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9fbbc077643b..e728360c66ff 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -519,11 +519,14 @@ int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
struct ovl_metacopy *metacopy);
bool ovl_is_metacopy_dentry(struct dentry *dentry);
char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
+int ovl_ensure_verity_loaded(struct path *path);
int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
u8 *digest_buf, int *buf_length);
int ovl_validate_verity(struct ovl_fs *ofs,
struct path *metapath,
struct path *datapath);
+int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
+ struct ovl_metacopy *metacopy);
int ovl_sync_status(struct ovl_fs *ofs);
static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 927a1133859d..439e23496713 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1188,7 +1188,7 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
}
/* Call with mounter creds as it may open the file */
-static int ovl_ensure_verity_loaded(struct path *datapath)
+int ovl_ensure_verity_loaded(struct path *datapath)
{
struct inode *inode = d_inode(datapath->dentry);
const struct fsverity_info *vi;
@@ -1264,6 +1264,37 @@ int ovl_validate_verity(struct ovl_fs *ofs,
return 0;
}
+int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
+ struct ovl_metacopy *metacopy)
+{
+ int err, digest_size;
+
+ if (!ofs->config.verity_mode || !S_ISREG(d_inode(src->dentry)->i_mode))
+ return 0;
+
+ err = ovl_ensure_verity_loaded(src);
+ if (err < 0) {
+ pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
+ src->dentry);
+ return -EIO;
+ }
+
+ digest_size = fsverity_get_digest(d_inode(src->dentry),
+ metacopy->digest, &metacopy->digest_algo, NULL);
+ if (digest_size == 0 ||
+ WARN_ON_ONCE(digest_size > FS_VERITY_MAX_DIGEST_SIZE)) {
+ if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
+ pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
+ src->dentry);
+ return -EIO;
+ }
+ return 0;
+ }
+
+ metacopy->len += digest_size;
+ return 0;
+}
+
/*
* ovl_sync_status() - Check fs sync status for volatile mounts
*
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] ovl: Add framework for verity support
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
@ 2023-06-21 12:18 ` Amir Goldstein
2023-07-03 19:08 ` Eric Biggers
1 sibling, 0 replies; 17+ messages in thread
From: Amir Goldstein @ 2023-06-21 12:18 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, ebiggers, tytso, fsverity
On Wed, Jun 21, 2023 at 2:18 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> This adds the scaffolding (docs, config, mount options) for supporting
> the new digest field in the metacopy xattr. This contains a fs-verity
> digest that need to match the fs-verity digest of the lowerdata
> file. The mount option "verity" specifies how this xattr is handled.
>
> If you enable verity ("verity=on") all existing xattrs are validated
> before use, and during metacopy we generate verity xattr in the upper
> metacopy file (if the source file has verity enabled). This means
> later accesses can guarantee that the same data is used.
>
> Additionally you can use "verity=require". In this mode all metacopy
> files must have a valid verity xattr. For this to work metadata
> copy-up must be able to create a verity xattr (so that later accesses
> are validated). Therefore, in this mode, if the lower data file
> doesn't have fs-verity enabled we fall back to a full copy rather than
> a metacopy.
>
> Actual implementation follows in a separate commit.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
Looks good.
Pending ack from Eric of documentations:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> Documentation/filesystems/fsverity.rst | 2 +
> Documentation/filesystems/overlayfs.rst | 47 +++++++++++++++++++
> fs/overlayfs/overlayfs.h | 6 +++
> fs/overlayfs/ovl_entry.h | 1 +
> fs/overlayfs/super.c | 61 +++++++++++++++++++++++--
> 5 files changed, 114 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index cb845e8e5435..13e4b18e5dbb 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -326,6 +326,8 @@ the file has fs-verity enabled. This can perform better than
> FS_IOC_GETFLAGS and FS_IOC_MEASURE_VERITY because it doesn't require
> opening the file, and opening verity files can be expensive.
>
> +.. _accessing_verity_files:
> +
> Accessing verity files
> ======================
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index eb7d2c88ddec..b63e0db03631 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -405,6 +405,53 @@ 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
> +----------------------
> +
> +During metadata copy up of a lower file, if the source file has
> +fs-verity enabled and overlay verity support is enabled, then the
> +digest of the lower file is added to the "trusted.overlay.metacopy"
> +xattr. This is then used to verify the content of the lower file
> +each the time the metacopy file is opened.
> +
> +When a layer containing verity xattrs is used, it means that any such
> +metacopy file in the upper layer is guaranteed to match the content
> +that was in the lower at the time of the copy-up. If at any time
> +(during a mount, after a remount, etc) such a file in the lower is
> +replaced or modified in any way, access to the corresponding file in
> +overlayfs will result in EIO errors (either on open, due to overlayfs
> +digest check, or from a later read due to fs-verity) and a detailed
> +error is printed to the kernel logs. For more details of how fs-verity
> +file access works, see :ref:`Documentation/filesystems/fsverity.rst
> +<accessing_verity_files>`.
> +
> +Verity can be used as a general robustness check to detect accidental
> +changes in the overlayfs directories in use. But, with additional care
> +it can also give more powerful guarantees. For example, if the upper
> +layer is fully trusted (by using dm-verity or something similar), then
> +an untrusted lower layer can be used to supply validated file content
> +for all metacopy files. If additionally the untrusted lower
> +directories are specified as "Data-only", then they can only supply
> +such file content, and the entire mount can be trusted to match the
> +upper layer.
> +
> +This feature is controlled by the "verity" mount option, which
> +supports these values:
> +
> +- "off":
> + The metacopy digest is never generated or used. This is the
> + default if verity option is not specified.
> +- "on":
> + Whenever a metacopy files specifies an expected digest, the
> + corresponding data file must match the specified digest. When
> + generating a metacopy file the verity digest will be set in it
> + based on the source file (if it has one).
> +- "require":
> + Same as "on", but additionally all metacopy files must specify a
> + digest (or EIO is returned on open). This means metadata copy up
> + will only be used if the data file has fs-verity enabled,
> + otherwise a full copy-up is used.
> +
> Sharing and copying layers
> --------------------------
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4142d1a457ff..cf92a9aaf934 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -70,6 +70,12 @@ enum {
> OVL_XINO_ON,
> };
>
> +enum {
> + OVL_VERITY_OFF,
> + OVL_VERITY_ON,
> + OVL_VERITY_REQUIRE,
> +};
> +
> /* The set of options that user requested explicitly via mount options */
> struct ovl_opt_set {
> bool metacopy;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 306e1ecdc96d..e999c73fb0c3 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -10,6 +10,7 @@ struct ovl_config {
> char *workdir;
> bool default_permissions;
> int redirect_mode;
> + int verity_mode;
> bool index;
> bool uuid;
> bool nfs_export;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index c14c52560fd6..a4eb9abd4b52 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -366,6 +366,23 @@ static inline int ovl_xino_def(void)
> return ovl_xino_auto_def ? OVL_XINO_AUTO : OVL_XINO_OFF;
> }
>
> +static const struct constant_table ovl_parameter_verity[] = {
> + { "off", OVL_VERITY_OFF },
> + { "on", OVL_VERITY_ON },
> + { "require", OVL_VERITY_REQUIRE },
> + {}
> +};
> +
> +static const char *ovl_verity_mode(struct ovl_config *config)
> +{
> + return ovl_parameter_verity[config->verity_mode].name;
> +}
> +
> +static int ovl_verity_mode_def(void)
> +{
> + return OVL_VERITY_OFF;
> +}
> +
> /**
> * ovl_show_options
> * @m: the seq_file handle
> @@ -414,6 +431,9 @@ 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 (ofs->config.verity_mode != ovl_verity_mode_def())
> + seq_printf(m, ",verity=%s",
> + ovl_verity_mode(&ofs->config));
> return 0;
> }
>
> @@ -463,6 +483,7 @@ enum {
> Opt_xino,
> Opt_metacopy,
> Opt_volatile,
> + Opt_verity,
> };
>
> static const struct constant_table ovl_parameter_bool[] = {
> @@ -487,6 +508,7 @@ static const struct fs_parameter_spec ovl_parameter_spec[] = {
> fsparam_enum("xino", Opt_xino, ovl_parameter_xino),
> fsparam_enum("metacopy", Opt_metacopy, ovl_parameter_bool),
> fsparam_flag("volatile", Opt_volatile),
> + fsparam_enum("verity", Opt_verity, ovl_parameter_verity),
> {}
> };
>
> @@ -568,6 +590,9 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
> case Opt_userxattr:
> config->userxattr = true;
> break;
> + case Opt_verity:
> + config->verity_mode = result.uint_32;
> + break;
> default:
> pr_err("unrecognized mount option \"%s\" or missing value\n",
> param->key);
> @@ -607,6 +632,18 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> config->ovl_volatile = false;
> }
>
> + /* Resolve verity -> metacopy dependency */
> + if (config->verity_mode && !config->metacopy) {
> + /* Don't allow explicit specified conflicting combinations */
> + if (set.metacopy) {
> + pr_err("conflicting options: metacopy=off,verity=%s\n",
> + ovl_verity_mode(config));
> + return -EINVAL;
> + }
> + /* Otherwise automatically enable metacopy. */
> + config->metacopy = true;
> + }
> +
> /*
> * This is to make the logic below simpler. It doesn't make any other
> * difference, since redirect_dir=on is only used for upper.
> @@ -614,13 +651,18 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> if (!config->upperdir && config->redirect_mode == OVL_REDIRECT_FOLLOW)
> config->redirect_mode = OVL_REDIRECT_ON;
>
> - /* Resolve metacopy -> redirect_dir dependency */
> + /* Resolve verity -> metacopy -> redirect_dir dependency */
> if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
> if (set.metacopy && set.redirect) {
> pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
> ovl_redirect_mode(config));
> return -EINVAL;
> }
> + if (config->verity_mode && set.redirect) {
> + pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
> + ovl_verity_mode(config), ovl_redirect_mode(config));
> + return -EINVAL;
> + }
> if (set.redirect) {
> /*
> * There was an explicit redirect_dir=... that resulted
> @@ -657,7 +699,7 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> }
> }
>
> - /* Resolve nfs_export -> !metacopy dependency */
> + /* Resolve nfs_export -> !metacopy && !verity dependency */
> if (config->nfs_export && config->metacopy) {
> if (set.nfs_export && set.metacopy) {
> pr_err("conflicting options: nfs_export=on,metacopy=on\n");
> @@ -670,6 +712,14 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> */
> pr_info("disabling nfs_export due to metacopy=on\n");
> config->nfs_export = false;
> + } else if (config->verity_mode) {
> + /*
> + * There was an explicit verity=.. that resulted
> + * in this conflict.
> + */
> + pr_info("disabling nfs_export due to verity=%s\n",
> + ovl_verity_mode(config));
> + config->nfs_export = false;
> } else {
> /*
> * There was an explicit nfs_export=on that resulted
> @@ -681,7 +731,7 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> }
>
>
> - /* Resolve userxattr -> !redirect && !metacopy dependency */
> + /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
> if (config->userxattr) {
> if (set.redirect &&
> config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
> @@ -693,6 +743,11 @@ static int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> pr_err("conflicting options: userxattr,metacopy=on\n");
> return -EINVAL;
> }
> + if (config->verity_mode) {
> + pr_err("conflicting options: userxattr,verity=%s\n",
> + ovl_verity_mode(config));
> + return -EINVAL;
> + }
> /*
> * Silently disable default setting of redirect and metacopy.
> * This shall be the default in the future as well: these
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
@ 2023-06-21 12:21 ` Amir Goldstein
2023-07-03 19:13 ` Eric Biggers
1 sibling, 0 replies; 17+ messages in thread
From: Amir Goldstein @ 2023-06-21 12:21 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, ebiggers, tytso, fsverity
On Wed, Jun 21, 2023 at 2:18 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> Historically overlay.metacopy was a zero-size xattr, and it's
> existence marked a metacopy file. This change adds a versioned header
> with a flag field, a length and a digest. The initial use-case of this
> will be for validating a fs-verity digest, but the flags field could
> also be used later for other new features.
>
> ovl_check_metacopy_xattr() now returns the size of the xattr,
> emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
> distinguish it from the no-xattr case.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
Looks good.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/namei.c | 10 +++++-----
> fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
> fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
> 3 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 57adf911735f..3dd480253710 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -25,7 +25,7 @@ struct ovl_lookup_data {
> bool stop;
> bool last;
> char *redirect;
> - bool metacopy;
> + int metacopy;
> /* Referring to last redirect xattr */
> bool absolute_redirect;
> };
> @@ -270,7 +270,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> d->stop = true;
> goto put_and_out;
> }
> - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
> + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
> if (err < 0)
> goto out_err;
>
> @@ -963,7 +963,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> .stop = false,
> .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> .redirect = NULL,
> - .metacopy = false,
> + .metacopy = 0,
> };
>
> if (dentry->d_name.len > ofs->namelen)
> @@ -1120,7 +1120,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
> /* Defer lookup of lowerdata in data-only layers to first access */
> if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
> - d.metacopy = false;
> + d.metacopy = 0;
> ctr++;
> }
>
> @@ -1211,7 +1211,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> upperredirect = NULL;
> goto out_free_oe;
> }
> - err = ovl_check_metacopy_xattr(ofs, &upperpath);
> + err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
> if (err < 0)
> goto out_free_oe;
> uppermetacopy = err;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index cf92a9aaf934..6d4e08df0dfe 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -7,6 +7,7 @@
> #include <linux/kernel.h>
> #include <linux/uuid.h>
> #include <linux/fs.h>
> +#include <linux/fsverity.h>
> #include <linux/namei.h>
> #include <linux/posix_acl.h>
> #include <linux/posix_acl_xattr.h>
> @@ -140,6 +141,26 @@ struct ovl_fh {
> #define OVL_FH_FID_OFFSET (OVL_FH_WIRE_OFFSET + \
> offsetof(struct ovl_fb, fid))
>
> +/* On-disk format for "metacopy" xattr (if non-zero size) */
> +struct ovl_metacopy {
> + u8 version; /* 0 */
> + u8 len; /* size of this header + used digest bytes */
> + u8 flags;
> + u8 digest_algo; /* FS_VERITY_HASH_ALG_* constant, 0 for no digest */
> + u8 digest[FS_VERITY_MAX_DIGEST_SIZE]; /* Only the used part on disk */
> +} __packed;
> +
> +#define OVL_METACOPY_MAX_SIZE (sizeof(struct ovl_metacopy))
> +#define OVL_METACOPY_MIN_SIZE (OVL_METACOPY_MAX_SIZE - FS_VERITY_MAX_DIGEST_SIZE)
> +#define OVL_METACOPY_INIT { 0, OVL_METACOPY_MIN_SIZE }
> +
> +static inline int ovl_metadata_digest_size(const struct ovl_metacopy *metacopy)
> +{
> + if (metacopy->len < OVL_METACOPY_MIN_SIZE)
> + return 0;
> + return (int)metacopy->len - OVL_METACOPY_MIN_SIZE;
> +}
> +
> extern const char *const ovl_xattr_table[][2];
> static inline const char *ovl_xattr(struct ovl_fs *ofs, enum ovl_xattr ox)
> {
> @@ -490,7 +511,8 @@ bool ovl_need_index(struct dentry *dentry);
> int ovl_nlink_start(struct dentry *dentry);
> void ovl_nlink_end(struct dentry *dentry);
> int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
> -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path);
> +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> + struct ovl_metacopy *data);
> 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_sync_status(struct ovl_fs *ofs);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 7ef9e13c404a..921747223991 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1054,8 +1054,12 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
> return -EIO;
> }
>
> -/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
> +/*
> + * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
> + * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
> + */
> +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> + struct ovl_metacopy *data)
> {
> int res;
>
> @@ -1063,7 +1067,8 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
> if (!S_ISREG(d_inode(path->dentry)->i_mode))
> return 0;
>
> - res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, NULL, 0);
> + res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY,
> + data, data ? OVL_METACOPY_MAX_SIZE : 0);
> if (res < 0) {
> if (res == -ENODATA || res == -EOPNOTSUPP)
> return 0;
> @@ -1077,7 +1082,31 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
> goto out;
> }
>
> - return 1;
> + if (res == 0) {
> + /* Emulate empty data for zero size metacopy xattr */
> + res = OVL_METACOPY_MIN_SIZE;
> + if (data) {
> + memset(data, 0, res);
> + data->len = res;
> + }
> + } else if (res < OVL_METACOPY_MIN_SIZE) {
> + pr_warn_ratelimited("metacopy file '%pd' has too small xattr\n",
> + path->dentry);
> + return -EIO;
> + } else if (data) {
> + if (data->version != 0) {
> + pr_warn_ratelimited("metacopy file '%pd' has unsupported version\n",
> + path->dentry);
> + return -EIO;
> + }
> + if (res != data->len) {
> + pr_warn_ratelimited("metacopy file '%pd' has invalid xattr size\n",
> + path->dentry);
> + return -EIO;
> + }
> + }
> +
> + return res;
> out:
> pr_warn_ratelimited("failed to get metacopy (%i)\n", res);
> return res;
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
@ 2023-06-21 12:24 ` Amir Goldstein
2023-07-03 19:24 ` Eric Biggers
1 sibling, 0 replies; 17+ messages in thread
From: Amir Goldstein @ 2023-06-21 12:24 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, ebiggers, tytso, fsverity
On Wed, Jun 21, 2023 at 2:18 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> The new digest field in the metacopy xattr is used during lookup to
> record whether the header contained a digest in the OVL_HAS_DIGEST
> flags.
>
> When accessing file data the first time, if OVL_HAS_DIGEST is set, we
> reload the metadata and check that the source lowerdata inode matches
> the specified digest in it (according to the enabled verity
> options). If the verity check passes we store this info in the inode
> flags as OVL_VERIFIED_DIGEST, so that we can avoid doing it again if
> the inode remains in memory.
>
> The verification is done in ovl_maybe_validate_verity() which needs to
> be called in the same places as ovl_maybe_lookup_lowerdata(), so there
> is a new ovl_verify_lowerdata() helper that calls these in the right
> order, and all current callers of ovl_maybe_lookup_lowerdata() are
> changed to call it instead.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
Nice! looks much cleaner than v4.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/copy_up.c | 2 +-
> fs/overlayfs/file.c | 8 ++--
> fs/overlayfs/namei.c | 72 +++++++++++++++++++++++++++++++-
> fs/overlayfs/overlayfs.h | 11 ++++-
> fs/overlayfs/super.c | 5 ++-
> fs/overlayfs/util.c | 90 ++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 180 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 568f743a5584..68f01fd7f211 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -1078,7 +1078,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
> * not very important to optimize this case, so do lazy lowerdata lookup
> * before any copy up, so we can do it before taking ovl_inode_lock().
> */
> - err = ovl_maybe_lookup_lowerdata(dentry);
> + err = ovl_verify_lowerdata(dentry);
> if (err)
> return err;
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index cb53c84108fc..859a833c1035 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -115,8 +115,8 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
> if (allow_meta) {
> ovl_path_real(dentry, &realpath);
> } else {
> - /* lazy lookup of lowerdata */
> - err = ovl_maybe_lookup_lowerdata(dentry);
> + /* lazy lookup and verify of lowerdata */
> + err = ovl_verify_lowerdata(dentry);
> if (err)
> return err;
>
> @@ -159,8 +159,8 @@ static int ovl_open(struct inode *inode, struct file *file)
> struct path realpath;
> int err;
>
> - /* lazy lookup of lowerdata */
> - err = ovl_maybe_lookup_lowerdata(dentry);
> + /* lazy lookup and verify lowerdata */
> + err = ovl_verify_lowerdata(dentry);
> if (err)
> return err;
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 3dd480253710..d00ec43f2376 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -889,8 +889,58 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
> return err;
> }
>
> +static int ovl_maybe_validate_verity(struct dentry *dentry)
> +{
> + struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + struct inode *inode = d_inode(dentry);
> + struct path datapath, metapath;
> + int err;
> +
> + if (!ofs->config.verity_mode ||
> + !ovl_is_metacopy_dentry(dentry) ||
> + ovl_test_flag(OVL_VERIFIED_DIGEST, inode))
> + return 0;
> +
> + if (!ovl_test_flag(OVL_HAS_DIGEST, inode)) {
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + pr_warn_ratelimited("metacopy file '%pd' has no digest specified\n",
> + dentry);
> + return -EIO;
> + }
> + return 0;
> + }
> +
> + ovl_path_lowerdata(dentry, &datapath);
> + if (!datapath.dentry)
> + return -EIO;
> +
> + ovl_path_real(dentry, &metapath);
> + if (!metapath.dentry)
> + return -EIO;
> +
> + err = ovl_inode_lock_interruptible(inode);
> + if (err)
> + return err;
> +
> + if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) {
> + const struct cred *old_cred;
> +
> + old_cred = ovl_override_creds(dentry->d_sb);
> +
> + err = ovl_validate_verity(ofs, &metapath, &datapath);
> + if (err == 0)
> + ovl_set_flag(OVL_VERIFIED_DIGEST, inode);
> +
> + revert_creds(old_cred);
> + }
> +
> + ovl_inode_unlock(inode);
> +
> + return err;
> +}
> +
> /* Lazy lookup of lowerdata */
> -int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> +static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> {
> struct inode *inode = d_inode(dentry);
> const char *redirect = ovl_lowerdata_redirect(inode);
> @@ -935,6 +985,17 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> goto out;
> }
>
> +int ovl_verify_lowerdata(struct dentry *dentry)
> +{
> + int err;
> +
> + err = ovl_maybe_lookup_lowerdata(dentry);
> + if (err)
> + return err;
> +
> + return ovl_maybe_validate_verity(dentry);
> +}
> +
> struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> unsigned int flags)
> {
> @@ -955,6 +1016,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> unsigned int i;
> int err;
> bool uppermetacopy = false;
> + int metacopy_size = 0;
> struct ovl_lookup_data d = {
> .sb = dentry->d_sb,
> .name = dentry->d_name,
> @@ -999,6 +1061,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
> if (d.metacopy)
> uppermetacopy = true;
> + metacopy_size = d.metacopy;
> }
>
> if (d.redirect) {
> @@ -1076,6 +1139,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> origin = this;
> }
>
> + if (!upperdentry && !d.is_dir && !ctr && d.metacopy)
> + metacopy_size = d.metacopy;
> +
> if (d.metacopy && ctr) {
> /*
> * Do not store intermediate metacopy dentries in
> @@ -1215,6 +1281,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (err < 0)
> goto out_free_oe;
> uppermetacopy = err;
> + metacopy_size = err;
> }
>
> if (upperdentry || ctr) {
> @@ -1236,6 +1303,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_free_oe;
> if (upperdentry && !uppermetacopy)
> ovl_set_flag(OVL_UPPERDATA, inode);
> +
> + if (metacopy_size > OVL_METACOPY_MIN_SIZE)
> + ovl_set_flag(OVL_HAS_DIGEST, inode);
> }
>
> ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 6d4e08df0dfe..9fbbc077643b 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -50,6 +50,8 @@ enum ovl_inode_flag {
> OVL_UPPERDATA,
> /* Inode number will remain constant over copy up. */
> OVL_CONST_INO,
> + OVL_HAS_DIGEST,
> + OVL_VERIFIED_DIGEST,
> };
>
> enum ovl_entry_flag {
> @@ -513,8 +515,15 @@ void ovl_nlink_end(struct dentry *dentry);
> int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
> int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> struct ovl_metacopy *data);
> +int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> + struct ovl_metacopy *metacopy);
> bool ovl_is_metacopy_dentry(struct dentry *dentry);
> char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
> +int ovl_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)
> @@ -634,7 +643,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
> struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
> struct dentry *origin, bool verify);
> int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
> -int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
> +int ovl_verify_lowerdata(struct dentry *dentry);
> struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> unsigned int flags);
> bool ovl_lower_positive(struct dentry *dentry);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index a4eb9abd4b52..457a5bc439cb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -63,6 +63,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> const struct inode *inode)
> {
> struct dentry *real = NULL, *lower;
> + int err;
>
> /* It's an overlay file */
> if (inode && d_inode(dentry) == inode)
> @@ -89,7 +90,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> * uprobes on offset within the file, so lowerdata should be available
> * when setting the uprobe.
> */
> - ovl_maybe_lookup_lowerdata(dentry);
> + err = ovl_verify_lowerdata(dentry);
> + if (err)
> + goto bug;
> lower = ovl_dentry_lowerdata(dentry);
> if (!lower)
> goto bug;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 921747223991..927a1133859d 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -10,6 +10,7 @@
> #include <linux/cred.h>
> #include <linux/xattr.h>
> #include <linux/exportfs.h>
> +#include <linux/file.h>
> #include <linux/fileattr.h>
> #include <linux/uuid.h>
> #include <linux/namei.h>
> @@ -1112,6 +1113,18 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> return res;
> }
>
> +int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d, struct ovl_metacopy *metacopy)
> +{
> + size_t len = metacopy->len;
> +
> + /* If no flags or digest fall back to empty metacopy file */
> + if (metacopy->version == 0 && metacopy->flags == 0 && metacopy->digest_algo == 0)
> + len = 0;
> +
> + return ovl_check_setxattr(ofs, d, OVL_XATTR_METACOPY,
> + metacopy, len, -EOPNOTSUPP);
> +}
> +
> bool ovl_is_metacopy_dentry(struct dentry *dentry)
> {
> struct ovl_entry *oe = OVL_E(dentry);
> @@ -1174,6 +1187,83 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
> return ERR_PTR(res);
> }
>
> +/* Call with mounter creds as it may open the file */
> +static int ovl_ensure_verity_loaded(struct path *datapath)
> +{
> + struct inode *inode = d_inode(datapath->dentry);
> + const struct fsverity_info *vi;
> + struct file *filp;
> +
> + vi = fsverity_get_info(inode);
> + if (vi == NULL && IS_VERITY(inode)) {
> + /*
> + * If this inode was not yet opened, the verity info hasn't been
> + * loaded yet, so we need to do that here to force it into memory.
> + */
> + filp = kernel_file_open(datapath, O_RDONLY, inode, current_cred());
> + if (IS_ERR(filp))
> + return PTR_ERR(filp);
> + fput(filp);
> + }
> +
> + return 0;
> +}
> +
> +int ovl_validate_verity(struct ovl_fs *ofs,
> + struct path *metapath,
> + struct path *datapath)
> +{
> + struct ovl_metacopy metacopy_data;
> + u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE];
> + int xattr_digest_size, digest_size;
> + int xattr_size, err;
> + u8 verity_algo;
> +
> + if (!ofs->config.verity_mode ||
> + /* Verity only works on regular files */
> + !S_ISREG(d_inode(metapath->dentry)->i_mode))
> + return 0;
> +
> + xattr_size = ovl_check_metacopy_xattr(ofs, metapath, &metacopy_data);
> + if (xattr_size < 0)
> + return xattr_size;
> +
> + if (!xattr_size || !metacopy_data.digest_algo) {
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + pr_warn_ratelimited("metacopy file '%pd' has no digest specified\n",
> + metapath->dentry);
> + return -EIO;
> + }
> + return 0;
> + }
> +
> + xattr_digest_size = ovl_metadata_digest_size(&metacopy_data);
> +
> + err = ovl_ensure_verity_loaded(datapath);
> + if (err < 0) {
> + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
> + datapath->dentry);
> + return -EIO;
> + }
> +
> + digest_size = fsverity_get_digest(d_inode(datapath->dentry), actual_digest,
> + &verity_algo, NULL);
> + if (digest_size == 0) {
> + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry);
> + return -EIO;
> + }
> +
> + if (xattr_digest_size != digest_size ||
> + metacopy_data.digest_algo != verity_algo ||
> + memcmp(metacopy_data.digest, actual_digest, xattr_digest_size) != 0) {
> + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> + datapath->dentry);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> /*
> * ovl_sync_status() - Check fs sync status for volatile mounts
> *
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/4] ovl: Handle verity during copy-up
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
@ 2023-06-21 12:26 ` Amir Goldstein
2023-07-03 19:29 ` Eric Biggers
1 sibling, 0 replies; 17+ messages in thread
From: Amir Goldstein @ 2023-06-21 12:26 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, ebiggers, tytso, fsverity
On Wed, Jun 21, 2023 at 2:18 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> During regular metacopy, if lowerdata file has fs-verity enabled, and
> the verity option is enabled, we add the digest to the metacopy xattr.
>
> If verity is required, and lowerdata does not have fs-verity enabled,
> fall back to full copy-up (or the generated metacopy would not
> validate).
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
Looks good.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
> fs/overlayfs/overlayfs.h | 3 +++
> fs/overlayfs/util.c | 33 ++++++++++++++++++++++++++++-
> 3 files changed, 78 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 68f01fd7f211..fce7d048673c 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -544,6 +544,7 @@ struct ovl_copy_up_ctx {
> bool origin;
> bool indexed;
> bool metacopy;
> + bool metacopy_digest;
> };
>
> static int ovl_link_up(struct ovl_copy_up_ctx *c)
> @@ -641,8 +642,21 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
> }
>
> if (c->metacopy) {
> - err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
> - NULL, 0, -EOPNOTSUPP);
> + struct path lowerdatapath;
> + struct ovl_metacopy metacopy_data = OVL_METACOPY_INIT;
> +
> + ovl_path_lowerdata(c->dentry, &lowerdatapath);
> + if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> + err = -EIO;
> + else
> + err = ovl_set_verity_xattr_from(ofs, &lowerdatapath, &metacopy_data);
> +
> + if (metacopy_data.digest_algo)
> + c->metacopy_digest = true;
> +
> + if (!err)
> + err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> +
> if (err)
> return err;
> }
> @@ -751,6 +765,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (err)
> goto cleanup;
>
> + if (c->metacopy_digest)
> + ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + else
> + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> +
> if (!c->metacopy)
> ovl_set_upperdata(d_inode(c->dentry));
> inode = d_inode(c->dentry);
> @@ -813,6 +833,12 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
> if (err)
> goto out_fput;
>
> + if (c->metacopy_digest)
> + ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + else
> + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> +
> if (!c->metacopy)
> ovl_set_upperdata(d_inode(c->dentry));
> ovl_inode_update(d_inode(c->dentry), dget(temp));
> @@ -918,6 +944,19 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
> if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
> return false;
>
> + /* Fall back to full copy if no fsverity on source data and we require verity */
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + struct path lowerdata;
> +
> + ovl_path_lowerdata(dentry, &lowerdata);
> +
> + if (WARN_ON_ONCE(lowerdata.dentry == NULL) ||
> + ovl_ensure_verity_loaded(&lowerdata) ||
> + !fsverity_get_info(d_inode(lowerdata.dentry))) {
> + return false;
> + }
> + }
> +
> return true;
> }
>
> @@ -984,6 +1023,8 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> if (err)
> goto out_free;
>
> + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> ovl_set_upperdata(d_inode(c->dentry));
> out_free:
> kfree(capability);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 9fbbc077643b..e728360c66ff 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -519,11 +519,14 @@ int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> struct ovl_metacopy *metacopy);
> bool ovl_is_metacopy_dentry(struct dentry *dentry);
> char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
> +int ovl_ensure_verity_loaded(struct path *path);
> int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> u8 *digest_buf, int *buf_length);
> int ovl_validate_verity(struct ovl_fs *ofs,
> struct path *metapath,
> struct path *datapath);
> +int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
> + struct ovl_metacopy *metacopy);
> int ovl_sync_status(struct ovl_fs *ofs);
>
> static inline void ovl_set_flag(unsigned long flag, struct inode *inode)
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 927a1133859d..439e23496713 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1188,7 +1188,7 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa
> }
>
> /* Call with mounter creds as it may open the file */
> -static int ovl_ensure_verity_loaded(struct path *datapath)
> +int ovl_ensure_verity_loaded(struct path *datapath)
> {
> struct inode *inode = d_inode(datapath->dentry);
> const struct fsverity_info *vi;
> @@ -1264,6 +1264,37 @@ int ovl_validate_verity(struct ovl_fs *ofs,
> return 0;
> }
>
> +int ovl_set_verity_xattr_from(struct ovl_fs *ofs, struct path *src,
> + struct ovl_metacopy *metacopy)
> +{
> + int err, digest_size;
> +
> + if (!ofs->config.verity_mode || !S_ISREG(d_inode(src->dentry)->i_mode))
> + return 0;
> +
> + err = ovl_ensure_verity_loaded(src);
> + if (err < 0) {
> + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n",
> + src->dentry);
> + return -EIO;
> + }
> +
> + digest_size = fsverity_get_digest(d_inode(src->dentry),
> + metacopy->digest, &metacopy->digest_algo, NULL);
> + if (digest_size == 0 ||
> + WARN_ON_ONCE(digest_size > FS_VERITY_MAX_DIGEST_SIZE)) {
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n",
> + src->dentry);
> + return -EIO;
> + }
> + return 0;
> + }
> +
> + metacopy->len += digest_size;
> + return 0;
> +}
> +
> /*
> * ovl_sync_status() - Check fs sync status for volatile mounts
> *
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/4] ovl: Add framework for verity support
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
2023-06-21 12:18 ` Amir Goldstein
@ 2023-07-03 19:08 ` Eric Biggers
1 sibling, 0 replies; 17+ messages in thread
From: Eric Biggers @ 2023-07-03 19:08 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Wed, Jun 21, 2023 at 01:18:25PM +0200, Alexander Larsson wrote:
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index eb7d2c88ddec..b63e0db03631 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -405,6 +405,53 @@ 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
> +----------------------
> +
> +During metadata copy up of a lower file, if the source file has
> +fs-verity enabled and overlay verity support is enabled, then the
> +digest of the lower file is added to the "trusted.overlay.metacopy"
> +xattr. This is then used to verify the content of the lower file
> +each the time the metacopy file is opened.
> +
> +When a layer containing verity xattrs is used, it means that any such
> +metacopy file in the upper layer is guaranteed to match the content
> +that was in the lower at the time of the copy-up. If at any time
> +(during a mount, after a remount, etc) such a file in the lower is
> +replaced or modified in any way, access to the corresponding file in
> +overlayfs will result in EIO errors (either on open, due to overlayfs
> +digest check, or from a later read due to fs-verity) and a detailed
> +error is printed to the kernel logs. For more details of how fs-verity
> +file access works, see :ref:`Documentation/filesystems/fsverity.rst
> +<accessing_verity_files>`.
> +
> +Verity can be used as a general robustness check to detect accidental
> +changes in the overlayfs directories in use. But, with additional care
> +it can also give more powerful guarantees. For example, if the upper
> +layer is fully trusted (by using dm-verity or something similar), then
> +an untrusted lower layer can be used to supply validated file content
> +for all metacopy files. If additionally the untrusted lower
> +directories are specified as "Data-only", then they can only supply
> +such file content, and the entire mount can be trusted to match the
> +upper layer.
> +
> +This feature is controlled by the "verity" mount option, which
> +supports these values:
> +
> +- "off":
> + The metacopy digest is never generated or used. This is the
> + default if verity option is not specified.
> +- "on":
> + Whenever a metacopy files specifies an expected digest, the
> + corresponding data file must match the specified digest. When
> + generating a metacopy file the verity digest will be set in it
> + based on the source file (if it has one).
> +- "require":
> + Same as "on", but additionally all metacopy files must specify a
> + digest (or EIO is returned on open). This means metadata copy up
> + will only be used if the data file has fs-verity enabled,
> + otherwise a full copy-up is used.
> +
Thanks, it's not perfect but it's much improved.
Acked-by: Eric Biggers <ebiggers@google.com>
- Eric
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
2023-06-21 12:21 ` Amir Goldstein
@ 2023-07-03 19:13 ` Eric Biggers
2023-07-05 8:07 ` Alexander Larsson
1 sibling, 1 reply; 17+ messages in thread
From: Eric Biggers @ 2023-07-03 19:13 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote:
> Historically overlay.metacopy was a zero-size xattr, and it's
> existence marked a metacopy file. This change adds a versioned header
> with a flag field, a length and a digest. The initial use-case of this
> will be for validating a fs-verity digest, but the flags field could
> also be used later for other new features.
>
> ovl_check_metacopy_xattr() now returns the size of the xattr,
> emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
> distinguish it from the no-xattr case.
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
> fs/overlayfs/namei.c | 10 +++++-----
> fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
> fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
> 3 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 57adf911735f..3dd480253710 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -25,7 +25,7 @@ struct ovl_lookup_data {
> bool stop;
> bool last;
> char *redirect;
> - bool metacopy;
> + int metacopy;
Should this be called 'metacopy_size' now?
> - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
> + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
> if (err < 0)
> goto out_err;
This part is confusing because variables named 'err' conventionally contain only
0 or a negative errno value. But this patch makes it possible for
ovl_check_metacopy_xattr() to return a positive size.
- Eric
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
2023-06-21 12:24 ` Amir Goldstein
@ 2023-07-03 19:24 ` Eric Biggers
2023-07-05 9:09 ` Alexander Larsson
1 sibling, 1 reply; 17+ messages in thread
From: Eric Biggers @ 2023-07-03 19:24 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Wed, Jun 21, 2023 at 01:18:27PM +0200, Alexander Larsson wrote:
> +static int ovl_ensure_verity_loaded(struct path *datapath)
> +{
> + struct inode *inode = d_inode(datapath->dentry);
> + const struct fsverity_info *vi;
> + struct file *filp;
> +
> + vi = fsverity_get_info(inode);
> + if (vi == NULL && IS_VERITY(inode)) {
Can you please use '!fsverity_active(inode)' instead of
'fsverity_get_info(inode) == NULL'? The result is exactly the same, but
fsverity_active() is the intended "API" for code outside fs/verity/.
fsverity_get_info() is in the header only because fsverity_active() calls it.
Same comment in ovl_need_meta_copy_up().
- Eric
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/4] ovl: Handle verity during copy-up
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
2023-06-21 12:26 ` Amir Goldstein
@ 2023-07-03 19:29 ` Eric Biggers
2023-07-05 9:11 ` Alexander Larsson
1 sibling, 1 reply; 17+ messages in thread
From: Eric Biggers @ 2023-07-03 19:29 UTC (permalink / raw)
To: Alexander Larsson; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Wed, Jun 21, 2023 at 01:18:28PM +0200, Alexander Larsson wrote:
> During regular metacopy, if lowerdata file has fs-verity enabled, and
> the verity option is enabled, we add the digest to the metacopy xattr.
>
> If verity is required, and lowerdata does not have fs-verity enabled,
> fall back to full copy-up (or the generated metacopy would not
> validate).
>
> Signed-off-by: Alexander Larsson <alexl@redhat.com>
> ---
> fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
> fs/overlayfs/overlayfs.h | 3 +++
> fs/overlayfs/util.c | 33 ++++++++++++++++++++++++++++-
> 3 files changed, 78 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 68f01fd7f211..fce7d048673c 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -544,6 +544,7 @@ struct ovl_copy_up_ctx {
> bool origin;
> bool indexed;
> bool metacopy;
> + bool metacopy_digest;
> };
>
> static int ovl_link_up(struct ovl_copy_up_ctx *c)
> @@ -641,8 +642,21 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
> }
>
> if (c->metacopy) {
> - err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
> - NULL, 0, -EOPNOTSUPP);
> + struct path lowerdatapath;
> + struct ovl_metacopy metacopy_data = OVL_METACOPY_INIT;
> +
> + ovl_path_lowerdata(c->dentry, &lowerdatapath);
> + if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> + err = -EIO;
> + else
> + err = ovl_set_verity_xattr_from(ofs, &lowerdatapath, &metacopy_data);
There's no dedicated verity xattr anymore, so maybe ovl_set_verity_xattr_from()
should be renamed to something like ovl_get_verity_digest().
> +
> + if (metacopy_data.digest_algo)
> + c->metacopy_digest = true;
> +
> + if (!err)
> + err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> +
> if (err)
> return err;
The error handling above is a bit weird. Some early returns would make it
easier to read:
ovl_path_lowerdata(c->dentry, &lowerdatapath);
if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
return -EIO;
err = ovl_get_verity_digest(ofs, &lowerdatapath, &metacopy_data);
if (err)
return err;
if (metacopy_data.digest_algo)
c->metacopy_digest = true;
err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
if (err)
return err;
> }
> @@ -751,6 +765,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (err)
> goto cleanup;
>
> + if (c->metacopy_digest)
> + ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + else
> + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> +
> if (!c->metacopy)
> ovl_set_upperdata(d_inode(c->dentry));
> inode = d_inode(c->dentry);
Maybe the line 'inode = d_inode(c->dentry);' should be moved earlier, and then
'inode' used instead of 'd_inode(c->dentry)' later on.
> + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> + struct path lowerdata;
> +
> + ovl_path_lowerdata(dentry, &lowerdata);
> +
> + if (WARN_ON_ONCE(lowerdata.dentry == NULL) ||
> + ovl_ensure_verity_loaded(&lowerdata) ||
> + !fsverity_get_info(d_inode(lowerdata.dentry))) {
> + return false;
Please use !fsverity_active() instead of !fsverity_get_info().
- Eric
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
2023-07-03 19:13 ` Eric Biggers
@ 2023-07-05 8:07 ` Alexander Larsson
2023-07-05 13:12 ` Amir Goldstein
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Larsson @ 2023-07-05 8:07 UTC (permalink / raw)
To: Eric Biggers; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote:
> > Historically overlay.metacopy was a zero-size xattr, and it's
> > existence marked a metacopy file. This change adds a versioned header
> > with a flag field, a length and a digest. The initial use-case of this
> > will be for validating a fs-verity digest, but the flags field could
> > also be used later for other new features.
> >
> > ovl_check_metacopy_xattr() now returns the size of the xattr,
> > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
> > distinguish it from the no-xattr case.
> >
> > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > ---
> > fs/overlayfs/namei.c | 10 +++++-----
> > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
> > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
> > 3 files changed, 61 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 57adf911735f..3dd480253710 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -25,7 +25,7 @@ struct ovl_lookup_data {
> > bool stop;
> > bool last;
> > char *redirect;
> > - bool metacopy;
> > + int metacopy;
>
> Should this be called 'metacopy_size' now?
Honestly I don't know. That would change a lot of locations that still
use this as "essentially" a boolean (i.e. != 0 means "has metacopy"),
and ity would make that code less compact. I guess this is up to Amir
and Miklos. Surely we could add a comment in the struct definition
though.
> > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
> > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
> > if (err < 0)
> > goto out_err;
>
> This part is confusing because variables named 'err' conventionally contain only
> 0 or a negative errno value. But this patch makes it possible for
> ovl_check_metacopy_xattr() to return a positive size.
It was already returning "negative, 0 or 1", so it's not fundamentally
changed. Again, this is not my code so I'd rather Amir and Miklos
decide such code style questions.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@redhat.com alexander.larsson@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata
2023-07-03 19:24 ` Eric Biggers
@ 2023-07-05 9:09 ` Alexander Larsson
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-07-05 9:09 UTC (permalink / raw)
To: Eric Biggers; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Mon, Jul 3, 2023 at 9:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 21, 2023 at 01:18:27PM +0200, Alexander Larsson wrote:
> > +static int ovl_ensure_verity_loaded(struct path *datapath)
> > +{
> > + struct inode *inode = d_inode(datapath->dentry);
> > + const struct fsverity_info *vi;
> > + struct file *filp;
> > +
> > + vi = fsverity_get_info(inode);
> > + if (vi == NULL && IS_VERITY(inode)) {
>
> Can you please use '!fsverity_active(inode)' instead of
> 'fsverity_get_info(inode) == NULL'? The result is exactly the same, but
> fsverity_active() is the intended "API" for code outside fs/verity/.
> fsverity_get_info() is in the header only because fsverity_active() calls it.
Changed this in git:
https://github.com/alexlarsson/linux/tree/overlay-verity
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@redhat.com alexander.larsson@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/4] ovl: Handle verity during copy-up
2023-07-03 19:29 ` Eric Biggers
@ 2023-07-05 9:11 ` Alexander Larsson
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Larsson @ 2023-07-05 9:11 UTC (permalink / raw)
To: Eric Biggers; +Cc: miklos, linux-unionfs, amir73il, tytso, fsverity
On Mon, Jul 3, 2023 at 9:30 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 21, 2023 at 01:18:28PM +0200, Alexander Larsson wrote:
> > During regular metacopy, if lowerdata file has fs-verity enabled, and
> > the verity option is enabled, we add the digest to the metacopy xattr.
> >
> > If verity is required, and lowerdata does not have fs-verity enabled,
> > fall back to full copy-up (or the generated metacopy would not
> > validate).
> >
> > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > ---
> > fs/overlayfs/copy_up.c | 45 ++++++++++++++++++++++++++++++++++++++--
> > fs/overlayfs/overlayfs.h | 3 +++
> > fs/overlayfs/util.c | 33 ++++++++++++++++++++++++++++-
> > 3 files changed, 78 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 68f01fd7f211..fce7d048673c 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -544,6 +544,7 @@ struct ovl_copy_up_ctx {
> > bool origin;
> > bool indexed;
> > bool metacopy;
> > + bool metacopy_digest;
> > };
> >
> > static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > @@ -641,8 +642,21 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > }
> >
> > if (c->metacopy) {
> > - err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
> > - NULL, 0, -EOPNOTSUPP);
> > + struct path lowerdatapath;
> > + struct ovl_metacopy metacopy_data = OVL_METACOPY_INIT;
> > +
> > + ovl_path_lowerdata(c->dentry, &lowerdatapath);
> > + if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> > + err = -EIO;
> > + else
> > + err = ovl_set_verity_xattr_from(ofs, &lowerdatapath, &metacopy_data);
>
> There's no dedicated verity xattr anymore, so maybe ovl_set_verity_xattr_from()
> should be renamed to something like ovl_get_verity_digest().
Yeah, that makes a lot of sense.
> > +
> > + if (metacopy_data.digest_algo)
> > + c->metacopy_digest = true;
> > +
> > + if (!err)
> > + err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> > +
> > if (err)
> > return err;
>
> The error handling above is a bit weird. Some early returns would make it
> easier to read:
>
> ovl_path_lowerdata(c->dentry, &lowerdatapath);
> if (WARN_ON_ONCE(lowerdatapath.dentry == NULL))
> return -EIO;
> err = ovl_get_verity_digest(ofs, &lowerdatapath, &metacopy_data);
> if (err)
> return err;
>
> if (metacopy_data.digest_algo)
> c->metacopy_digest = true;
>
> err = ovl_set_metacopy_xattr(ofs, temp, &metacopy_data);
> if (err)
> return err;
Yeah.
> > }
> > @@ -751,6 +765,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > if (err)
> > goto cleanup;
> >
> > + if (c->metacopy_digest)
> > + ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> > + else
> > + ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
> > + ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
> > +
> > if (!c->metacopy)
> > ovl_set_upperdata(d_inode(c->dentry));
> > inode = d_inode(c->dentry);
>
> Maybe the line 'inode = d_inode(c->dentry);' should be moved earlier, and then
> 'inode' used instead of 'd_inode(c->dentry)' later on.
I wonder why this wasn't already done for the ovl_set_upperdata(), but
it seems ok so I did this.
> > + if (ofs->config.verity_mode == OVL_VERITY_REQUIRE) {
> > + struct path lowerdata;
> > +
> > + ovl_path_lowerdata(dentry, &lowerdata);
> > +
> > + if (WARN_ON_ONCE(lowerdata.dentry == NULL) ||
> > + ovl_ensure_verity_loaded(&lowerdata) ||
> > + !fsverity_get_info(d_inode(lowerdata.dentry))) {
> > + return false;
>
> Please use !fsverity_active() instead of !fsverity_get_info().
Done.
All these changes are in git:
https://github.com/alexlarsson/linux/commits/overlay-verity
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@redhat.com alexander.larsson@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr
2023-07-05 8:07 ` Alexander Larsson
@ 2023-07-05 13:12 ` Amir Goldstein
0 siblings, 0 replies; 17+ messages in thread
From: Amir Goldstein @ 2023-07-05 13:12 UTC (permalink / raw)
To: Alexander Larsson; +Cc: Eric Biggers, miklos, linux-unionfs, tytso, fsverity
On Wed, Jul 5, 2023 at 11:07 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote:
> > > Historically overlay.metacopy was a zero-size xattr, and it's
> > > existence marked a metacopy file. This change adds a versioned header
> > > with a flag field, a length and a digest. The initial use-case of this
> > > will be for validating a fs-verity digest, but the flags field could
> > > also be used later for other new features.
> > >
> > > ovl_check_metacopy_xattr() now returns the size of the xattr,
> > > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to
> > > distinguish it from the no-xattr case.
> > >
> > > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > > ---
> > > fs/overlayfs/namei.c | 10 +++++-----
> > > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++-
> > > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++----
> > > 3 files changed, 61 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index 57adf911735f..3dd480253710 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -25,7 +25,7 @@ struct ovl_lookup_data {
> > > bool stop;
> > > bool last;
> > > char *redirect;
> > > - bool metacopy;
> > > + int metacopy;
> >
> > Should this be called 'metacopy_size' now?
>
> Honestly I don't know. That would change a lot of locations that still
> use this as "essentially" a boolean (i.e. != 0 means "has metacopy"),
> and ity would make that code less compact. I guess this is up to Amir
> and Miklos. Surely we could add a comment in the struct definition
> though.
>
I agree most of the code looks nicer when this stays 'metacopy'
> > > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path);
> > > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL);
> > > if (err < 0)
> > > goto out_err;
> >
> > This part is confusing because variables named 'err' conventionally contain only
> > 0 or a negative errno value. But this patch makes it possible for
> > ovl_check_metacopy_xattr() to return a positive size.
>
> It was already returning "negative, 0 or 1", so it's not fundamentally
> changed. Again, this is not my code so I'd rather Amir and Miklos
> decide such code style questions.
>
I agree. It wasn't 0 or negative before the change and I don't
think this "convention" justifies adding another var.
Thanks,
Amir,
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-07-05 13:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-21 11:18 [PATCH v5 0/4] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 1/4] ovl: Add framework for verity support Alexander Larsson
2023-06-21 12:18 ` Amir Goldstein
2023-07-03 19:08 ` Eric Biggers
2023-06-21 11:18 ` [PATCH v4 2/4] ovl: Add versioned header for overlay.metacopy xattr Alexander Larsson
2023-06-21 12:21 ` Amir Goldstein
2023-07-03 19:13 ` Eric Biggers
2023-07-05 8:07 ` Alexander Larsson
2023-07-05 13:12 ` Amir Goldstein
2023-06-21 11:18 ` [PATCH v4 3/4] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
2023-06-21 12:24 ` Amir Goldstein
2023-07-03 19:24 ` Eric Biggers
2023-07-05 9:09 ` Alexander Larsson
2023-06-21 11:18 ` [PATCH v4 4/4] ovl: Handle verity during copy-up Alexander Larsson
2023-06-21 12:26 ` Amir Goldstein
2023-07-03 19:29 ` Eric Biggers
2023-07-05 9:11 ` Alexander Larsson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox