Re: overlayfs: verity validation broken since f77f281b6118
"Colin Walters" <[email protected]> Tue, 05 May 2026 12:51:47 -0400
| Newsgroups | dev.linux.lists.fsverity,org.kernel.vger.linux-fsdevel |
|---|---|
| Message-ID | <[email protected]> |
On Sat, May 2, 2026, at 5:23 AM, Amir Goldstein wrote: > On Fri, May 01, 2026 at 01:14:54PM -0400, Colin Walters wrote: >> Hi Christoph & Eric, >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f77f281b6118 broke composefs's usage of overlayfs verity=require, this was reported originally in https://github.com/bootc-dev/bootc/issues/2174 >> >> There's some output from an agent run I had in the <details> there, but here's an xfstests patch that passes on without that commit and fails with it. >> >> From 14231122bfd1e41337e4fb847acbbe038457c32a Mon Sep 17 00:00:00 2001 >> From: Colin Walters <[email protected]> >> Date: Fri, 1 May 2026 09:45:58 -0400 >> Subject: [PATCH] overlay/118: test fsverity lazy load through metacopy overlay >> >> Reproduces the regression reported at: >> https://github.com/bootc-dev/bootc/issues/2174 >> >> A recent change in how fsverity state was cached in memory >> I think caused inodes not in cache to appear to have >> missing verity=require for overlayfs. >> >> This test catches that. >> >> Generated-by: OpenCode (Claude Sonnet 4.5) >> Signed-off-by: Colin Walters <[email protected]> >> --- >> tests/overlay/118 | 62 +++++++++++++++++++++++++++++++++++++++++++ >> tests/overlay/118.out | 1 + > > > Please use free test numbers below 100 OK, I can resend with that change if that's the only thing. > > Is there a kernel fix for this? please mention it. Not that I know of. I did have my agent framework (opencode + combo of Gemini+Claude models) generate one initially, but I intentionally didn't post it because the generating is ~easy, verifying it's "good" is another thing and my C has bitrotted a bit (in favor of Rust mostly but I have to deal with a lot of Go too). Anyways, this trivial change works: diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 13cb60b52bd6..af8f6c2989ce 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -1039,7 +1039,7 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode, if (WARN_ON_ONCE(lowerdata.dentry == NULL) || ovl_ensure_verity_loaded(&lowerdata) || - !fsverity_active(d_inode(lowerdata.dentry))) { + !fsverity_get_info(d_inode(lowerdata.dentry))) { return false; } } diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 7b86a6bac644..bfdf9310ee78 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -1354,7 +1354,7 @@ int ovl_ensure_verity_loaded(const struct path *datapath) struct inode *inode = d_inode(datapath->dentry); struct file *filp; - if (!fsverity_active(inode) && IS_VERITY(inode)) { + if (IS_VERITY(inode) && !fsverity_get_info(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. However, when I looked at this more closely I felt the APIs are too confusing, and my Rust-ified mindset especially didn't like the side-effectful nature of ovl_ensure_verity_loaded(). So here's a much bigger patch that looks a lot nicer to me upon a quick local review: diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index af8f6c2989ce..9d3908666b7e 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -1037,9 +1037,11 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode, ovl_path_lowerdata(dentry, &lowerdata); + struct inode *inode = d_inode(lowerdata.dentry); + if (WARN_ON_ONCE(lowerdata.dentry == NULL) || - ovl_ensure_verity_loaded(&lowerdata) || - !fsverity_get_info(d_inode(lowerdata.dentry))) { + !IS_VERITY(inode) || + IS_ERR(fsverity_get_or_load_info(inode))) { return false; } } diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index b75df37f70ac..4d23ff29a4c3 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -581,7 +581,6 @@ 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(const struct path *path); int ovl_validate_verity(struct ovl_fs *ofs, const struct path *metapath, const struct path *datapath); diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index bfdf9310ee78..99aa2312de39 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -1348,25 +1348,6 @@ 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 */ -int ovl_ensure_verity_loaded(const struct path *datapath) -{ - struct inode *inode = d_inode(datapath->dentry); - struct file *filp; - - if (IS_VERITY(inode) && !fsverity_get_info(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, current_cred()); - if (IS_ERR(filp)) - return PTR_ERR(filp); - fput(filp); - } - - return 0; -} int ovl_validate_verity(struct ovl_fs *ofs, const struct path *metapath, @@ -1375,7 +1356,7 @@ int ovl_validate_verity(struct ovl_fs *ofs, struct ovl_metacopy metacopy_data; u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE]; int xattr_digest_size, digest_size; - int xattr_size, err; + int xattr_size; u8 verity_algo; if (!ofs->config.verity_mode || @@ -1398,16 +1379,9 @@ int ovl_validate_verity(struct ovl_fs *ofs, 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) { + if (digest_size <= 0) { pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); return -EIO; } @@ -1426,21 +1400,14 @@ int ovl_validate_verity(struct ovl_fs *ofs, int ovl_get_verity_digest(struct ovl_fs *ofs, const struct path *src, struct ovl_metacopy *metacopy) { - int err, digest_size; + int 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 || + 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", diff --git a/fs/verity/measure.c b/fs/verity/measure.c index 6a35623ebdf0..4ccf2ab8a70a 100644 --- a/fs/verity/measure.c +++ b/fs/verity/measure.c @@ -68,9 +68,9 @@ EXPORT_SYMBOL_GPL(fsverity_ioctl_measure); * @alg: (out) the digest's algorithm, as a FS_VERITY_HASH_ALG_* value * @halg: (out) the digest's algorithm, as a HASH_ALGO_* value * - * Retrieves the fsverity digest of the given file. The file must have been - * opened at least once since the inode was last loaded into the inode cache; - * otherwise this function will not recognize when fsverity is enabled. + * Retrieves the fsverity digest of the given file. If the inode has the + * S_VERITY flag set but the fsverity_info has not yet been loaded into the + * in-memory hash table, this function will load it from disk automatically. * * The file's fsverity digest consists of @raw_digest in combination with either * @alg or @halg. (The caller can choose which one of @alg or @halg to use.) @@ -80,8 +80,8 @@ EXPORT_SYMBOL_GPL(fsverity_ioctl_measure); * provides no security guarantee for users who ignore the algorithm ID, even if * they use the digest size (since algorithms can share the same digest size). * - * Return: The size of the raw digest in bytes, or 0 if the file doesn't have - * fsverity enabled. + * Return: The size of the raw digest in bytes, 0 if the file doesn't have + * fsverity enabled, or -errno on error. */ int fsverity_get_digest(struct inode *inode, u8 raw_digest[FS_VERITY_MAX_DIGEST_SIZE], @@ -90,10 +90,13 @@ int fsverity_get_digest(struct inode *inode, const struct fsverity_info *vi; const struct fsverity_hash_alg *hash_alg; - vi = fsverity_get_info(inode); - if (!vi) + if (!IS_VERITY(inode)) return 0; /* not a verity file */ + vi = fsverity_get_or_load_info(inode); + if (IS_ERR(vi)) + return PTR_ERR(vi); + hash_alg = vi->tree_params.hash_alg; memcpy(raw_digest, vi->file_digest, hash_alg->digest_size); if (alg) @@ -116,6 +119,13 @@ __bpf_kfunc_start_defs(); * * Read fsverity_digest of *file* into *digest_ptr*. * + * If the file's fsverity_info has not yet been loaded into the in-memory hash + * table (i.e. the inode is "cold"), this function will read the verity + * descriptor from disk before returning. This may block on I/O. The + * function is restricted to BPF_PROG_TYPE_LSM hooks (sleepable context), so + * blocking is safe, but callers should be aware of the potential latency on + * the first call for a given inode. + * * Return: 0 on success, a negative value on error. */ __bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr *digest_p) @@ -138,10 +148,13 @@ __bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr *di if (!IS_ALIGNED((uintptr_t)arg, __alignof__(*arg))) return -EINVAL; - vi = fsverity_get_info(inode); - if (!vi) + if (!IS_VERITY(inode)) return -ENODATA; /* not a verity file */ + vi = fsverity_get_or_load_info(inode); + if (IS_ERR(vi)) + return PTR_ERR(vi); + hash_alg = vi->tree_params.hash_alg; arg->digest_algorithm = hash_alg - fsverity_hash_algs; diff --git a/fs/verity/open.c b/fs/verity/open.c index dfa0d1afe0fe..4db71b396cdb 100644 --- a/fs/verity/open.c +++ b/fs/verity/open.c @@ -344,24 +344,48 @@ int fsverity_get_descriptor(struct inode *inode, return 0; } -static int ensure_verity_info(struct inode *inode) +/** + * fsverity_get_or_load_info() - get the fsverity_info for a verity inode, + * loading it from disk if necessary + * @inode: inode with the S_VERITY flag set + * + * If the fsverity_info has already been loaded into the in-memory hash table + * (i.e. the file has been opened), return it immediately. Otherwise read the + * verity descriptor from disk and populate the hash table entry. + * + * Unlike fsverity_get_info(), this function never returns NULL for a verity + * inode — it either returns the fsverity_info or an error. + * + * On encrypted verity files, the fscrypt key must already be set up on the + * inode (e.g. via a prior fscrypt_file_open()) before calling this function, + * as the descriptor read uses the existing crypto context rather than + * establishing one. Callers such as overlayfs that reach this path during + * copy-up already satisfy this precondition. + * + * The caller must ensure @inode has the S_VERITY flag set. + * + * Return: the fsverity_info on success, ERR_PTR(-errno) on failure + */ +struct fsverity_info *fsverity_get_or_load_info(struct inode *inode) { - struct fsverity_info *vi = fsverity_get_info(inode), *found; + struct fsverity_info *vi, *found; struct fsverity_descriptor *desc; int err; + if (WARN_ON_ONCE(!IS_VERITY(inode))) + return ERR_PTR(-EINVAL); + + vi = fsverity_get_info(inode); if (vi) - return 0; + return vi; err = fsverity_get_descriptor(inode, &desc); if (err) - return err; + return ERR_PTR(err); vi = fsverity_create_info(inode, desc); - if (IS_ERR(vi)) { - err = PTR_ERR(vi); + if (IS_ERR(vi)) goto out_free_desc; - } /* * Multiple tasks may race to set the inode's verity info, in which case @@ -372,20 +396,20 @@ static int ensure_verity_info(struct inode *inode) fsverity_info_hash_params); if (found) { fsverity_free_info(vi); - if (IS_ERR(found)) - err = PTR_ERR(found); + vi = found; /* either the winner's entry or ERR_PTR */ } out_free_desc: kfree(desc); - return err; + return vi; } +EXPORT_SYMBOL_GPL(fsverity_get_or_load_info); int __fsverity_file_open(struct inode *inode, struct file *filp) { if (filp->f_mode & FMODE_WRITE) return -EPERM; - return ensure_verity_info(inode); + return PTR_ERR_OR_ZERO(fsverity_get_or_load_info(inode)); } EXPORT_SYMBOL_GPL(__fsverity_file_open); diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h index a8f9aa75b792..0ea8ef2348d2 100644 --- a/include/linux/fsverity.h +++ b/include/linux/fsverity.h @@ -136,13 +136,25 @@ struct fsverity_operations { #ifdef CONFIG_FS_VERITY /** - * fsverity_active() - do reads from the inode need to go through fs-verity? + * fsverity_active() - does the inode have the S_VERITY flag set? * @inode: inode to check * - * This checks whether the inode's verity info has been set, and reads need - * to verify the file data. + * Check whether the inode has the S_VERITY flag set. This flag is set + * when the filesystem marks the inode as a verity file, and it persists + * for the lifetime of the inode in the inode cache. * - * Return: true if reads need to go through fs-verity, otherwise false + * Note: S_VERITY being set does NOT mean the fsverity_info has been loaded + * into the in-memory hash table yet. The info is loaded lazily on first + * open (via __fsverity_file_open()). Use fsverity_get_info() to check + * whether the info is loaded, or fsverity_get_or_load_info() to load it + * on demand. + * + * The smp_mb() pairs with the try_cmpxchg in set_mask_bits() that SETS the + * S_VERITY bit in i_flags. This ensures that stores made before the flag + * was set (in particular, the hash table insertion of the fsverity_info) are + * visible to any CPU that subsequently observes S_VERITY set. + * + * Return: true if the inode has S_VERITY set, otherwise false */ static inline bool fsverity_active(const struct inode *inode) { @@ -159,13 +171,26 @@ static inline bool fsverity_active(const struct inode *inode) } struct fsverity_info *__fsverity_get_info(const struct inode *inode); + /** - * fsverity_get_info - get fsverity information for an inode - * @inode: inode to operate on. + * fsverity_get_info() - get the in-memory fsverity_info for an inode + * @inode: inode to look up * - * This gets the fsverity_info for @inode if it exists. Safe to call without - * knowin that a fsverity_info exist for @inode, including on file systems that - * do not support fsverity. + * Return the fsverity_info for @inode if it has been loaded into the + * in-memory hash table, otherwise return NULL. + * + * A NULL return has two distinct meanings: + * 1. The inode is not a verity file at all (S_VERITY not set). + * 2. The inode IS a verity file, but its info has not been loaded yet + * because the file has not been opened since the inode was last evicted. + * + * Callers that need to distinguish these cases should check IS_VERITY(inode) + * alongside the return value. Callers that need the info to always be + * available should use fsverity_get_or_load_info() instead. + * + * Safe to call on any inode, even on filesystems that do not support fsverity. + * + * Return: the fsverity_info if loaded, NULL otherwise */ static inline struct fsverity_info *fsverity_get_info(const struct inode *inode) { @@ -187,6 +212,7 @@ int fsverity_get_digest(struct inode *inode, /* open.c */ +struct fsverity_info *fsverity_get_or_load_info(struct inode *inode); int __fsverity_file_open(struct inode *inode, struct file *filp); /* read_metadata.c */ @@ -242,6 +268,11 @@ static inline int fsverity_get_digest(struct inode *inode, /* open.c */ +static inline struct fsverity_info *fsverity_get_or_load_info(struct inode *inode) +{ + return ERR_PTR(-EOPNOTSUPP); +} + static inline int __fsverity_file_open(struct inode *inode, struct file *filp) { return -EOPNOTSUPP; diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 0916f24f005f..1eabb2d76961 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -213,9 +213,14 @@ static bool ima_get_verity_digest(struct ima_iint_cache *iint, /* * On failure, 'measure' policy rules will result in a file data * hash containing 0's. + * + * fsverity_get_digest() returns 0 if fsverity is not enabled on the + * file, or a negative errno if the verity info could not be loaded + * (e.g. -EIO reading the descriptor from disk). Treat both cases the + * same: fall back to the file data hash. */ digest_len = fsverity_get_digest(inode, hash->digest, NULL, &alg); - if (digest_len == 0) + if (digest_len <= 0) return false; /* diff --git a/security/ipe/eval.c b/security/ipe/eval.c index 21439c5be336..1910f4b3641b 100644 --- a/security/ipe/eval.c +++ b/security/ipe/eval.c @@ -204,10 +204,10 @@ static bool evaluate_fsv_digest(const struct ipe_eval_ctx *const ctx, if (!ctx->ino) return false; - if (!fsverity_get_digest((struct inode *)ctx->ino, - digest, - NULL, - &alg)) + if (fsverity_get_digest((struct inode *)ctx->ino, + digest, + NULL, + &alg) <= 0) return false; info.alg = hash_algo_name[alg]; -- 2.52.0