[PATCH] freevxfs: don't BUG() on unknown typed-extent type

Farhad Alemi posted 1 patch 1 week, 2 days ago
There is a newer version of this series
fs/freevxfs/vxfs_bmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] freevxfs: don't BUG() on unknown typed-extent type
Posted by Farhad Alemi 1 week, 2 days ago
vxfs_bmap_typed() dispatches on the on-disk typed-extent type,
(u32)(hdr >> VXFS_TYPED_TYPESHIFT), where hdr comes from the
attacker-controlled vt_hdr of each typed extent. Only four type values
are handled; any other value falls through the switch to the default
case, which is BUG(). After mounting a crafted VxFS image, an
ioctl(FIBMAP) on a regular file reaches this path and crashes the
kernel:

  kernel BUG at fs/freevxfs/vxfs_bmap.c:230!
  RIP: vxfs_bmap_typed fs/freevxfs/vxfs_bmap.c:230 [inline]
       vxfs_bmap1+0x128a/0x12d0 fs/freevxfs/vxfs_bmap.c:257
  Call Trace:
   vxfs_getblk fs/freevxfs/vxfs_subr.c:104
   generic_block_bmap fs/buffer.c:2764
   bmap fs/inode.c:1948
   ioctl_fibmap fs/ioctl.c:77 [inline]
   file_ioctl+0x4b1/0x870 fs/ioctl.c:327

An unrecognized extent type is malformed on-disk input rather than a
kernel invariant violation. Replacing the BUG() with WARN_ON_ONCE()
would log the unexpected type once, and return 0 -- the failure value
vxfs_bmap_typed() already documents ("the physical block number on
success, else Zero") and the value its neighbouring DEV4 case returns.
vxfs_getblk() maps a 0 result to -EIO, so the FIBMAP ioctl fails
cleanly instead of crashing.

Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Signed-off-by: Farhad Alemi <farhad.alemi@berkeley.edu>
---
 fs/freevxfs/vxfs_bmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/freevxfs/vxfs_bmap.c b/fs/freevxfs/vxfs_bmap.c
index e85222892038..1b8216eb1d90 100644
--- a/fs/freevxfs/vxfs_bmap.c
+++ b/fs/freevxfs/vxfs_bmap.c
@@ -227,6 +227,7 @@ vxfs_bmap_typed(struct inode *ip, long iblock)
 			return 0;
 		}
 		default:
-			BUG();
+			WARN_ON_ONCE(1);
+			return 0;
 		}
 	}
-- 
2.43.0
Re: [PATCH] freevxfs: don't BUG() on unknown typed-extent type
Posted by Christoph Hellwig 1 week ago
Not a fan of the attacker wording here - malіcious devices are not part
of the threat model of a read-only compatibility for a historic file
system used for hobby purposes.  That being said, robustness is a good
thing, and the patch itself looks good.

Can we tone down the language a bit?
[PATCH v2] freevxfs: don't BUG() on unknown typed-extent type
Posted by Farhad Alemi 6 days, 5 hours ago
vxfs_bmap_typed() handles four typed-extent types and calls BUG() in
its default case, so an on-disk typed extent with any other type value
crashes the kernel. It is reachable from ioctl(FIBMAP) on a regular
file:

  kernel BUG at fs/freevxfs/vxfs_bmap.c:230!
  RIP: vxfs_bmap_typed fs/freevxfs/vxfs_bmap.c:230 [inline]
       vxfs_bmap1+0x128a/0x12d0 fs/freevxfs/vxfs_bmap.c:257

Replace the BUG() with WARN_ON_ONCE() and return 0 -- the value
vxfs_bmap_typed() already returns on failure (and from the DEV4 case
above); vxfs_getblk() maps 0 to -EIO, so the ioctl fails cleanly.

Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Signed-off-by: Farhad Alemi <farhad.alemi@berkeley.edu>
---
v2: tone down the changelog wording (Christoph Hellwig); code unchanged.

 fs/freevxfs/vxfs_bmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/freevxfs/vxfs_bmap.c b/fs/freevxfs/vxfs_bmap.c
index e85222892038..1b8216eb1d90 100644
--- a/fs/freevxfs/vxfs_bmap.c
+++ b/fs/freevxfs/vxfs_bmap.c
@@ -227,7 +227,8 @@ vxfs_bmap_typed(struct inode *ip, long iblock)
 			return 0;
 		}
 		default:
-			BUG();
+			WARN_ON_ONCE(1);
+			return 0;
 		}
 	}

-- 
2.43.0
Re: [PATCH v2] freevxfs: don't BUG() on unknown typed-extent type
Posted by Christoph Hellwig 6 days, 2 hours ago
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Christian, can you pick this up through the vfs tree?