Forwarded: [PATCH] ext4: validate dirents before splitting a directory

[syzbot <syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: [PATCH] ext4: validate dirents before splitting a directory
Author: adrianox@gmail.com

#syz test: upstream master
syzbot reported a slab-use-after-free write in do_split() while
renaming an entry in a directory that is being converted into an
indexed (htree) directory by make_indexed_dir():

  BUG: KASAN: slab-use-after-free in dx_move_dirents [inline]
  BUG: KASAN: slab-use-after-free in do_split+0x1241/0x1e90
  Write of size 90458 at addr ffff88803b38ac6e by task syz.0.17/6003

do_split() builds a map of the leaf's dirents and then moves a subset
of them to a new block.  dx_move_dirents() trusts map[i].offs and uses
the rec_len found there as the length of a memset().  With a corrupted
or crafted directory block, a bogus map entry makes that rec_len
garbage (here 90464), turning the memset() into an out-of-bounds write
that can corrupt arbitrary memory.

Validate each entry that is about to be moved with
ext4_check_dir_entry() before using it, and bail out with
-EFSCORRUPTED if the entry does not lie inside the block.  This is the
same class of validation already used elsewhere in the directory code.

This is a filesystem-corruption hardening issue; the crash needs a
corrupt directory, which is why it is not reachable on a consistent
filesystem.  The syzbot "introduced by" bisection pointed at an
unrelated btrfs commit and can be ignored.

Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
---
 fs/ext4/namei.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index a6386c1d237f..b2ec222b15e4 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1951,6 +1951,25 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 		goto journal_error;
 	}
 	map -= count;
+	/*
+	 * The map is built from the on-disk dirents, so its entries should
+	 * always refer to valid dirents.  However, if the leaf block is
+	 * corrupted (e.g. a crafted image), a bogus map entry can make
+	 * dx_move_dirents() read a rec_len from an arbitrary location and use
+	 * it as the length of a memset(), writing far out of bounds.  Validate
+	 * every entry we are about to move before using it.
+	 */
+	for (i = 0; i < count; i++) {
+		unsigned int off = map[i].offs << 2;
+
+		if (off > blocksize - sizeof(struct ext4_dir_entry_2) ||
+		    ext4_check_dir_entry(dir, NULL,
+					 (struct ext4_dir_entry_2 *)(data1 + off),
+					 *bh, data1, blocksize, off)) {
+			err = -EFSCORRUPTED;
+			goto out;
+		}
+	}
 	dx_sort_map(map, count);
 	/* Ensure that neither split block is over half full */
 	size = 0;
-- 
2.51.0