[PATCH] omfs: fix inverted check and use-after-brelse in omfs_dir_is_empty()

[PATCH] omfs: fix inverted check and use-after-brelse in omfs_dir_is_empty()

[Hui Peng]

omfs_dir_is_empty() scans the directory's hash bucket table for an
occupied slot (~0 marks an empty bucket) and then returns:

        for (i = 0; i < nbuckets; i++, ptr++)
                if (*ptr != ~0)
                        break;

        brelse(bh);
        return *ptr != ~0;

That return statement has three bugs at once:

  1. The sense is backwards. Its only caller is omfs_remove():

         if (S_ISDIR(inode->i_mode) &&
             !omfs_dir_is_empty(inode))
                 return -ENOTEMPTY;

     When the directory is NOT empty the loop breaks on the first
     occupied bucket, so *ptr != ~0 is true, omfs_dir_is_empty() returns
     1 ("empty"), -ENOTEMPTY is skipped, and rmdir unlinks the directory
     while leaving its children allocated and unreachable.

  2. When the directory IS empty the loop runs to i == nbuckets, leaving
     ptr one u64 past the end of the bucket array - which for a standard
     block size is one u64 past the end of bh->b_data itself. Both the
     out-of-bounds read and the emptiness verdict then depend on whatever
     sits in memory after the buffer.

  3. Either way, ptr points into bh->b_data and is dereferenced after
     brelse(bh) has dropped the buffer reference.

With KASAN enabled, rmdir of an empty directory reports:

 ==================================================================
 BUG: KASAN: use-after-free in omfs_remove+0x265/0x270
 Read of size 8 at addr ffff88811a145000 by task init/1

 CPU: 1 UID: 0 PID: 1 Comm: init Tainted: G    B D  7.3.0-rc3 #1
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  omfs_remove+0x265/0x270
  vfs_rmdir+0x2e6/0x810
  filename_rmdir+0x3bf/0x530
  __x64_sys_rmdir+0x4b/0x70
  do_syscall_64+0xda/0x4b0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>
 ==================================================================

All the information needed is already in the loop counter: the directory
is empty iff the scan reached nbuckets without breaking. Return
`i == nbuckets`, which neither touches the buffer after brelse() nor
reads past the end of the array, and gives omfs_remove() the polarity it
expects.

Fixes: a3ab7155ea21 ("omfs: add directory routines")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Note that nbuckets itself is bounded: omfs_iget() sets inode->i_size to
sbi->s_sys_blocksize for OMFS_DIR inodes (not from the on-disk i_size),
and omfs_fill_super() rejects s_sys_blocksize < OMFS_DIR_START, so the
loop always stays within the single block read by omfs_bread().

Because bug (1) causes rmdir on a normal, non-corrupted filesystem to
delete a non-empty directory rather than return -ENOTEMPTY, this may be
worth a stable backport even though I originally hit it via a KASAN image
test. Left Cc: stable off for you to judge.

diff --git a/fs/omfs/dir.c b/fs/omfs/dir.c
--- a/fs/omfs/dir.c
+++ b/fs/omfs/dir.c
@@ -232,7 +232,7 @@
 			break;
 
 	brelse(bh);
-	return *ptr != ~0;
+	return i == nbuckets;
 }
 
 static int omfs_remove(struct inode *dir, struct dentry *dentry)
-- 
2.43.0