Skip to contentSkip to conversation

fsverity: RCU-delay the freeing of struct fsverity_info

PATCH Applied Eric Biggers 3 participants 4 messages Oct 1 – Oct 5

Subsystems (3)
Stable branch VFS fs-verity: read-only file-based authenticity protection
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

PATCH 1/1 fsverity: RCU-delay the freeing of struct fsverity_info

__fsverity_get_info() calls rhashtable_lookup_fast(), which just uses rcu_read_lock() and doesn't directly synchronize with fsverity_remove_info().

For the same inode this isn't a problem: its fsverity_info is removed only at inode eviction time or upon failure to enable verity, when the inode no longer needs its fsverity_info and it will no longer be accessed via that inode.

However, this is broken and can cause a use-after-free for concurrent __fsverity_get_info() for *different* inodes. Those rely on following fsverity_info::rhash_head in the rhashtable under rcu_read_lock() only. They also use fsverity_info::inode to do the key comparison.

Fix this by RCU-delaying the freeing of 'struct fsverity_info' after it's been removed from the rhashtable.

Fixes f77f281b6118 ("fsverity: use a hashtable to find the fsverity_info")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 fs/verity/fsverity_private.h | 15 ++++++++++++++-
 fs/verity/open.c             | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)
fs/verity/fsverity_private.h +14 -1
index 881d46f25e08..6f85f63861aa 100644
@@ -11,6 +11,7 @@
 #define pr_fmt(fmt) "fs-verity: " fmt
 
 #include <linux/fsverity.h>
+#include <linux/rcupdate.h>
 #include <linux/rhashtable.h>
 
 /*

@@ -76,7 +77,19 @@ struct merkle_tree_params {
 struct fsverity_info {
 	struct rhash_head rhash_head;
 	struct merkle_tree_params tree_params;
-	u8 root_hash[FS_VERITY_MAX_DIGEST_SIZE];
+	union {
+		u8 root_hash[FS_VERITY_MAX_DIGEST_SIZE];
+
+		/*
+		 * Used for final freeing of the struct, in which case root_hash
+		 * is no longer needed.  Specifically, the only fields needed
+		 * besides the rcu_head are those needed by fsverity_free_info()
+		 * itself, and those that can be accessed by fsverity_get_info()
+		 * (rhash_head and inode) during the RCU-protected rhashtable
+		 * chain search trying to find another inode's fsverity_info.
+		 */
+		struct rcu_head rcu_head;
+	};
 	u8 file_digest[FS_VERITY_MAX_DIGEST_SIZE];
 	struct inode *inode;
 	unsigned long *hash_block_verified;
fs/verity/open.c +12 -1
index d0c56a7faa3b..52c8cd014921 100644
@@ -399,11 +399,22 @@ void fsverity_free_info(struct fsverity_info *vi)
 	kmem_cache_free(fsverity_info_cachep, vi);
 }
 
+static void fsverity_free_info_rcu(struct rcu_head *head)
+{
+	fsverity_free_info(container_of(head, struct fsverity_info, rcu_head));
+}
+
 void fsverity_remove_info(struct fsverity_info *vi)
 {
 	rhashtable_remove_fast(&fsverity_info_hash, &vi->rhash_head,
 			       fsverity_info_hash_params);
-	fsverity_free_info(vi);
+	/*
+	 * The freeing must be RCU-delayed because __fsverity_get_info() for a
+	 * *different* inode's fsverity_info in the same rhashtable hash chain
+	 * can still be accessing *this* fsverity_info's rhash_head and inode
+	 * fields as part of the RCU-protected hash chain search.
+	 */
+	call_rcu(&vi->rcu_head, fsverity_free_info_rcu);
 }
 
 void fsverity_cleanup_inode(struct inode *inode)

base-commit: 551c722f40809618230001baccf219193e22fc5a

Signature · 1 line
2.56.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 18 lines · Eric Biggers

__fsverity_get_info() calls rhashtable_lookup_fast(), which just uses rcu_read_lock() and doesn't directly synchronize with fsverity_remove_info().

For the same inode this isn't a problem: its fsverity_info is removed only at inode eviction time or upon failure to enable verity, when the inode no longer needs its fsverity_info and it will no longer be accessed via that inode.

However, this is broken and can cause a use-after-free for concurrent __fsverity_get_info() for *different* inodes. Those rely on following fsverity_info::rhash_head in the rhashtable under rcu_read_lock() only. They also use fsverity_info::inode to do the key comparison.

Fix this by RCU-delaying the freeing of 'struct fsverity_info' after it's been removed from the rhashtable.

Quoted 7 lines
Fixes f77f281b6118 ("fsverity: use a hashtable to find the fsverity_info")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 fs/verity/fsverity_private.h | 15 ++++++++++++++-
 fs/verity/open.c             | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)
Reviewed-by Sandeep Dhavale <[email protected]>

Thanks,
Sandeep.

author maintainer Applied
#2 lore raw
In reply to Eric Biggers
Quoted 17 lines · Eric Biggers

__fsverity_get_info() calls rhashtable_lookup_fast(), which just uses rcu_read_lock() and doesn't directly synchronize with fsverity_remove_info().

For the same inode this isn't a problem: its fsverity_info is removed only at inode eviction time or upon failure to enable verity, when the inode no longer needs its fsverity_info and it will no longer be accessed via that inode.

However, this is broken and can cause a use-after-free for concurrent __fsverity_get_info() for *different* inodes. Those rely on following fsverity_info::rhash_head in the rhashtable under rcu_read_lock() only. They also use fsverity_info::inode to do the key comparison.

Fix this by RCU-delaying the freeing of 'struct fsverity_info' after it's been removed from the rhashtable.

Quoted 7 lines
Fixes f77f281b6118 ("fsverity: use a hashtable to find the fsverity_info")
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 fs/verity/fsverity_private.h | 15 ++++++++++++++-
 fs/verity/open.c             | 13 ++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

Applied to https://git.kernel.org/pub/scm/fs/fsverity/linux.git/log/?h=for-current

- Eric