[PATCH v2 0/6] fs: ext4: check on-disk indices before using them

[PATCH v2 0/6] fs: ext4: check on-disk indices before using them

[Ana Bobillo]

The ext4 driver takes several indices from the filesystem image and uses
them without checking them against the structures they index. A corrupt
or crafted image can then make ext4load read past an extent tree node, or
make ext4write (and saveenv with the environment on ext4) write outside
the block bitmaps, the inode bitmaps and the group descriptor table when
it replaces an existing file.

This series checks each of those indices where it is read:

1. extent tree nodes: entry count against the node's capacity, and the
   depth, as Linux does
2. the triple indirect lookup compares the cached block with the wrong
   index, read past the end of its buffer
3. the write path refuses block group layouts its bitmap code cannot
   handle, including zero blocks per group, which divides by zero
4. block numbers released by the delete path must lie inside the
   filesystem, and a failed read of an indirect block stops the delete
5. the block group of an inode is (n - 1) / inodes_per_group in the write
   path too, and inode numbers from directory entries are checked as
   Linux checks them
6. tests in test/py for each of the above

With sandbox_defconfig and CONFIG_ASAN, the new tests pass with the
series applied. On master the corrupt extent header is not rejected, the
looping extent tree hangs ext4load, ext4fs_init() divides by zero, ASan
reports segfaults in ext4fs_reset_block_bmap() and in
ext4fs_reset_inode_bmap(), the journal's inode is deleted, and replacing
the file in the last inode of a group fails with "no inode left to
assign". The triple indirect test passes on master too: there the wrong
compare reads past the buffer but seldom changes which block is read, so
the test checks that blocks reached through the later entries of the
triple indirect block still read back correctly. The existing ext4 and
env tests still pass, and repeatedly overwriting files on clean images,
with and without extents, with 1 KiB and 4 KiB blocks, keeps the images
clean for e2fsck.

Tom asked for the results of a CI run. This series, exactly as posted
here, went through U-Boot's Azure pipeline on a pull request opened only
for CI, together with three other series of mine that touch other files
(block device block sizes, USB Ethernet rx headers and EFI capsule
sizes), on top of v2027.01-rc1. Every job passed:
https://github.com/u-boot/u-boot/pull/1064
https://dev.azure.com/u-boot/u-boot/_build/results?buildId=13698

Simon suggested that ext4fs_get_new_blk_no() could use
ext4fs_blk_to_group() too. I would rather do that as a follow-up: there,
running past the last group means that the filesystem is full, not that
a block number is corrupt, so it needs a quiet check rather than the
helper's error message.

Changes in v2:
- Add Simon's Reviewed-by tags to patches 1 to 3
- Use printf() for every error message, as the rest of the driver does
- Patch 4: compute the block group from the first data block, as Linux
  does; check the reads of the indirect blocks; return -EIO for read and
  journal errors
- Patch 5: add ext4fs_inode_to_group() and use it for every inode bitmap
  index; reject reserved inodes other than the root
- Patch 6: clean up after a failing test; use f-strings; add tests for
  patch 1's depth check, patch 2, the last inode of a group and the
  journal's inode
- Rebase on main

Link to v1: https://patchwork.ozlabs.org/project/uboot/list/?series=529698

Ana Bobillo (6):
  fs: ext4: check extent tree nodes before walking them
  fs: ext4: fix the triple indirect cache check in
    read_allocated_block()
  fs: ext4: refuse to write with an unsupported block group layout
  fs: ext4: check block numbers before releasing them
  fs: ext4: fix the block group of an inode in the write path
  test: fs: ext4: add tests for corrupt on-disk indices

 fs/ext4/ext4_common.c                 | 132 ++++++++++-
 fs/ext4/ext4_common.h                 |   2 +
 fs/ext4/ext4_write.c                  | 151 ++++++++-----
 include/ext4fs.h                      |   1 +
 scripts/pylint.base                   |   1 +
 test/py/tests/test_fs/test_ext_oob.py | 310 ++++++++++++++++++++++++++
 6 files changed, 530 insertions(+), 67 deletions(-)
 create mode 100644 test/py/tests/test_fs/test_ext_oob.py

[PATCH v2 1/6] fs: ext4: check extent tree nodes before walking them

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

ext4fs_get_extent_block() walks the extent tree of an inode using the
eh_entries, eh_max and eh_depth fields of each node as found on disk,
checking nothing but the magic number. read_allocated_block() then
scans eh_entries extents of the leaf it returns.

A node can claim more entries than it can hold: up to 4 in the
60-byte i_block area of the inode, (blocksize - 12) / 12 in a tree
block. Both loops then read past the end of the node, into the rest
of the inode or past the cache buffer. An index node can also point
back at itself or at any node of the same or a greater depth, so the
walk never ends.

Check each node as Linux __ext4_ext_check() does: eh_max is non-zero
and fits in the node, eh_entries does not exceed eh_max, the depth
drops by exactly one at each level and an index node has at least one
entry; and, as ext4_find_extent() requires, the depth is at most 5.
A node that fails is reported as an invalid extent block, the error
read_allocated_block() already returns for a bad magic number.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)

 fs/ext4/ext4_common.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 include/ext4fs.h      |  1 +
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 9e1d574e5b1..3aab1c87233 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -1529,6 +1529,38 @@ void ext4fs_allocate_blocks(struct ext2_inode *file_inode,
 
 #endif
 
+/**
+ * ext4fs_check_extent_header() - check an extent tree node before using it
+ *
+ * Some of the checks of Linux __ext4_ext_check() and ext4_find_extent(): the
+ * node must not claim more entries than fit in it, and the depth must be at
+ * most 5 and drop by one at each level, so that a corrupt node can neither be
+ * walked past its end nor make the tree loop.
+ *
+ * @eh:		extent header of the node
+ * @max:	number of entries that fit in the node
+ * @depth:	depth this node must have, or -1 for the root in the inode
+ * Return:	0 if the node is valid, -EINVAL otherwise
+ */
+static int ext4fs_check_extent_header(struct ext4_extent_header *eh, int max,
+				      int depth)
+{
+	if (le16_to_cpu(eh->eh_magic) != EXT4_EXT_MAGIC)
+		return -EINVAL;
+	if (!eh->eh_max || le16_to_cpu(eh->eh_max) > max)
+		return -EINVAL;
+	if (le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))
+		return -EINVAL;
+	if (le16_to_cpu(eh->eh_depth) > EXT4_MAX_EXTENT_DEPTH)
+		return -EINVAL;
+	if (depth >= 0 && le16_to_cpu(eh->eh_depth) != depth)
+		return -EINVAL;
+	if (eh->eh_depth && !eh->eh_entries)
+		return -EINVAL;
+
+	return 0;
+}
+
 static struct ext4_extent_header *ext4fs_get_extent_block
 	(struct ext2_data *data, struct ext_block_cache *cache,
 		struct ext4_extent_header *ext_block,
@@ -1537,16 +1569,24 @@ static struct ext4_extent_header *ext4fs_get_extent_block
 	struct ext4_extent_idx *index;
 	unsigned long long block;
 	int blksz = EXT2_BLOCK_SIZE(data);
+	/* The root node is held in the inode's 60-byte i_block area */
+	int max = (sizeof(((struct ext2_inode *)0)->b) -
+		   sizeof(struct ext4_extent_header)) /
+		  sizeof(struct ext4_extent);
+	int depth = -1;
 	int i;
 
 	while (1) {
 		index = (struct ext4_extent_idx *)(ext_block + 1);
 
-		if (le16_to_cpu(ext_block->eh_magic) != EXT4_EXT_MAGIC)
+		if (ext4fs_check_extent_header(ext_block, max, depth))
 			return NULL;
 
 		if (ext_block->eh_depth == 0)
 			return ext_block;
+		depth = le16_to_cpu(ext_block->eh_depth) - 1;
+		max = (blksz - sizeof(struct ext4_extent_header)) /
+		      sizeof(struct ext4_extent);
 		i = -1;
 		do {
 			i++;
diff --git a/include/ext4fs.h b/include/ext4fs.h
index fe3fb301ec8..19eda150cb6 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -35,6 +35,7 @@ struct disk_partition;
 #define EXT4_TOPDIR_FL		0x00020000 /* Top of directory hierarchies*/
 #define EXT4_EXTENTS_FL		0x00080000 /* Inode uses extents */
 #define EXT4_EXT_MAGIC			0xf30a
+#define EXT4_MAX_EXTENT_DEPTH		5
 
 #define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER  0x0001
 #define EXT4_FEATURE_RO_COMPAT_LARGE_FILE    0x0002

[PATCH v2 2/6] fs: ext4: fix the triple indirect cache check in read_allocated_block()

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

In the triple indirect branch, read_allocated_block() reads the third
level block from ext4fs_indir2_block[(rblock / perblock_child) %
(blksz / 4)] and records it in ext4fs_indir3_blkno, but it decides
whether that cached block can be reused by comparing against
ext4fs_indir2_block[rblock / perblock_child], without the modulo.

For a file block reached through the second or a later entry of the
triple indirect block (rblock >= (blksz / 4) * (blksz / 4)),
rblock / perblock_child is at least blksz / 4, so the compare reads
past the end of the blksz-byte ext4fs_indir2_block buffer and can
make the lookup reuse the wrong cached block.

Use the same index for the compare as for the read.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)

 fs/ext4/ext4_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 3aab1c87233..cff112fb99b 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -2008,9 +2008,9 @@ long int read_allocated_block(struct ext2_inode *inode, int fileblock,
 			}
 			ext4fs_indir3_size = blksz;
 		}
-		if ((le32_to_cpu(ext4fs_indir2_block[rblock
-						       /
-						       perblock_child]) <<
+		if ((le32_to_cpu(ext4fs_indir2_block[(rblock /
+						      perblock_child) %
+						     (blksz / 4)]) <<
 		     log2_blksz) != ext4fs_indir3_blkno) {
 			status =
 			    ext4fs_devread((lbaint_t)le32_to_cpu

[PATCH v2 3/6] fs: ext4: refuse to write with an unsupported block group layout

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

ext4fs_init() divides by s_blocks_per_group from the superblock to
count the block groups, so a value of zero crashes the write path at
once. The bitmap code of the write path also assumes one bitmap
block's worth of blocks per group: ext4fs_set_block_bmap() and
ext4fs_reset_block_bmap() find the bits of group n at n * blocksize
bytes into its bitmap, and ext4fs_get_new_blk_no() numbers the blocks
of group n from n * blocksize * 8. With any other s_blocks_per_group
the groups after the first are indexed outside their bitmap buffers.
s_inodes_per_group likewise cannot exceed the bits of one bitmap block.

mke2fs uses that layout unless it is told otherwise; it lowers
s_blocks_per_group for 64 KiB blocks, with -g, or when many inodes are
asked for. Check the layout at the start of ext4fs_init(), before the
journal is replayed, and refuse to write to a filesystem of more than
one group laid out any other way, instead of dividing by zero or
indexing outside the bitmap buffers. A filesystem of a single group
is still written to, as before.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Use printf() rather than log_err(), as the rest of the driver does

 fs/ext4/ext4_write.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 2fba2197c19..5f185387030 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -605,11 +605,30 @@ int ext4fs_init(void)
 	int i;
 	uint32_t real_free_blocks = 0;
 	struct ext_filesystem *fs = get_fs();
+	struct ext2_sblock *sblock = &ext4fs_root->sblock;
+	uint32_t blk_per_grp = le32_to_cpu(sblock->blocks_per_group);
+	uint32_t inode_per_grp = le32_to_cpu(sblock->inodes_per_group);
 
 	/* populate fs */
 	fs->blksz = EXT2_BLOCK_SIZE(ext4fs_root);
 	fs->sect_perblk = fs->blksz >> fs->dev_desc->log2blksz;
 
+	/*
+	 * The bitmap code finds the bitmap bits of group n at n * blocksize
+	 * bytes, so every group must hold one bitmap block's worth of
+	 * blocks, unless there is only one group. Check this before the
+	 * journal is replayed, so that nothing is written to a filesystem
+	 * that is refused.
+	 */
+	if (!blk_per_grp || blk_per_grp > fs->blksz * 8 ||
+	    (blk_per_grp != fs->blksz * 8 &&
+	     le32_to_cpu(sblock->total_blocks) -
+	     le32_to_cpu(sblock->first_data_block) > blk_per_grp) ||
+	    !inode_per_grp || inode_per_grp > fs->blksz * 8) {
+		printf("Unsupported block group layout, not writing\n");
+		return -EINVAL;
+	}
+
 	/* get the superblock */
 	fs->sb = zalloc(SUPERBLOCK_SIZE);
 	if (!fs->sb)

[PATCH v2 4/6] fs: ext4: check block numbers before releasing them

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

When ext4fs_write() replaces an existing file, ext4fs_delete_file()
releases the file's blocks: the data blocks found by
read_allocated_block() and, for a file using indirect blocks, the
blocks listed in its indirect blocks. Each block number comes from the
inode or from an indirect block on disk and is turned into a block
group index used for fs->blk_bmaps[] and ext4fs_get_group_descriptor()
without any check. A block number past the end of the filesystem
makes ext4fs_reset_block_bmap() and ext4fs_bg_free_blocks_inc() write
outside the bitmap array and the group descriptor table.

Add ext4fs_blk_to_group(), which computes the block group of a block
as Linux ext4_get_group_no_and_offset() does, from the first data
block. As Linux does, it rejects a block that does not lie after the
first data block, which holds the superblock, and before the end of the
filesystem. Use it wherever the delete path computes the block group of
a block, in place of the open-coded division whose special case for
1 KiB blocks stood in for a first data block of 1.

Make the delete_*_indirect_block() helpers return an error, so that
ext4fs_delete_file() stops rather than releasing blocks of a corrupt
inode: -EINVAL for an invalid block number, -ENOMEM when out of
memory and -EIO when a read or the journal fails. That includes the
reads of the double and triple indirect blocks and of the blocks the
triple indirect block lists, which used to be unchecked: when one
failed, the zeroed buffer ended the walk at once, so the indirect block
was released and the blocks listed in it were leaked.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Move ext4fs_blk_to_group() to ext4_common.c and compute the group
  from the first data block, as Linux does, without the 1 KiB case
- Check the reads of the double and triple indirect blocks and of the
  blocks the triple indirect block lists
- Return -EIO for read and journal errors, -ENOMEM when out of memory

 fs/ext4/ext4_common.c |  35 +++++++++++++
 fs/ext4/ext4_common.h |   1 +
 fs/ext4/ext4_write.c  | 115 ++++++++++++++++++++++--------------------
 3 files changed, 97 insertions(+), 54 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index cff112fb99b..650597873f8 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -963,6 +963,41 @@ int ext4fs_filename_unlink(char *filename)
 	return -1;
 }
 
+/**
+ * ext4fs_blk_to_group() - get the block group of a block
+ *
+ * The block number may come from disk, so check that it lies after the
+ * first data block, which holds the superblock, and before the end of the
+ * filesystem, as Linux does, before it is used to index the block bitmaps
+ * and group descriptors. The group is computed as Linux
+ * ext4_get_group_no_and_offset() does.
+ *
+ * @fs:		filesystem
+ * @blknr:	block number
+ * Return:	block group of @blknr, or -EINVAL if it is not a valid block
+ */
+int ext4fs_blk_to_group(struct ext_filesystem *fs, uint64_t blknr)
+{
+	struct ext2_sblock *sblock = &ext4fs_root->sblock;
+	uint32_t first_data_block = le32_to_cpu(sblock->first_data_block);
+	uint32_t bg_idx;
+
+	if (blknr <= first_data_block ||
+	    blknr >= le32_to_cpu(sblock->total_blocks))
+		goto err;
+
+	/* blknr is below total_blocks, so it fits in 32 bits */
+	bg_idx = ((uint32_t)blknr - first_data_block) /
+		 le32_to_cpu(sblock->blocks_per_group);
+	if (bg_idx >= fs->no_blkgrp)
+		goto err;
+
+	return bg_idx;
+err:
+	printf("Invalid block number %llu\n", (unsigned long long)blknr);
+	return -EINVAL;
+}
+
 uint32_t ext4fs_get_new_blk_no(void)
 {
 	short i;
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
index 346752092b0..eecc11d9995 100644
--- a/fs/ext4/ext4_common.h
+++ b/fs/ext4/ext4_common.h
@@ -63,6 +63,7 @@ uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n);
 uint16_t ext4fs_checksum_update(unsigned int i);
 int ext4fs_get_parent_inode_num(const char *dirname, char *dname, int flags);
 int ext4fs_update_parent_dentry(char *filename, int file_type);
+int ext4fs_blk_to_group(struct ext_filesystem *fs, uint64_t blknr);
 uint32_t ext4fs_get_new_blk_no(void);
 int ext4fs_get_new_inode_no(void);
 void ext4fs_reset_block_bmap(long int blockno, unsigned char *buffer,
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 5f185387030..ea7843775b3 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -140,31 +140,29 @@ fail:
 	return -1;
 }
 
-static void delete_single_indirect_block(struct ext2_inode *inode)
+static int delete_single_indirect_block(struct ext2_inode *inode)
 {
 	struct ext2_block_group *bgd = NULL;
 	static int prev_bg_bmap_idx = -1;
 	uint32_t blknr;
-	int remainder;
 	int bg_idx;
 	int status;
-	uint32_t blk_per_grp = le32_to_cpu(ext4fs_root->sblock.blocks_per_group);
+	int ret = -EIO;
 	struct ext_filesystem *fs = get_fs();
 	char *journal_buffer = zalloc(fs->blksz);
 	if (!journal_buffer) {
 		printf("No memory\n");
-		return;
+		return -ENOMEM;
 	}
 
 	/* deleting the single indirect block associated with inode */
 	if (inode->b.blocks.indir_block != 0) {
 		blknr = le32_to_cpu(inode->b.blocks.indir_block);
 		debug("SIPB releasing %u\n", blknr);
-		bg_idx = blknr / blk_per_grp;
-		if (fs->blksz == 1024) {
-			remainder = blknr % blk_per_grp;
-			if (!remainder)
-				bg_idx--;
+		bg_idx = ext4fs_blk_to_group(fs, blknr);
+		if (bg_idx < 0) {
+			ret = bg_idx;
+			goto fail;
 		}
 		ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx], bg_idx);
 		/* get  block group descriptor table */
@@ -184,19 +182,21 @@ static void delete_single_indirect_block(struct ext2_inode *inode)
 			prev_bg_bmap_idx = bg_idx;
 		}
 	}
+	ret = 0;
 fail:
 	free(journal_buffer);
+
+	return ret;
 }
 
-static void delete_double_indirect_block(struct ext2_inode *inode)
+static int delete_double_indirect_block(struct ext2_inode *inode)
 {
 	int i;
 	short status;
 	static int prev_bg_bmap_idx = -1;
 	uint32_t blknr;
-	int remainder;
 	int bg_idx;
-	uint32_t blk_per_grp = le32_to_cpu(ext4fs_root->sblock.blocks_per_group);
+	int ret = -EIO;
 	__le32 *di_buffer = NULL;
 	void *dib_start_addr = NULL;
 	struct ext2_block_group *bgd = NULL;
@@ -204,29 +204,31 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
 	char *journal_buffer = zalloc(fs->blksz);
 	if (!journal_buffer) {
 		printf("No memory\n");
-		return;
+		return -ENOMEM;
 	}
 
 	if (inode->b.blocks.double_indir_block != 0) {
 		di_buffer = zalloc(fs->blksz);
 		if (!di_buffer) {
 			printf("No memory\n");
+			ret = -ENOMEM;
 			goto fail;
 		}
 		dib_start_addr = di_buffer;
 		blknr = le32_to_cpu(inode->b.blocks.double_indir_block);
 		status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0,
 					fs->blksz, (char *)di_buffer);
+		if (status == 0)
+			goto fail;
 		for (i = 0; i < fs->blksz / sizeof(int); i++) {
 			if (*di_buffer == 0)
 				break;
 
 			debug("DICB releasing %u\n", *di_buffer);
-			bg_idx = le32_to_cpu(*di_buffer) / blk_per_grp;
-			if (fs->blksz == 1024) {
-				remainder = le32_to_cpu(*di_buffer) % blk_per_grp;
-				if (!remainder)
-					bg_idx--;
+			bg_idx = ext4fs_blk_to_group(fs, le32_to_cpu(*di_buffer));
+			if (bg_idx < 0) {
+				ret = bg_idx;
+				goto fail;
 			}
 			/* get  block group descriptor table */
 			bgd = ext4fs_get_group_descriptor(fs, bg_idx);
@@ -255,11 +257,10 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
 
 		/* removing the parent double indirect block */
 		blknr = le32_to_cpu(inode->b.blocks.double_indir_block);
-		bg_idx = blknr / blk_per_grp;
-		if (fs->blksz == 1024) {
-			remainder = blknr % blk_per_grp;
-			if (!remainder)
-				bg_idx--;
+		bg_idx = ext4fs_blk_to_group(fs, blknr);
+		if (bg_idx < 0) {
+			ret = bg_idx;
+			goto fail;
 		}
 		/* get  block group descriptor table */
 		bgd = ext4fs_get_group_descriptor(fs, bg_idx);
@@ -280,20 +281,22 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
 		}
 		debug("DIPB releasing %d\n", blknr);
 	}
+	ret = 0;
 fail:
 	free(dib_start_addr);
 	free(journal_buffer);
+
+	return ret;
 }
 
-static void delete_triple_indirect_block(struct ext2_inode *inode)
+static int delete_triple_indirect_block(struct ext2_inode *inode)
 {
 	int i, j;
 	short status;
 	static int prev_bg_bmap_idx = -1;
 	uint32_t blknr;
-	int remainder;
 	int bg_idx;
-	uint32_t blk_per_grp = le32_to_cpu(ext4fs_root->sblock.blocks_per_group);
+	int ret = -EIO;
 	__le32 *tigp_buffer = NULL;
 	void *tib_start_addr = NULL;
 	__le32 *tip_buffer = NULL;
@@ -303,39 +306,46 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
 	char *journal_buffer = zalloc(fs->blksz);
 	if (!journal_buffer) {
 		printf("No memory\n");
-		return;
+		return -ENOMEM;
 	}
 
 	if (inode->b.blocks.triple_indir_block != 0) {
 		tigp_buffer = zalloc(fs->blksz);
 		if (!tigp_buffer) {
 			printf("No memory\n");
+			ret = -ENOMEM;
 			goto fail;
 		}
 		tib_start_addr = tigp_buffer;
 		blknr = le32_to_cpu(inode->b.blocks.triple_indir_block);
 		status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0,
 					fs->blksz, (char *)tigp_buffer);
+		if (status == 0)
+			goto fail;
 		for (i = 0; i < fs->blksz / sizeof(int); i++) {
 			if (*tigp_buffer == 0)
 				break;
 			debug("tigp buffer releasing %u\n", *tigp_buffer);
 
 			tip_buffer = zalloc(fs->blksz);
-			if (!tip_buffer)
+			if (!tip_buffer) {
+				ret = -ENOMEM;
 				goto fail;
+			}
 			tipb_start_addr = tip_buffer;
 			status = ext4fs_devread((lbaint_t)le32_to_cpu(*tigp_buffer) *
 						fs->sect_perblk, 0, fs->blksz,
 						(char *)tip_buffer);
+			if (status == 0)
+				goto fail;
 			for (j = 0; j < fs->blksz / sizeof(int); j++) {
 				if (le32_to_cpu(*tip_buffer) == 0)
 					break;
-				bg_idx = le32_to_cpu(*tip_buffer) / blk_per_grp;
-				if (fs->blksz == 1024) {
-					remainder = le32_to_cpu(*tip_buffer) % blk_per_grp;
-					if (!remainder)
-						bg_idx--;
+				bg_idx = ext4fs_blk_to_group(fs,
+							     le32_to_cpu(*tip_buffer));
+				if (bg_idx < 0) {
+					ret = bg_idx;
+					goto fail;
 				}
 
 				ext4fs_reset_block_bmap(le32_to_cpu(*tip_buffer),
@@ -373,11 +383,10 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
 			 * removing the grand parent blocks
 			 * which is connected to inode
 			 */
-			bg_idx = le32_to_cpu(*tigp_buffer) / blk_per_grp;
-			if (fs->blksz == 1024) {
-				remainder = le32_to_cpu(*tigp_buffer) % blk_per_grp;
-				if (!remainder)
-					bg_idx--;
+			bg_idx = ext4fs_blk_to_group(fs, le32_to_cpu(*tigp_buffer));
+			if (bg_idx < 0) {
+				ret = bg_idx;
+				goto fail;
 			}
 			ext4fs_reset_block_bmap(le32_to_cpu(*tigp_buffer),
 						fs->blk_bmaps[bg_idx], bg_idx);
@@ -408,11 +417,10 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
 
 		/* removing the grand parent triple indirect block */
 		blknr = le32_to_cpu(inode->b.blocks.triple_indir_block);
-		bg_idx = blknr / blk_per_grp;
-		if (fs->blksz == 1024) {
-			remainder = blknr % blk_per_grp;
-			if (!remainder)
-				bg_idx--;
+		bg_idx = ext4fs_blk_to_group(fs, blknr);
+		if (bg_idx < 0) {
+			ret = bg_idx;
+			goto fail;
 		}
 		ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx], bg_idx);
 		/* get  block group descriptor table */
@@ -433,10 +441,13 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
 		}
 		debug("tigp buffer itself releasing %d\n", blknr);
 	}
+	ret = 0;
 fail:
 	free(tib_start_addr);
 	free(tipb_start_addr);
 	free(journal_buffer);
+
+	return ret;
 }
 
 static int ext4fs_delete_file(int inodeno)
@@ -444,7 +455,6 @@ static int ext4fs_delete_file(int inodeno)
 	struct ext2_inode inode;
 	short status;
 	int i;
-	int remainder;
 	long int blknr;
 	int bg_idx;
 	int ibmap_idx;
@@ -456,7 +466,6 @@ static int ext4fs_delete_file(int inodeno)
 	unsigned int inodes_per_block;
 	uint32_t blkno;
 	unsigned int blkoff;
-	uint32_t blk_per_grp = le32_to_cpu(ext4fs_root->sblock.blocks_per_group);
 	uint32_t inode_per_grp = le32_to_cpu(ext4fs_root->sblock.inodes_per_group);
 	struct ext2_inode *inode_buffer = NULL;
 	struct ext2_block_group *bgd = NULL;
@@ -489,9 +498,10 @@ static int ext4fs_delete_file(int inodeno)
 				inode.b.blocks.dir_blocks;
 		debug("del: dep=%d entries=%d\n", eh->eh_depth, eh->eh_entries);
 	} else {
-		delete_single_indirect_block(&inode);
-		delete_double_indirect_block(&inode);
-		delete_triple_indirect_block(&inode);
+		if (delete_single_indirect_block(&inode) ||
+		    delete_double_indirect_block(&inode) ||
+		    delete_triple_indirect_block(&inode))
+			goto fail;
 	}
 
 	/* release data blocks */
@@ -501,12 +511,9 @@ static int ext4fs_delete_file(int inodeno)
 			continue;
 		if (blknr < 0)
 			goto fail;
-		bg_idx = blknr / blk_per_grp;
-		if (fs->blksz == 1024) {
-			remainder = blknr % blk_per_grp;
-			if (!remainder)
-				bg_idx--;
-		}
+		bg_idx = ext4fs_blk_to_group(fs, blknr);
+		if (bg_idx < 0)
+			goto fail;
 		ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx],
 					bg_idx);
 		debug("EXT4 Block releasing %ld: %d\n", blknr, bg_idx);

[PATCH v2 5/6] fs: ext4: fix the block group of an inode in the write path

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Inode numbers start at 1, so the block group of inode n is
(n - 1) / s_inodes_per_group, as ext4fs_read_inode() computes it. The
write path computes n / s_inodes_per_group instead, in
ext4fs_delete_file(), for the new and the parent inode in
ext4fs_write(), and in ext4fs_get_new_inode_no(). For the last inode
of a group this gives the next group: ext4fs_delete_file() then clears
a bit in the byte before that group's inode bitmap and zeroes an inode
in the wrong inode table, and for the last group of the filesystem
the index is past the end of fs->inode_bmaps[] and the group
descriptor table.

Add ext4fs_inode_to_group(), next to ext4fs_blk_to_group(), which
computes the group that way, and use it in all four places. The inode
numbers given to ext4fs_delete_file() and taken as the parent inode
come from directory entries on disk, so it also checks the inode as
Linux ext4_valid_inum() does: one past the inode count or the last
block group, or a reserved inode other than the root, such as the
journal's, is reported and refused rather than having its blocks
released and its inode zeroed.

When ext4fs_get_new_inode_no() finds no free inode after the last one
it allocated, it goes on past the last group's inode bitmap. Make it
stop at the inode count instead.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Add ext4fs_inode_to_group() and use it for every inode bitmap index,
  so a bad parent inode is now reported too
- Reject reserved inodes other than the root, as ext4_valid_inum() does

 fs/ext4/ext4_common.c | 49 +++++++++++++++++++++++++++++++++++++++----
 fs/ext4/ext4_common.h |  1 +
 fs/ext4/ext4_write.c  | 17 ++++++++++-----
 3 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 650597873f8..273e6628b8d 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -998,6 +998,41 @@ err:
 	return -EINVAL;
 }
 
+/**
+ * ext4fs_inode_to_group() - get the block group of an inode
+ *
+ * The inode number may come from disk, so check it before it is used to
+ * index the inode bitmaps and group descriptors: as Linux ext4_valid_inum()
+ * does, it must be the root or a non-reserved inode within the inode count,
+ * and its block group must be inside the filesystem.
+ *
+ * @fs:		filesystem
+ * @inodeno:	inode number
+ * Return:	block group of @inodeno, or -EINVAL if it is not a valid inode
+ */
+int ext4fs_inode_to_group(struct ext_filesystem *fs, int inodeno)
+{
+	struct ext2_sblock *sblock = &ext4fs_root->sblock;
+	uint32_t first_ino = EXT2_GOOD_OLD_FIRST_INO;
+	uint32_t group;
+
+	if (le32_to_cpu(sblock->revision_level) != EXT2_GOOD_OLD_REV)
+		first_ino = le32_to_cpu(sblock->first_inode);
+	if (inodeno != EXT2_ROOT_INO &&
+	    (inodeno < 1 || (uint32_t)inodeno < first_ino ||
+	     (uint32_t)inodeno > le32_to_cpu(sblock->total_inodes)))
+		goto err;
+
+	group = (inodeno - 1) / le32_to_cpu(sblock->inodes_per_group);
+	if (group >= fs->no_blkgrp)
+		goto err;
+
+	return group;
+err:
+	printf("Invalid inode number %d\n", inodeno);
+	return -EINVAL;
+}
+
 uint32_t ext4fs_get_new_blk_no(void)
 {
 	short i;
@@ -1131,7 +1166,7 @@ int ext4fs_get_new_inode_no(void)
 {
 	short i;
 	short status;
-	unsigned int ibmap_idx;
+	int ibmap_idx;
 	static int prev_inode_bitmap_index = -1;
 	unsigned int inodes_per_grp = le32_to_cpu(ext4fs_root->sblock.inodes_per_group);
 	struct ext_filesystem *fs = get_fs();
@@ -1191,8 +1226,14 @@ int ext4fs_get_new_inode_no(void)
 	} else {
 restart:
 		fs->curr_inode_no++;
-		/* get the blockbitmap index respective to blockno */
-		ibmap_idx = fs->curr_inode_no / inodes_per_grp;
+		/* no free inode left after the last one allocated */
+		if (fs->curr_inode_no >
+		    le32_to_cpu(ext4fs_root->sblock.total_inodes))
+			goto fail;
+		/* get the inode bitmap index respective to inode no */
+		ibmap_idx = ext4fs_inode_to_group(fs, fs->curr_inode_no);
+		if (ibmap_idx < 0)
+			goto fail;
 		struct ext2_block_group *bgd =
 			ext4fs_get_group_descriptor(fs, ibmap_idx);
 		uint16_t bg_flags = ext4fs_bg_get_flags(bgd);
@@ -1210,7 +1251,7 @@ restart:
 		if (ext4fs_set_inode_bmap(fs->curr_inode_no,
 					  fs->inode_bmaps[ibmap_idx],
 					  ibmap_idx) != 0) {
-			debug("going for restart for the block no %d %u\n",
+			debug("going for restart for the block no %d %d\n",
 			      fs->curr_inode_no, ibmap_idx);
 			goto restart;
 		}
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
index eecc11d9995..eda04b5b3a3 100644
--- a/fs/ext4/ext4_common.h
+++ b/fs/ext4/ext4_common.h
@@ -64,6 +64,7 @@ uint16_t ext4fs_checksum_update(unsigned int i);
 int ext4fs_get_parent_inode_num(const char *dirname, char *dname, int flags);
 int ext4fs_update_parent_dentry(char *filename, int file_type);
 int ext4fs_blk_to_group(struct ext_filesystem *fs, uint64_t blknr);
+int ext4fs_inode_to_group(struct ext_filesystem *fs, int inodeno);
 uint32_t ext4fs_get_new_blk_no(void);
 int ext4fs_get_new_inode_no(void);
 void ext4fs_reset_block_bmap(long int blockno, unsigned char *buffer,
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index ea7843775b3..0cbbdc6c934 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -473,6 +473,9 @@ static int ext4fs_delete_file(int inodeno)
 	char *journal_buffer = zalloc(fs->blksz);
 	if (!journal_buffer)
 		return -ENOMEM;
+	ibmap_idx = ext4fs_inode_to_group(fs, inodeno);
+	if (ibmap_idx < 0)
+		goto fail;
 	status = ext4fs_read_inode(ext4fs_root, inodeno, &inode);
 	if (status == 0)
 		goto fail;
@@ -539,7 +542,6 @@ static int ext4fs_delete_file(int inodeno)
 	/* release inode */
 	/* from the inode no to blockno */
 	inodes_per_block = fs->blksz / fs->inodesz;
-	ibmap_idx = inodeno / inode_per_grp;
 
 	/* get the block no */
 	inodeno--;
@@ -892,7 +894,8 @@ int ext4fs_write(const char *fname, const char *buffer,
 	long int blkoff;
 	struct ext2_sblock *sblock = &(ext4fs_root->sblock);
 	unsigned int inodes_per_block;
-	unsigned int ibmap_idx;
+	int ibmap_idx;
+	int parent_ibmap_idx;
 	struct ext2_block_group *bgd = NULL;
 	struct ext_filesystem *fs = get_fs();
 	ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256);
@@ -929,6 +932,9 @@ int ext4fs_write(const char *fname, const char *buffer,
 	parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
 	if (parent_inodeno == -1)
 		goto fail;
+	parent_ibmap_idx = ext4fs_inode_to_group(fs, parent_inodeno);
+	if (parent_ibmap_idx < 0)
+		goto fail;
 	if (ext4fs_iget(parent_inodeno, g_parent_inode))
 		goto fail;
 	/* do not mess up a directory using hash trees */
@@ -1020,7 +1026,9 @@ int ext4fs_write(const char *fname, const char *buffer,
 	temp_ptr = zalloc(fs->blksz);
 	if (!temp_ptr)
 		goto fail;
-	ibmap_idx = inodeno / le32_to_cpu(ext4fs_root->sblock.inodes_per_group);
+	ibmap_idx = ext4fs_inode_to_group(fs, inodeno);
+	if (ibmap_idx < 0)
+		goto fail;
 	inodeno--;
 	bgd = ext4fs_get_group_descriptor(fs, ibmap_idx);
 	itable_blkno = ext4fs_bg_get_inode_table_id(bgd, fs) +
@@ -1041,9 +1049,8 @@ int ext4fs_write(const char *fname, const char *buffer,
 		/* FIXME: Deallocate data blocks */
 		goto fail;
 	}
-	ibmap_idx = parent_inodeno / le32_to_cpu(ext4fs_root->sblock.inodes_per_group);
 	parent_inodeno--;
-	bgd = ext4fs_get_group_descriptor(fs, ibmap_idx);
+	bgd = ext4fs_get_group_descriptor(fs, parent_ibmap_idx);
 	parent_itable_blkno = ext4fs_bg_get_inode_table_id(bgd, fs) +
 	    (parent_inodeno %
 	     le32_to_cpu(sblock->inodes_per_group)) / inodes_per_block;

[PATCH v2 6/6] test: fs: ext4: add tests for corrupt on-disk indices

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Add tests that make a small ext4 image with mkfs.ext4 and check how
U-Boot handles the on-disk indices that the previous patches check:

- test_fs_ext_oob_extent_header: an extent header with more entries
  than the inode can hold must give "invalid extent block"
- test_fs_ext_oob_extent_loop: an extent index node pointing back at
  itself must give "invalid extent block" rather than loop for ever
- test_fs_ext_oob_triple_indirect: a sparse file must read back
  correctly through the second and third entries of its triple
  indirect block
- test_fs_ext_oob_group_layout: writing to a filesystem with
  s_blocks_per_group of zero must be refused
- test_fs_ext_oob_delete_block: replacing a file whose indirect block
  lies past the end of the filesystem must fail with "Invalid block
  number"
- test_fs_ext_oob_delete_inode: replacing a file whose directory entry
  names an inode past the last block group, or the journal's inode,
  must fail with "Invalid inode number"
- test_fs_ext_oob_last_inode: replacing a file whose inode is the last
  of its block group must work and leave the filesystem clean for
  e2fsck

Each image is removed and host device 0 unbound even when a test fails.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v2:
- Say that some tests patch the image directly rather than with debugfs
- Replace victim_field() with victim_has(), using DEVNULL
- Remove the image and unbind host 0 even when a test fails
- Use f-strings and record the new file's pylint score as 10.00
- Add tests for the extent depth check, the triple indirect lookup, the
  last inode of a group and a directory entry naming the journal's inode

 scripts/pylint.base                   |   1 +
 test/py/tests/test_fs/test_ext_oob.py | 310 ++++++++++++++++++++++++++
 2 files changed, 311 insertions(+)

diff --git a/scripts/pylint.base b/scripts/pylint.base
index bc39e2385a3..1905e32588f 100644
--- a/scripts/pylint.base
+++ b/scripts/pylint.base
@@ -50,6 +50,7 @@ test_tests_test_fs_fstest_helpers.py 4.29
 test_tests_test_fs_test_basic.py 1.90
 test_tests_test_fs_test_erofs.py 8.97
 test_tests_test_fs_test_ext.py 1.48
+test_tests_test_fs_test_ext_oob.py 10.00
 test_tests_test_fs_test_fs_cmd.py 8.00
 test_tests_test_fs_test_fs_fat.py 2.50
 test_tests_test_fs_test_mkdir.py 3.04
diff --git a/test/py/tests/test_fs/test_ext_oob.py b/test/py/tests/test_fs/test_ext_oob.py
new file mode 100644
index 00000000000..12f4095c6e5
--- /dev/null
+++ b/test/py/tests/test_fs/test_ext_oob.py
@@ -0,0 +1,310 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+"""Test ext4 handling of corrupt on-disk indices
+
+Most tests make a small ext4 image with mkfs.ext4, corrupt one on-disk
+field, with debugfs or by patching the image directly, and check that
+U-Boot rejects the image and is still responsive afterwards. The others
+check that U-Boot reads or replaces a valid file whose blocks or inode lie
+at a boundary that it used to get wrong.
+"""
+
+from contextlib import contextmanager
+import os
+import struct
+from subprocess import run, check_output, DEVNULL
+import zlib
+import pytest
+from fstest_defs import ADDR
+
+VICTIM = b'victim file\n'
+
+
+@contextmanager
+def ext4_image(ubman, prefix, opts=()):
+    """Make an 8 MiB ext4 image and remove it once the test is done
+
+    mk_fs() takes no mkfs.ext4 options, which some tests need. When the with
+    block ends, whether or not the test passed, the image is removed and
+    host device 0, which the test may have bound to it, is unbound.
+
+    Args:
+        ubman (ConsoleBase): U-Boot console
+        prefix (str): prefix of the image file name
+        opts (list of str): extra options for mkfs.ext4
+
+    Yields:
+        str: path of the image
+    """
+    img = os.path.join(ubman.config.persistent_data_dir, f'{prefix}.ext4.img')
+    with open(img, 'wb') as fil:
+        fil.truncate(8 << 20)
+    try:
+        # U-Boot does not write to a filesystem with metadata checksums
+        run(['mkfs.ext4', '-q', '-F', '-O', '^metadata_csum', *opts, img],
+            check=True)
+        yield img
+    finally:
+        os.remove(img)
+        ubman.run_command('host unbind 0')
+
+
+def debugfs(img, cmds):
+    """Run debugfs commands which write to an image
+
+    Args:
+        img (str): path of the image
+        cmds (list of str): debugfs commands
+    """
+    script = '\n'.join(cmds) + '\n'
+    run(['debugfs', '-w', '-f', '-', img], input=script.encode(),
+        capture_output=True, check=True)
+
+
+def add_file(img, name, chunks):
+    """Write a file into the root directory of an image with debugfs
+
+    Args:
+        img (str): path of the image
+        name (str): name of the file
+        chunks (dict): data of the file (bytes) by offset (int), the rest
+            of the file being left as holes
+    """
+    src = f'{img}.{name}'
+    with open(src, 'wb') as fil:
+        for offset, data in chunks.items():
+            fil.seek(offset)
+            fil.write(data)
+    try:
+        debugfs(img, [f'write {src} {name}'])
+    finally:
+        os.remove(src)
+
+
+def read_at(img, offset, size):
+    """Read size bytes of the image at offset"""
+    with open(img, 'rb') as fil:
+        fil.seek(offset)
+        return fil.read(size)
+
+
+def write_at(img, offset, data):
+    """Write data into the image at offset"""
+    with open(img, 'r+b') as fil:
+        fil.seek(offset)
+        fil.write(data)
+
+
+def block_size(img):
+    """Get the block size of the filesystem in an image"""
+    return 1024 << struct.unpack('<I', read_at(img, 1024 + 24, 4))[0]
+
+
+def file_block(img, path, blk):
+    """Get the filesystem block which holds a block of a file"""
+    return int(check_output(['debugfs', '-R', f'bmap {path} {blk}', img],
+                            stderr=DEVNULL).decode())
+
+
+def victim_has(img, text):
+    """Check that debugfs shows some text in the inode of /victim"""
+    out = check_output(['debugfs', '-R', 'stat victim', img],
+                       stderr=DEVNULL).decode()
+    return text in out
+
+
+def load_crc(ubman, size, pos):
+    """Read part of /victim from host device 0 and get its CRC32
+
+    Args:
+        ubman (ConsoleBase): U-Boot console
+        size (int): number of bytes to read
+        pos (int): offset in the file to read from
+
+    Returns:
+        str: output of the crc32 command
+    """
+    ubman.run_command(f'ext4load host 0:0 {ADDR:x} /victim {size:x} {pos:x}')
+    return ubman.run_command(f'crc32 {ADDR:x} {size:x}')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_extent_header(ubman):
+    """ext4fs_get_extent_block(): an extent header claiming more entries
+    than the inode's i_block area holds must be rejected"""
+    with ext4_image(ubman, 'ext_oob_eh') as img:
+        add_file(img, 'victim', {0: VICTIM})
+        # i_block[0] = eh_magic | eh_entries << 16: 255 entries, eh_max stays 4
+        debugfs(img, ['sif victim block[0] 0x00fff30a'])
+        assert victim_has(img, 'Flags: 0x80000')
+
+        ubman.run_command(f'host bind 0 {img}')
+        out = ubman.run_command(f'ext4load host 0:0 {ADDR:x} /victim')
+        assert 'invalid extent block' in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_extent_loop(ubman):
+    """ext4fs_get_extent_block(): an index node pointing back at itself, so
+    that the depth does not drop, must be rejected rather than walked for
+    ever"""
+    with ext4_image(ubman, 'ext_oob_loop') as img:
+        add_file(img, 'victim', {0: VICTIM})
+        blksz = block_size(img)
+        blk = file_block(img, 'victim', 0)
+        # Make the data block an index node of depth 1 with one entry, which
+        # points at the node itself: eh_magic, eh_entries, eh_max, eh_depth,
+        # eh_generation, then ei_block, ei_leaf_lo, ei_leaf_hi, ei_unused
+        write_at(img, blk * blksz,
+                 struct.pack('<4HI2I2H', 0xf30a, 1, (blksz - 12) // 12, 1, 0,
+                             0, blk, 0, 0))
+        # Make the root an index node of depth 1 pointing at that block:
+        # i_block[1] = eh_max | eh_depth << 16, i_block[4] = ei_leaf_lo
+        debugfs(img, ['sif victim block[1] 0x00010004',
+                      f'sif victim block[4] {blk}', 'sif victim block[5] 0'])
+
+        ubman.run_command(f'host bind 0 {img}')
+        out = ubman.run_command(f'ext4load host 0:0 {ADDR:x} /victim')
+        assert 'invalid extent block' in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4')
+@pytest.mark.buildconfigspec('cmd_crc32')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_triple_indirect(ubman):
+    """read_allocated_block(): blocks reached through the second and third
+    entries of the triple indirect block must be read correctly"""
+    # With 1 KiB blocks an indirect block holds 256 block numbers. The file
+    # blocks after the 12 direct, 256 single and 256 * 256 double indirect
+    # ones are reached through the triple indirect block, each entry of which
+    # maps 256 * 256 blocks.
+    per = 256
+    tind = 12 + per + per * per
+    # The first block of entry 1, the last block of its first indirect block
+    # and the first of its second, and the first block of entry 2
+    blocks = [tind + per * per, tind + per * per + per - 1,
+              tind + per * per + per, tind + 2 * per * per]
+    chunks = {blk * 1024: f'{blk:08x}'.encode() * 128 for blk in blocks}
+    with ext4_image(ubman, 'ext_oob_tind',
+                    ['-b', '1024', '-O', '^extent,^64bit']) as img:
+        add_file(img, 'victim', chunks)
+        assert victim_has(img, '(TIND)')
+
+        ubman.run_command(f'host bind 0 {img}')
+        for first, count in ((blocks[0], 1), (blocks[1], 2), (blocks[3], 1)):
+            data = b''.join(chunks[(first + i) * 1024] for i in range(count))
+            out = load_crc(ubman, len(data), first * 1024)
+            assert f'==> {zlib.crc32(data):08x}' in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4_write')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_group_layout(ubman):
+    """ext4fs_init(): a superblock with zero blocks per group must make
+    writes fail rather than divide by zero"""
+    with ext4_image(ubman, 'ext_oob_geo') as img:
+        # s_blocks_per_group is at offset 32 of the superblock at byte 1024
+        write_at(img, 1024 + 32, struct.pack('<I', 0))
+
+        ubman.run_command(f'host bind 0 {img}')
+        ubman.run_command(f'mw.b {ADDR:x} 41 1000')
+        out = ubman.run_command(
+            f'ext4write host 0:0 {ADDR:x} /newfile 0x1000')
+        assert 'Unsupported block group layout' in out
+        assert 'written' not in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4_write')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_delete_block(ubman):
+    """delete_single_indirect_block(): replacing a file whose indirect block
+    lies past the end of the filesystem must fail before it is released"""
+    with ext4_image(ubman, 'ext_oob_del') as img:
+        add_file(img, 'victim', {0: VICTIM})
+        # Make /victim a block-mapped file whose only bad field is block[IND]
+        cmds = ['sif victim flags 0', 'sif victim size 4096',
+                'sif victim block[IND] 0x00ffff00']
+        cmds += [f'sif victim block[{i}] 0' for i in range(12)]
+        debugfs(img, cmds)
+        assert victim_has(img, '(IND):16776960')
+
+        ubman.run_command(f'host bind 0 {img}')
+        ubman.run_command(f'mw.b {ADDR:x} 41 1000')
+        out = ubman.run_command(f'ext4write host 0:0 {ADDR:x} /victim 0x1000')
+        assert 'Invalid block number' in out
+        assert 'written' not in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4_write')
+@pytest.mark.requiredtool('debugfs')
+@pytest.mark.parametrize('inum', [1000000, 8])
+def test_fs_ext_oob_delete_inode(ubman, inum):
+    """ext4fs_delete_file(): replacing a file whose directory entry names an
+    inode past the last block group, or a reserved inode such as the
+    journal's (8), must fail"""
+    with ext4_image(ubman, f'ext_oob_ino{inum}') as img:
+        add_file(img, 'victim', {0: VICTIM})
+        # Point the /victim entry of the root directory at the inode; debugfs
+        # refuses to link a missing inode, so patch the entry in place
+        blksz = block_size(img)
+        blk = file_block(img, '/', 0)
+        data = bytearray(read_at(img, blk * blksz, blksz))
+        off = 0
+        while off < blksz:
+            rec_len, name_len = struct.unpack_from('<HB', data, off + 4)
+            if data[off + 8:off + 8 + name_len] == b'victim':
+                struct.pack_into('<I', data, off, inum)
+                break
+            off += rec_len
+        else:
+            pytest.fail('no /victim entry in the root directory')
+        write_at(img, blk * blksz, data)
+
+        ubman.run_command(f'host bind 0 {img}')
+        ubman.run_command(f'mw.b {ADDR:x} 41 1000')
+        out = ubman.run_command(f'ext4write host 0:0 {ADDR:x} /victim 0x1000')
+        assert f'Invalid inode number {inum}' in out
+        assert 'written' not in out
+        assert 'alive' in ubman.run_command('echo alive')
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4_write')
+@pytest.mark.buildconfigspec('cmd_crc32')
+@pytest.mark.requiredtool('debugfs')
+@pytest.mark.requiredtool('e2fsck')
+def test_fs_ext_oob_last_inode(ubman):
+    """ext4fs_delete_file(), ext4fs_write(): replacing a file whose inode is
+    the last of its block group must use that group's inode bitmap and
+    inode table"""
+    # With 16 inodes per group, fill the free inodes of group 0 but the last
+    # one, inode 16, which /victim then takes
+    with ext4_image(ubman, 'ext_oob_last', ['-N', '16']) as img:
+        out = check_output(['debugfs', '-R', 'ffi', img],
+                           stderr=DEVNULL).decode()
+        for num in range(int(out.split()[-1]), 16):
+            add_file(img, f'file{num}', {0: VICTIM})
+        add_file(img, 'victim', {0: VICTIM})
+        assert victim_has(img, 'Inode: 16 ')
+
+        ubman.run_command(f'host bind 0 {img}')
+        ubman.run_command(f'mw.b {ADDR:x} 41 1000')
+        out = ubman.run_command(f'ext4write host 0:0 {ADDR:x} /victim 0x1000')
+        assert 'written' in out
+        out = load_crc(ubman, 0x1000, 0)
+        assert f'==> {zlib.crc32(b"A" * 0x1000):08x}' in out
+        assert 'alive' in ubman.run_command('echo alive')
+        run(['e2fsck', '-fn', img], capture_output=True, check=True)

Re: [PATCH v2 4/6] fs: ext4: check block numbers before releasing them

[Simon Glass]

On 2026-10-28T05:50:47, Ana Bobillo <anabobillo3@gmail.com> wrote:
> fs: ext4: check block numbers before releasing them
>
> When ext4fs_write() replaces an existing file, ext4fs_delete_file()
> releases the file's blocks: the data blocks found by
> read_allocated_block() and, for a file using indirect blocks, the
> blocks listed in its indirect blocks. Each block number comes from the
> inode or from an indirect block on disk and is turned into a block
> group index used for fs->blk_bmaps[] and ext4fs_get_group_descriptor()
> without any check. A block number past the end of the filesystem
> makes ext4fs_reset_block_bmap() and ext4fs_bg_free_blocks_inc() write
> outside the bitmap array and the group descriptor table.
>
> Add ext4fs_blk_to_group(), which computes the block group of a block
> as Linux ext4_get_group_no_and_offset() does, from the first data
> block. As Linux does, it rejects a block that does not lie after the
> first data block, which holds the superblock, and before the end of the
> filesystem. Use it wherever the delete path computes the block group of
> a block, in place of the open-coded division whose special case for
> 1 KiB blocks stood in for a first data block of 1.
>
> Make the delete_*_indirect_block() helpers return an error, so that
> ext4fs_delete_file() stops rather than releasing blocks of a corrupt
> inode: -EINVAL for an invalid block number, -ENOMEM when out of
> memory and -EIO when a read or the journal fails. That includes the
> reads of the double and triple indirect blocks and of the blocks the
> triple indirect block lists, which used to be unchecked: when one
> failed, the zeroed buffer ended the walk at once, so the indirect block
> was released and the blocks listed in it were leaked.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  fs/ext4/ext4_common.c |  35 +++++++++++++++
>  fs/ext4/ext4_common.h |   1 +
>  fs/ext4/ext4_write.c  | 115 ++++++++++++++++++++++++++------------------------
>  3 files changed, 97 insertions(+), 54 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 5/6] fs: ext4: fix the block group of an inode in the write path

[Simon Glass]

On 2026-10-28T05:50:47, Ana Bobillo <anabobillo3@gmail.com> wrote:
> fs: ext4: fix the block group of an inode in the write path
>
> Inode numbers start at 1, so the block group of inode n is
> (n - 1) / s_inodes_per_group, as ext4fs_read_inode() computes it. The
> write path computes n / s_inodes_per_group instead, in
> ext4fs_delete_file(), for the new and the parent inode in
> ext4fs_write(), and in ext4fs_get_new_inode_no(). For the last inode
> of a group this gives the next group: ext4fs_delete_file() then clears
> a bit in the byte before that group's inode bitmap and zeroes an inode
> in the wrong inode table, and for the last group of the filesystem
> the index is past the end of fs->inode_bmaps[] and the group
> descriptor table.
>
> Add ext4fs_inode_to_group(), next to ext4fs_blk_to_group(), which
> computes the group that way, and use it in all four places. The inode
> numbers given to ext4fs_delete_file() and taken as the parent inode
> come from directory entries on disk, so it also checks the inode as
> Linux ext4_valid_inum() does: one past the inode count or the last
> block group, or a reserved inode other than the root, such as the
> journal's, is reported and refused rather than having its blocks
> released and its inode zeroed.
>
> When ext4fs_get_new_inode_no() finds no free inode after the last one
> it allocated, it goes on past the last group's inode bitmap. Make it
> stop at the inode count instead.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  fs/ext4/ext4_common.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  fs/ext4/ext4_common.h |  1 +
>  fs/ext4/ext4_write.c  | 17 ++++++++++++-----
>  3 files changed, 58 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 6/6] test: fs: ext4: add tests for corrupt on-disk indices

[Simon Glass]

On 2026-10-28T05:50:47, Ana Bobillo <anabobillo3@gmail.com> wrote:
> test: fs: ext4: add tests for corrupt on-disk indices
>
> Add tests that make a small ext4 image with mkfs.ext4 and check how
> U-Boot handles the on-disk indices that the previous patches check:
>
> - test_fs_ext_oob_extent_header: an extent header with more entries
>   than the inode can hold must give "invalid extent block"
> - test_fs_ext_oob_extent_loop: an extent index node pointing back at
>   itself must give "invalid extent block" rather than loop for ever
> - test_fs_ext_oob_triple_indirect: a sparse file must read back
>   correctly through the second and third entries of its triple
>   indirect block
> - test_fs_ext_oob_group_layout: writing to a filesystem with
>   s_blocks_per_group of zero must be refused
> - test_fs_ext_oob_delete_block: replacing a file whose indirect block
>   lies past the end of the filesystem must fail with "Invalid block
>   number"
> - test_fs_ext_oob_delete_inode: replacing a file whose directory entry
>   names an inode past the last block group, or the journal's inode,
>   must fail with "Invalid inode number"
> - test_fs_ext_oob_last_inode: replacing a file whose inode is the last
>   of its block group must work and leave the filesystem clean for
>   e2fsck
>
> Each image is removed and host device 0 unbound even when a test fails.
>
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
>
>  scripts/pylint.base                   |   1 +
>  test/py/tests/test_fs/test_ext_oob.py | 310 ++++++++++++++++++++++++++++++++++
>  2 files changed, 311 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>