xfs: detect corrupt inobt records better

 fs/xfs/xfs_icache.c | 73 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 25 deletions(-)

fs/xfs/xfs_icache.c +0 -0

index 98b7a4ae15e4..fb37ada55710 100644

Quoted 8 lines

 fs/xfs/xfs_icache.c | 73 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 25 deletions(-)

fs/xfs/xfs_icache.c +0 -0

index 98b7a4ae15e4..fb37ada55710 100644

fs/xfs/xfs_icache.c xfs_reinit_inode( · line 315

Quoted 7 lines

fs/xfs/xfs_icache.c +4 -0

@@ -309,6 +309,46 @@ xfs_reinit_inode(
 	return error;
 }
 
+/*
+ * If we are allocating a new inode, then check what was returned is
+ * actually a free, empty inode. If we are not allocating an inode,
+ * the check we didn't find a free inode.

I know it's been like that for a while, but maybe it's a good time to fix this typo? "then check we didn't find..." ?

Fixing the old typo or not, the patch looks good:

Reviewed-by Carlos Maiolino <[email protected]>

Quoted 19 lines · Carlos Maiolino

Quoted 8 lines

 fs/xfs/xfs_icache.c | 73 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 25 deletions(-)

fs/xfs/xfs_icache.c +0 -0

index 98b7a4ae15e4..fb37ada55710 100644

fs/xfs/xfs_icache.c xfs_reinit_inode( · line 315

Quoted 7 lines

fs/xfs/xfs_icache.c +4 -0

@@ -309,6 +309,46 @@ xfs_reinit_inode(
 	return error;
 }
 
+/*
+ * If we are allocating a new inode, then check what was returned is
+ * actually a free, empty inode. If we are not allocating an inode,
+ * the check we didn't find a free inode.

I know it's been like that for a while, but maybe it's a good time to fix this typo? "then check we didn't find..." ?

Quoted 3 lines

Fixing the old typo or not, the patch looks good:

Reviewed-by Carlos Maiolino <[email protected]>

Darrick, do want me to resend with the typo fix, or can you do it on the way into your tree?

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

A recent fuzzed filesystem image cached random dcache corruption when the reproducer was run. This often showed up as panics in lookup_slow() on a null inode->i_ops pointer when doing pathwalks.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
....
Call Trace:
 lookup_slow+0x44/0x60
 walk_component+0x3dd/0x9f0
 link_path_walk+0x4a7/0x830
 path_lookupat+0xc1/0x470
 filename_lookup+0x129/0x270
 user_path_at_empty+0x36/0x40
 path_listxattr+0x98/0x110
 SyS_listxattr+0x13/0x20
 do_syscall_64+0xf5/0x280
 entry_SYSCALL_64_after_hwframe+0x42/0xb7

but had many different failure modes including deadlocks trying to lock the inode that was just allocated or KASAN reports of use-after-free violations.

The cause of the problem was a corrupt INOBT on a v4 fs where the root inode was marked as free in the inobt record. Hence when we allocated an inode, it chose the root inode to allocate, found it in the cache and re-initialised it.

We recently fixed a similar inode allocation issue caused by inobt record corruption problem in xfs_iget_cache_miss() in commit ee457001ed6c ("xfs: catch inode allocation state mismatch corruption"). This change adds similar checks to the cache-hit path to catch it, and turns the reproducer into a corruption shutdown situation.

Reported-by Wen Xu <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_icache.c | 73 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 25 deletions(-)

fs/xfs/xfs_icache.c +48 -25

index 98b7a4ae15e4..fb37ada55710 100644
@@ -309,6 +309,46 @@ xfs_reinit_inode(
 	return error;
 }
 
+/*
+ * If we are allocating a new inode, then check what was returned is
+ * actually a free, empty inode. If we are not allocating an inode,
+ * the check we didn't find a free inode.
+ *
+ * Returns:
+ *	0		if the inode free state matches the lookup context
+ *	-ENOENT		if the inode is free and we are not allocating
+ *	-EFSCORRUPTED	if there is any state mismatch at all
+ */
+static int
+xfs_iget_check_free_state(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	if (flags & XFS_IGET_CREATE) {
+		/* should be a free inode */
+		if (VFS_I(ip)->i_mode != 0) {
+			xfs_warn(ip->i_mount,
+"Corruption detected! Free inode 0x%llx not marked free! (mode 0x%x)",
+				ip->i_ino, VFS_I(ip)->i_mode);
+			return -EFSCORRUPTED;
+		}
+
+		if (ip->i_d.di_nblocks != 0) {
+			xfs_warn(ip->i_mount,
+"Corruption detected! Free inode 0x%llx has blocks allocated!",
+				ip->i_ino);
+			return -EFSCORRUPTED;
+		}
+		return 0;
+	}
+
+	/* should be an allocated inode */
+	if (VFS_I(ip)->i_mode == 0)
+		return -ENOENT;
+
+	return 0;
+}
+
 /*
  * Check the validity of the inode we just found it the cache
  */

@@ -358,12 +398,12 @@ xfs_iget_cache_hit(
 	}
 
 	/*
-	 * If lookup is racing with unlink return an error immediately.
+	 * Check the inode free state is valid. This also detects lookup
+	 * racing with unlinks.
 	 */
-	if (VFS_I(ip)->i_mode == 0 && !(flags & XFS_IGET_CREATE)) {
-		error = -ENOENT;
+	error = xfs_iget_check_free_state(ip, flags);
+	if (error)
 		goto out_error;
-	}
 
 	/*
 	 * If IRECLAIMABLE is set, we've torn down the VFS inode already.

@@ -486,29 +526,12 @@ xfs_iget_cache_miss(
 
 
 	/*
-	 * If we are allocating a new inode, then check what was returned is
-	 * actually a free, empty inode. If we are not allocating an inode,
-	 * the check we didn't find a free inode.
+	 * Check the inode free state is valid. This also detects lookup
+	 * racing with unlinks.
 	 */
-	if (flags & XFS_IGET_CREATE) {
-		if (VFS_I(ip)->i_mode != 0) {
-			xfs_warn(mp,
-"Corruption detected! Free inode 0x%llx not marked free on disk",
-				ino);
-			error = -EFSCORRUPTED;
-			goto out_destroy;
-		}
-		if (ip->i_d.di_nblocks != 0) {
-			xfs_warn(mp,
-"Corruption detected! Free inode 0x%llx has blocks allocated!",
-				ino);
-			error = -EFSCORRUPTED;
-			goto out_destroy;
-		}
-	} else if (VFS_I(ip)->i_mode == 0) {
-		error = -ENOENT;
+	error = xfs_iget_check_free_state(ip, flags);
+	if (error)
 		goto out_destroy;
-	}
 
 	/*
 	 * Preload the radix tree so we can insert safely under the

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

When we have corrupted free inode btrees, we can attempt to allocate inodes that we know are already allocated. Catch allocation of these inodes and report corruption as early as possible to prevent corruption propagation or deadlocks.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/xfs/xfs_inode.c +0 -0

index 7b764f746ff2..12e6c1aec386 100644
@@ -790,6 +790,19 @@ xfs_ialloc(
 	}
 	ASSERT(*ialloc_context == NULL);

From: Dave Chinner <[email protected]>

When we have corrupted free inode btrees, we can attempt to allocate inodes that we know are already allocated. Catch allocation of these inodes and report corruption as early as possible to prevent corruption propagation or deadlocks.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/xfs_inode.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/xfs/xfs_inode.c +13 -0

index 7b764f746ff2..12e6c1aec386 100644
@@ -790,6 +790,19 @@ xfs_ialloc(
 	}
 	ASSERT(*ialloc_context == NULL);
 
+	/*
+	 * Protect against obviously corrupt allocation btree records. Later
+	 * xfs_iget checks will catch re-allocation of other active in-memory
+	 * and on-disk inodes. If we don't catch reallocating the parent inode
+	 * here we will deadlock in xfs_iget() so we have to do these checks
+	 * first.
+	 */
+	if ((pip && ino == pip->i_ino) ||
+	    !xfs_verify_dir_ino(mp, ino)) {
+		xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
+		return -EFSCORRUPTED;
+	}
+
 	/*
 	 * Get the in-core inode with the lock held exclusively.
 	 * This is because we're setting fields here we need

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

When we have corrupted free inode btrees, we can attempt to allocate inodes that we know are already allocated. Catch allocation of these inodes and report corruption as early as possible to prevent corruption propagation or deadlocks.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/xfs/xfs_inode.c +13 -0

index 7b764f746ff2..12e6c1aec386 100644
@@ -790,6 +790,19 @@ xfs_ialloc(
 	}
 	ASSERT(*ialloc_context == NULL);
 
+	/*
+	 * Protect against obviously corrupt allocation btree records. Later
+	 * xfs_iget checks will catch re-allocation of other active in-memory
+	 * and on-disk inodes. If we don't catch reallocating the parent inode
+	 * here we will deadlock in xfs_iget() so we have to do these checks
+	 * first.
+	 */
+	if ((pip && ino == pip->i_ino) ||
+	    !xfs_verify_dir_ino(mp, ino)) {
+		xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
+		return -EFSCORRUPTED;
+	}
+
 	/*
 	 * Get the in-core inode with the lock held exclusively.
 	 * This is because we're setting fields here we need

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

However, v5 is still not safe against is malicious corruption, where a bad actor intentionally modifies the on-disk structures to mark inodes free in both the inobt and the finobt and then recalculates the CRCs and other metadata. We could check the rmapbt if it's configured, but an attacker can also manipulate that structure to say that region does contain inodes. They can also manipulate the free space btrees to say it's used space and so once we've chased everything we can cross-check down we still can't reliably detect malicious corruption attacks on the v5 filesystem structure.

IOWs, even with all the extra on-disk verification the v5 format has, the only thing we can do to protect against propagation of malicious corruption is the same thing as for v4: check the inode is free on disk before allowing the allocation to proceed.

I have not done this, because I think malicious corruption is not something we can defend against. Hence it makes no sense to add checks that reduce performance but don't provide any extra benefit to device-based corruption detection or propagation prevention. IOWs, I don't think we should try to mitigate malicious corruption attack scenarios.

I think we need to keep improving our bounds checking and structure corruption detection, but we should not worry about things that take multiple, highly improbably structure corruptions that are hihgly expensive to detect and/or mitigate. i.e. unprivileged mounts of untrusted XFS filesystem images should never, ever be allowed.

Quoted 22 lines

However, v5 is still not safe against is malicious corruption, where a bad actor intentionally modifies the on-disk structures to mark inodes free in both the inobt and the finobt and then recalculates the CRCs and other metadata. We could check the rmapbt if it's configured, but an attacker can also manipulate that structure to say that region does contain inodes. They can also manipulate the free space btrees to say it's used space and so once we've chased everything we can cross-check down we still can't reliably detect malicious corruption attacks on the v5 filesystem structure.

IOWs, even with all the extra on-disk verification the v5 format has, the only thing we can do to protect against propagation of malicious corruption is the same thing as for v4: check the inode is free on disk before allowing the allocation to proceed.

I have not done this, because I think malicious corruption is not something we can defend against. Hence it makes no sense to add checks that reduce performance but don't provide any extra benefit to device-based corruption detection or propagation prevention. IOWs, I don't think we should try to mitigate malicious corruption attack scenarios.

Quoted 6 lines

I think we need to keep improving our bounds checking and structure corruption detection, but we should not worry about things that take multiple, highly improbably structure corruptions that are hihgly expensive to detect and/or mitigate. i.e. unprivileged mounts of untrusted XFS filesystem images should never, ever be allowed.

I agree with it, recently, I had a discussion with Eric about something similar, and well, this sort of attack requires some access to the filesystem image itself, or the device where it lays on. And well, IMHO, in a system, if somebody is ever allowed to do such modifications in the filesystem, the security problem is way above the filesystem itself, either by allowing unprivileged access to the FS image, or superuser being compromised.

Either way, I do not believe we will ever be able to 100% protect the superuser against himself, or bad administrative policies even so if such protection would be a performance trade-off.