[2/4] xfs_repair: don't crash in get_inode_parent

Commit Message

Darrick J. Wong Oct. 2, 2024, 1:26 a.m. UTC

From: Darrick J. Wong <[e-mail]>

The xfs_repair fuzz test suite encountered a crash in xfs_repair.  In
the fuzzed filesystem, inode 8388736 is a single-block directory where
the one dir data block has been trashed.  This inode maps to agno 1
agino 128, and all other inodes in that inode chunk are regular files.
Output is as follows:

Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
        - scan filesystem freespace and inode maps...
        - found root inode chunk
Phase 3 - for each AG...
        - scan (but don't clear) agi unlinked lists...
        - process known inodes and perform inode discovery...
        - agno = 0
        - agno = 1
Metadata corruption detected at 0x565335fbd534, xfs_dir3_block block 0x4ebc78/0x1000
corrupt directory block 0 for inode 8388736
no . entry for directory 8388736
no .. entry for directory 8388736
problem with directory contents in inode 8388736
would have cleared inode 8388736
        - agno = 2
        - agno = 3
        - process newly discovered inodes...
Phase 4 - check for duplicate blocks...
        - setting up duplicate extent list...
        - check for inodes claiming duplicate blocks...
        - agno = 0
entry "S_IFDIR.FMT_BLOCK" at block 0 offset 1728 in directory inode 128 references free inode 8388736
        would clear inode number in entry at offset 1728...
        - agno = 1
entry "." at block 0 offset 64 in directory inode 8388736 references free inode 8388736
imap claims in-use inode 8388736 is free, would correct imap
        - agno = 2
        - agno = 3
No modify flag set, skipping phase 5
Phase 6 - check inode connectivity...
        - traversing filesystem ...
./common/xfs: line 387: 84940 Segmentation fault      (core dumped) $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV

From the coredump, we crashed in get_inode_parent here because ptbl is a
NULL pointer:

	if (ptbl->pmask & (1ULL << offset))  {

Directory inode 8388736 doesn't have a dotdot entry and phase 3 decides
to clear that inode, so it never calls set_inode_parent for 8388736.
Because the rest of the inodes in the chunk are regular files, phase 3
never calls set_inode_parent on the corresponding irec.  As a result,
neither irec->ino_un.plist nor irec->ino_un.ex_data->parents are ever
set to a parents array.

When phase 6 calls get_inode_parent to check the S_IFDIR.FMT_BLOCK
dirent from the root directory to inode 8388736, it sets ptbl to
irec->ino_un.ex_data->parents (which is still NULL) and walks off the
NULL pointer.

Because get_inode_parent already has the behavior that it can return
zero for "unknown parent", the correction is simple: check ptbl before
dereferencing it.  git blame says this code has been in xfsprogs since
the beginning of git, so I won't bother with a fixes tag.

Found by fuzzing bhdr.hdr.bno = zeroes in xfs/386.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 repair/incore_ino.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Oct. 2, 2024, 5:58 a.m. UTC | #1

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Patch

diff --git a/repair/incore_ino.c b/repair/incore_ino.c
index 6618e534a..158e9b498 100644
--- a/repair/incore_ino.c
+++ b/repair/incore_ino.c
@@ -714,7 +714,7 @@  get_inode_parent(ino_tree_node_t *irec, int offset)
 	else
 		ptbl = irec->ino_un.plist;
 
-	if (ptbl->pmask & (1ULL << offset))  {
+	if (ptbl && (ptbl->pmask & (1ULL << offset)))  {
 		bitmask = 1ULL;
 		target = 0;