[PATCH 0/6] fs: ext4: check on-disk values in the write path

[PATCH 0/6] fs: ext4: check on-disk values in the write path

[Ana Bobillo]

When ext4write (or saveenv with the environment on ext4) writes to a
filesystem, it walks the parent directory's block and the journal by
values it reads from the image. A corrupt or crafted image can then make
it write a directory entry past the end of its block buffer, overflow
the fixed journal entry tables, or loop for ever in the journal scan
that runs before every write. The write path also takes the first data
block to be the one mke2fs makes, while ext4fs_blk_to_group() reads it
from the superblock, so that the two can disagree on the block group of
a block.

This series follows "fs: ext4: check on-disk indices before using them"
v2, which covers the read path and the delete path, and applies on top
of it:
https://patchwork.ozlabs.org/project/uboot/list/?series=530226

1. the write path refuses a first data block other than the one its
   bitmap code assumes, so that ext4fs_blk_to_group() and the bitmap
   code always agree on the group of a block
2. ext4fs_get_new_blk_no() uses ext4fs_blk_to_group(), as Simon
   suggested on that series, after a quiet check for the end of the
   filesystem
3. directory entries: one helper checks that each entry lies inside its
   block and that its length covers its name, and the three functions
   that walk a directory block (the walk that adds a name, search_dir()
   and unlink_filename()) use it
4. the journal entry tables: every function that adds an entry checks
   for room, counting the entries already used, and a transaction logs
   no more blocks than one descriptor block has tags for
5. the journal scan ends at a block without the journal magic, of an
   unknown type, unmapped or past the end of the journal, or at a revoke
   block whose count is larger than the block
6. tests in test/py for 1, 3 and 5

With sandbox_defconfig and CONFIG_ASAN, on the earlier series the new
directory test corrupts the heap (dlmalloc's check fails, "Assertion
`inuse(p)' failed"), the journal test times out in the scan, and the
first data block test replaces the file, which under valgrind reads one
byte past the end of a block bitmap in ext4fs_reset_block_bmap(); with
this series all three pass. The existing ext4 tests still pass with
--fs-type ext4. Writing files to filesystems of several block groups
until they are full, with 1 KiB and 4 KiB blocks, with and without
extents, gives the same block counts as without this series, and
e2fsck finds them clean.

The journal table limit only bites on filesystems with very many block
groups, so no test makes one; with the default of 100 entries a normal
write uses a handful.

CI: the series passes all 91 jobs of U-Boot's Azure pipeline, in build
13738 of my CI-only pull request, on top of the ext4 v2 above (Patchwork
series 530226) on main feb8913982e.
PR:    https://github.com/u-boot/u-boot/pull/1064
Build: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=13738

Ana Bobillo (6):
  fs: ext4: refuse to write with an unexpected first data block
  fs: ext4: use ext4fs_blk_to_group() in ext4fs_get_new_blk_no()
  fs: ext4: check directory entries before using them
  fs: ext4: bound the journal entry tables
  fs: ext4: stop the journal scan at the end of the log
  test: fs: ext4: add tests for the write path checks

 fs/ext4/ext4_common.c                 | 122 ++++++++++++++++----------
 fs/ext4/ext4_journal.c                | 105 +++++++++++++++-------
 fs/ext4/ext4_write.c                  |  13 ++-
 test/py/tests/test_fs/test_ext_oob.py | 115 +++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 85 deletions(-)


base-commit: 8649a54d301a4d0ac526324c7a7ad2574939ff27
prerequisite-patch-id: d5a6719867877bf4c0422833eee0bf2cc5aaaab8
prerequisite-patch-id: b835c6b7cdb7863c6f71d94f725f072fffdaa24a
prerequisite-patch-id: 117c9e973a140f08733823f076375012f7f0c285
prerequisite-patch-id: 2ed9cf40521d69baa52454f829bcd1d1da991892
prerequisite-patch-id: 7fa629dd32831d8e39db5f23ab2131a84ed097af
prerequisite-patch-id: b53bdb663aab3246d5ddecf18de1ae73e30fe6a5

[PATCH 1/6] fs: ext4: refuse to write with an unexpected first data block

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

ext4fs_blk_to_group() computes the block group of a block from
s_first_data_block, as Linux does. The rest of the write path takes the
first data block to be block 1 with 1 KiB blocks and block 0 otherwise,
which is what mke2fs makes: ext4fs_set_block_bmap() and
ext4fs_reset_block_bmap() find the bit of a block from the block number
alone, _get_new_blk_no() numbers the blocks of a group the same way, and
ext4fs_init() reads the group descriptor table from the block after it.

With any other s_first_data_block the two disagree, and the delete path
clears the bit of a block in the bitmap of another group. With 4 KiB
blocks and a first data block of 1, for example, block 32768 is in
group 0 for ext4fs_blk_to_group(), and ext4fs_reset_block_bmap() then
reads the byte just past the end of group 0's bitmap buffer and clears
a bit of it.

Add the first data block to the layout check of ext4fs_init(), so that
the write path refuses a filesystem whose first data block is not the
one it assumes, as it refuses other layouts its bitmap code cannot
handle. Linux refuses a first data block of 0 with 1 KiB blocks too,
unless clusters are larger.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 fs/ext4/ext4_write.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 0cbbdc6c934..11d9957de11 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -617,6 +617,7 @@ int ext4fs_init(void)
 	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);
+	uint32_t first_data_block = le32_to_cpu(sblock->first_data_block);
 
 	/* populate fs */
 	fs->blksz = EXT2_BLOCK_SIZE(ext4fs_root);
@@ -625,14 +626,20 @@ int ext4fs_init(void)
 	/*
 	 * 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
+	 * blocks, unless there is only one group. It also takes the first
+	 * data block, which holds the superblock, to be block 1 with 1 KiB
+	 * blocks and block 0 otherwise, as mke2fs makes it, and so does the
+	 * write path when it finds the group descriptor table; with any
+	 * other first data block, ext4fs_blk_to_group() puts a block in
+	 * another group than the bitmap code does. 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) ||
+	     le32_to_cpu(sblock->total_blocks) - first_data_block >
+	     blk_per_grp) ||
+	    first_data_block != (fs->blksz == EXT2_MIN_BLOCK_SIZE) ||
 	    !inode_per_grp || inode_per_grp > fs->blksz * 8) {
 		printf("Unsupported block group layout, not writing\n");
 		return -EINVAL;

[PATCH 2/6] fs: ext4: use ext4fs_blk_to_group() in ext4fs_get_new_blk_no()

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

ext4fs_get_new_blk_no() works out the block group of the next block to
allocate with its own arithmetic, which takes the first data block to
be block 1 with 1 KiB blocks and block 0 otherwise. Now that
ext4fs_init() refuses any other first data block, ext4fs_blk_to_group()
gives the same group, so use it, and step to the first block of the next
group from the first data block too.

Running past the last group means here that the filesystem is full, not
that a block number is corrupt, so first check quietly that the block
lies inside the filesystem, as ext4fs_get_new_inode_no() does for
inodes, and stop rather than skip past a full last group. The search for
the next block then ends at the end of the filesystem rather than at the
end of the last group, so it no longer takes a block past the end when
the padding bits of the last group's bitmap are clear.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 fs/ext4/ext4_common.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 273e6628b8d..ccfce5c745c 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -1037,10 +1037,11 @@ uint32_t ext4fs_get_new_blk_no(void)
 {
 	short i;
 	short status;
-	int remainder;
-	unsigned int bg_idx;
+	int bg_idx;
 	static int prev_bg_bitmap_index = -1;
 	unsigned int blk_per_grp = le32_to_cpu(ext4fs_root->sblock.blocks_per_group);
+	uint32_t first_data_block =
+		le32_to_cpu(ext4fs_root->sblock.first_data_block);
 	struct ext_filesystem *fs = get_fs();
 	char *journal_buffer = zalloc(fs->blksz);
 	char *zero_buffer = zalloc(fs->blksz);
@@ -1092,28 +1093,27 @@ uint32_t ext4fs_get_new_blk_no(void)
 	} else {
 		fs->curr_blkno++;
 restart:
+		/* no free block left after the last one allocated */
+		if (fs->curr_blkno >=
+		    le32_to_cpu(ext4fs_root->sblock.total_blocks))
+			goto fail;
 		/* get the blockbitmap index respective to blockno */
-		bg_idx = fs->curr_blkno / blk_per_grp;
-		if (fs->blksz == 1024) {
-			remainder = fs->curr_blkno % blk_per_grp;
-			if (!remainder)
-				bg_idx--;
-		}
+		bg_idx = ext4fs_blk_to_group(fs, fs->curr_blkno);
+		if (bg_idx < 0)
+			goto fail;
 
 		/*
 		 * To skip completely filled block group bitmaps
 		 * Optimize the block allocation
 		 */
-		if (bg_idx >= fs->no_blkgrp)
-			goto fail;
-
 		struct ext2_block_group *bgd = NULL;
 		bgd = ext4fs_get_group_descriptor(fs, bg_idx);
 		if (ext4fs_bg_get_free_blocks(bgd, fs) == 0) {
-			debug("block group %u is full. Skipping\n", bg_idx);
-			fs->curr_blkno = (bg_idx + 1) * blk_per_grp;
-			if (fs->blksz == 1024)
-				fs->curr_blkno += 1;
+			debug("block group %d is full. Skipping\n", bg_idx);
+			if (bg_idx + 1 >= fs->no_blkgrp)
+				goto fail;
+			fs->curr_blkno = (bg_idx + 1) * blk_per_grp +
+					 first_data_block;
 			goto restart;
 		}
 
@@ -1129,7 +1129,7 @@ restart:
 
 		if (ext4fs_set_block_bmap(fs->curr_blkno, fs->blk_bmaps[bg_idx],
 				   bg_idx) != 0) {
-			debug("going for restart for the block no %ld %u\n",
+			debug("going for restart for the block no %ld %d\n",
 			      fs->curr_blkno, bg_idx);
 			fs->curr_blkno++;
 			goto restart;

[PATCH 3/6] fs: ext4: check directory entries before using them

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

ext4fs_update_parent_dentry() looks for room for a new name by walking
the entries of the parent directory's last block by their on-disk
rec_len. The walk only stops at an entry whose rec_len reaches exactly
to the end of the block, so an entry that claims a length past the end
of the block, or a misaligned or too short one, moves the walk out of
the block buffer: it reads entries from the memory after it, and the
new entry is then written there.

search_dir() and unlink_filename() check the alignment and the minimum
length of each entry since commit ecdfb4195b20 ("ext4: recover from
filesystem corruption when reading"), but not that the entry's name
fits in its rec_len and in the block.

An entry with a rec_len of 0 also ends the walk early, and the new
entry is then written over it.

Add ext4fs_get_dirent(), which returns an entry of a directory block
only if it is aligned, lies inside the block and its rec_len covers its
header and name, much as Linux checks in ext4_check_dir_entry(), and
use it for every entry the three functions walk. search_dir() and
unlink_filename() stop reading a corrupt block, as they did at a
misaligned entry, and ext4fs_update_parent_dentry() now refuses to
write to one. A block it has just added to the directory has no entry
yet, so it is not walked.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 fs/ext4/ext4_common.c | 90 ++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index ccfce5c745c..9c41d2f60fc 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -443,6 +443,37 @@ uint16_t ext4fs_checksum_update(uint32_t i)
 	return crc;
 }
 
+/**
+ * ext4fs_get_dirent() - get a directory entry of a directory block
+ *
+ * Check that the entry at @offset lies inside the block and that its length
+ * covers its header and name, as Linux does in ext4_check_dir_entry(), so
+ * that the entry can be used and the next one found at @offset plus its
+ * length.
+ *
+ * @block:	directory block
+ * @offset:	offset of the entry in @block
+ * @blksz:	size of @block
+ * Return:	the entry, or NULL if it is not valid
+ */
+static struct ext2_dirent *ext4fs_get_dirent(char *block, unsigned int offset,
+					     unsigned int blksz)
+{
+	struct ext2_dirent *dir;
+	unsigned int direntlen;
+
+	if ((offset & 3) || offset > blksz - sizeof(*dir))
+		return NULL;
+
+	dir = (struct ext2_dirent *)(block + offset);
+	direntlen = le16_to_cpu(dir->direntlen);
+	if (direntlen < sizeof(*dir) + dir->namelen ||
+	    direntlen > blksz - offset)
+		return NULL;
+
+	return dir;
+}
+
 static int check_void_in_dentry(struct ext2_dirent *dir, char *filename)
 {
 	int dentry_length;
@@ -493,6 +524,7 @@ int ext4fs_update_parent_dentry(char *filename, int file_type)
 	uint32_t new_size;
 	uint32_t new_blockcnt;
 	uint32_t directory_blocks;
+	bool new_blk = false;
 
 	zero_buffer = zalloc(fs->blksz);
 	if (!zero_buffer) {
@@ -530,16 +562,25 @@ restart_read:
 	dir = (struct ext2_dirent *)root_first_block_buffer;
 	totalbytes = 0;
 
-	while (le16_to_cpu(dir->direntlen) > 0) {
-		unsigned short used_len = ROUND(dir->namelen +
-		    sizeof(struct ext2_dirent), 4);
+	/* a block just added to the directory has no entry yet */
+	while (!new_blk) {
+		unsigned short used_len;
+		int direntlen;
+
+		dir = ext4fs_get_dirent(root_first_block_buffer, totalbytes,
+					fs->blksz);
+		if (!dir) {
+			printf("Corrupt directory block, not writing\n");
+			goto fail;
+		}
+		direntlen = le16_to_cpu(dir->direntlen);
+		used_len = ROUND(dir->namelen + sizeof(struct ext2_dirent), 4);
 
 		/* last entry of block */
-		if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen)) {
+		if (fs->blksz - totalbytes == direntlen) {
 
 			/* check if new entry fits */
-			if ((used_len + new_entry_byte_reqd) <=
-			    le16_to_cpu(dir->direntlen)) {
+			if ((used_len + new_entry_byte_reqd) <= direntlen) {
 				dir->direntlen = cpu_to_le16(used_len);
 				break;
 			} else {
@@ -581,17 +622,15 @@ restart_read:
 				    (root_first_block_buffer,
 				     first_block_no_of_root))
 					goto fail;
+				new_blk = true;
 				goto restart;
 			}
 		}
 
-		templength = le16_to_cpu(dir->direntlen);
-		totalbytes = totalbytes + templength;
+		totalbytes = totalbytes + direntlen;
 		sizeof_void_space = check_void_in_dentry(dir, filename);
 		if (sizeof_void_space)
 			break;
-
-		dir = (struct ext2_dirent *)((char *)dir + templength);
 	}
 
 	/* make a pointer ready for creating next directory entry */
@@ -663,17 +702,13 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 
 		offset = 0;
 		do {
-			if (offset & 3) {
-				printf("Badly aligned ext2_dirent\n");
+			dir = ext4fs_get_dirent(block_buffer, offset,
+						fs->blksz);
+			if (!dir) {
+				printf("Corrupt directory block\n");
 				break;
 			}
-
-			dir = (struct ext2_dirent *)(block_buffer + offset);
-			direntname = (char*)(dir) + sizeof(struct ext2_dirent);
-
-			int direntlen = le16_to_cpu(dir->direntlen);
-			if (direntlen < sizeof(struct ext2_dirent))
-				break;
+			direntname = (char *)(dir) + sizeof(struct ext2_dirent);
 
 			if (dir->inode && (strlen(dirname) == dir->namelen) &&
 			    (strncmp(dirname, direntname, dir->namelen) == 0)) {
@@ -681,7 +716,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 				break;
 			}
 
-			offset += direntlen;
+			offset += le16_to_cpu(dir->direntlen);
 
 		} while (offset < fs->blksz);
 
@@ -892,26 +927,21 @@ static int unlink_filename(char *filename, unsigned int blknr)
 
 	offset = 0;
 	do {
-		if (offset & 3) {
-			printf("Badly aligned ext2_dirent\n");
+		previous_dir = dir;
+		dir = ext4fs_get_dirent(block_buffer, offset, fs->blksz);
+		if (!dir) {
+			printf("Corrupt directory block\n");
 			break;
 		}
-
-		previous_dir = dir;
-		dir = (struct ext2_dirent *)(block_buffer + offset);
 		direntname = (char *)(dir) + sizeof(struct ext2_dirent);
 
-		int direntlen = le16_to_cpu(dir->direntlen);
-		if (direntlen < sizeof(struct ext2_dirent))
-			break;
-
 		if (dir->inode && (strlen(filename) == dir->namelen) &&
 		    (strncmp(direntname, filename, dir->namelen) == 0)) {
 			inodeno = le32_to_cpu(dir->inode);
 			break;
 		}
 
-		offset += direntlen;
+		offset += le16_to_cpu(dir->direntlen);
 
 	} while (offset < fs->blksz);
 

[PATCH 4/6] fs: ext4: bound the journal entry tables

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

A write logs each metadata block it changes in journal_ptr[] and
dirty_block_ptr[], which hold CONFIG_EXT4_MAX_JOURNAL_ENTRIES entries
each. ext4fs_log_journal() and ext4fs_put_metadata() add an entry
without checking for room, so a write that changes more blocks than
that, for example on a filesystem with many block groups whose block
bitmaps it touches, writes past the end of both tables.

Commit 29cb98d515f9 ("fs: ext4fs: Fix: Data abort in ext4fs_log_gdt()")
added a check to ext4fs_log_gdt(), but it compares the number of group
descriptor blocks with the size of the whole table, while
ext4fs_init_journal() has already used one entry for the superblock.

update_descriptor_block() also writes one tag per logged block into a
single journal block, which holds (blksz - 12) / 8 tags: 126 with 1 KiB
blocks. A larger CONFIG_EXT4_MAX_JOURNAL_ENTRIES lets it write past the
end of that block buffer.

Add ext4fs_journal_slots(), the number of entries a transaction may
use, which is the smaller of the table size and the number of tags in a
descriptor block, and check it in ext4fs_log_gdt(), counting the
entries already in use, and in ext4fs_log_journal(). Check the size of
dirty_block_ptr[] in ext4fs_put_metadata(). These return -ENOMEM, and
print their message with printf(), as the rest of the driver does. A
write that needs more entries now fails before its metadata changes
are written.

With the default of 100 entries this does not change what a write can
do: a typical ext4write logs a handful of blocks, and the limit is only
reached on filesystems with very many block groups, or by a write that
allocates blocks in very many of them.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 fs/ext4/ext4_journal.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4_journal.c b/fs/ext4/ext4_journal.c
index 3a2e9f30c12..cb63addea0a 100644
--- a/fs/ext4/ext4_journal.c
+++ b/fs/ext4/ext4_journal.c
@@ -126,15 +126,38 @@ void ext4fs_free_journal(void)
 	jrnl_blk_idx = 1;
 }
 
+/**
+ * ext4fs_journal_slots() - get the number of blocks one transaction can log
+ *
+ * update_descriptor_block() writes a tag for each logged block into a single
+ * descriptor block, so a transaction can log no more blocks than there are
+ * tags in a block, nor more than journal_ptr[] holds.
+ *
+ * Return:	the number of journal_ptr[] entries that may be used
+ */
+static int ext4fs_journal_slots(void)
+{
+	struct ext_filesystem *fs = get_fs();
+	int tags = (fs->blksz - sizeof(struct journal_header_t)) /
+		   sizeof(struct ext3_journal_block_tag);
+
+	return min(tags, MAX_JOURNAL_ENTRIES);
+}
+
 int ext4fs_log_gdt(char *gd_table)
 {
 	struct ext_filesystem *fs = get_fs();
-	short i;
+	u32 i;
 	long int var = fs->gdtable_blkno;
 
-	/* Make sure there is enough journal entries */
-	if (fs->no_blk_pergdt > MAX_JOURNAL_ENTRIES) {
-		log_err("*** Not enough journal entries allocated\n");
+	/*
+	 * Make sure there are enough journal entries left:
+	 * ext4fs_init_journal() has already taken the slots for the
+	 * superblock, so count from gindex rather than from the start of the
+	 * table.
+	 */
+	if (gindex + fs->no_blk_pergdt > ext4fs_journal_slots()) {
+		printf("*** Not enough journal entries allocated\n");
 		return -ENOMEM;
 	}
 
@@ -172,6 +195,11 @@ int ext4fs_log_journal(char *journal_buffer, uint32_t blknr)
 			return 0;
 	}
 
+	if (gindex >= ext4fs_journal_slots()) {
+		printf("*** Not enough journal entries allocated\n");
+		return -ENOMEM;
+	}
+
 	journal_ptr[gindex]->buf = zalloc(fs->blksz);
 	if (!journal_ptr[gindex]->buf)
 		return -ENOMEM;
@@ -194,6 +222,10 @@ int ext4fs_put_metadata(char *metadata_buffer, uint32_t blknr)
 		printf("Invalid input arguments %s\n", __func__);
 		return -EINVAL;
 	}
+	if (gd_index >= MAX_JOURNAL_ENTRIES) {
+		printf("*** Not enough journal entries allocated\n");
+		return -ENOMEM;
+	}
 	if (dirty_block_ptr[gd_index]->buf)
 		assert(dirty_block_ptr[gd_index]->blknr == blknr);
 	else

[PATCH 5/6] fs: ext4: stop the journal scan at the end of the log

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Before every write, ext4fs_check_journal_state() scans the journal of a
filesystem that needs recovery for the transactions to replay. It
follows the log block by block and decides what each block is from its
h_blocktype alone:

- a block of an unknown type with the current sequence number does not
  advance the scan, so the scan never ends
- the h_magic of a block is not checked, so a block that is not a
  journal block is taken as one when its type and sequence words match
- the scan is not bounded by the length of the journal, s_maxlen, nor
  stopped at a block the journal inode does not map
- a revoke block is kept with its r_count unchecked, and
  check_blknr_for_revoke() then reads r_count bytes of records from a
  copy of one block

End the log at a block without the journal magic or of an unknown
type, as Linux's jbd2 does, and at a revoke block whose r_count is
larger than the block, which jbd2's scan_revoke_records() rejects. Also
end it at a block the journal inode does not map and at the end of the
journal: the scan does not follow a log that wraps around the end of
the journal, so it now ends there instead of reading past it.

Ending the log keeps the transactions committed before the block that
ends it, which are replayed. Failing the scan instead would make
ext4fs_init() fail, and its ext4fs_deinit() would mark the journal
clean without replaying any of them.

Remove print_revoke_blks(), which has no caller and walks a revoke
block by its r_count in the same way.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 fs/ext4/ext4_journal.c | 65 +++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/fs/ext4/ext4_journal.c b/fs/ext4/ext4_journal.c
index cb63addea0a..c2c022d6c4f 100644
--- a/fs/ext4/ext4_journal.c
+++ b/fs/ext4/ext4_journal.c
@@ -239,28 +239,6 @@ int ext4fs_put_metadata(char *metadata_buffer, uint32_t blknr)
 	return 0;
 }
 
-void print_revoke_blks(char *revk_blk)
-{
-	int offset;
-	int max;
-	long int blocknr;
-	struct journal_revoke_header_t *header;
-
-	if (revk_blk == NULL)
-		return;
-
-	header = (struct journal_revoke_header_t *) revk_blk;
-	offset = sizeof(struct journal_revoke_header_t);
-	max = be32_to_cpu(header->r_count);
-	printf("total bytes %d\n", max);
-
-	while (offset < max) {
-		blocknr = be32_to_cpu(*((__be32 *)(revk_blk + offset)));
-		printf("revoke blknr is %ld\n", blocknr);
-		offset += 4;
-	}
-}
-
 static struct revoke_blk_list *_get_node(void)
 {
 	struct revoke_blk_list *tmp_node;
@@ -430,7 +408,7 @@ void print_jrnl_status(int recovery_flag)
 
 int ext4fs_check_journal_state(int recovery_flag)
 {
-	int i;
+	unsigned int i, maxlen;
 	int DB_FOUND = NO;
 	long int blknr;
 	int transaction_state = TRANSACTION_COMPLETE;
@@ -440,6 +418,7 @@ int ext4fs_check_journal_state(int recovery_flag)
 	struct ext2_inode inode_journal;
 	struct journal_superblock_t *jsb = NULL;
 	struct journal_header_t *jdb = NULL;
+	struct journal_revoke_header_t *revoke;
 	char *p_jdb = NULL;
 	struct ext3_journal_block_tag *tag = NULL;
 	char *temp_buff = NULL;
@@ -482,14 +461,35 @@ int ext4fs_check_journal_state(int recovery_flag)
 		jsb->s_feature_compat |=
 				cpu_to_be32(JBD2_FEATURE_COMPAT_CHECKSUM);
 
+	/*
+	 * The scan does not follow a log that wraps around the end of the
+	 * journal, so it ends there. Keep the block indexes in the range
+	 * read_allocated_block() takes.
+	 */
+	maxlen = min_t(u32, be32_to_cpu(jsb->s_maxlen), INT_MAX);
 	i = be32_to_cpu(jsb->s_first);
 	while (1) {
+		if (i >= maxlen) {
+			print_jrnl_status(recovery_flag);
+			break;
+		}
+
 		blknr = read_allocated_block(&inode_journal, i, NULL);
+		if (blknr <= 0) {
+			print_jrnl_status(recovery_flag);
+			break;
+		}
 		memset(temp_buff1, '\0', fs->blksz);
 		ext4fs_devread((lbaint_t)blknr * fs->sect_perblk,
 			       0, fs->blksz, temp_buff1);
 		jdb = (struct journal_header_t *) temp_buff1;
 
+		/* A block that is not a journal block ends the log */
+		if (jdb->h_magic != cpu_to_be32(EXT3_JOURNAL_MAGIC_NUMBER)) {
+			print_jrnl_status(recovery_flag);
+			break;
+		}
+
 		if (be32_to_cpu(jdb->h_blocktype) ==
 		    EXT3_JOURNAL_DESCRIPTOR_BLOCK) {
 			if (be32_to_cpu(jdb->h_sequence) !=
@@ -547,16 +547,23 @@ int ext4fs_check_journal_state(int recovery_flag)
 				print_jrnl_status(recovery_flag);
 				break;
 			}
+			/*
+			 * check_blknr_for_revoke() walks the records up to
+			 * r_count bytes, which must lie inside the block
+			 */
+			revoke = (struct journal_revoke_header_t *)jdb;
+			if (be32_to_cpu(revoke->r_count) > fs->blksz) {
+				printf("Invalid revoke block\n");
+				print_jrnl_status(recovery_flag);
+				break;
+			}
 			if (recovery_flag == SCAN)
 				ext4fs_push_revoke_blk((char *)jdb);
 			i++;
 		} else {
-			debug("Else Case\n");
-			if (be32_to_cpu(jdb->h_sequence) !=
-			    be32_to_cpu(jsb->s_sequence)) {
-				print_jrnl_status(recovery_flag);
-				break;
-			}
+			/* An unknown block type ends the log, as for Linux */
+			print_jrnl_status(recovery_flag);
+			break;
 		}
 	}
 

[PATCH 6/6] test: fs: ext4: add tests for the write path checks

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Add three cases to test_ext_oob.py:

- test_fs_ext_oob_first_data_block: a filesystem of two block groups of
  4 KiB blocks whose first data block is 1 must not be written to, so
  that replacing a file whose block is the first of group 1 does not
  clear its bit in group 0's bitmap
- test_fs_ext_oob_parent_dentry: writing a new file into a directory
  whose last entry claims a length past the end of its block must fail
  with "Corrupt directory block, not writing" rather than walk past the
  block
- test_fs_ext_oob_journal_block_type: a journal that needs recovery and
  holds a block of an unknown type with the current sequence number
  must not stall the journal scan; the journal must be recovered and
  the write must go through

ext4_image() now takes the size of the image, as the first case needs
two block groups.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 test/py/tests/test_fs/test_ext_oob.py | 115 +++++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 3 deletions(-)

diff --git a/test/py/tests/test_fs/test_ext_oob.py b/test/py/tests/test_fs/test_ext_oob.py
index 12f4095c6e5..67eb5ea060f 100644
--- a/test/py/tests/test_fs/test_ext_oob.py
+++ b/test/py/tests/test_fs/test_ext_oob.py
@@ -21,8 +21,8 @@ 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
+def ext4_image(ubman, prefix, opts=(), size=8 << 20):
+    """Make an 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
@@ -32,13 +32,14 @@ def ext4_image(ubman, prefix, opts=()):
         ubman (ConsoleBase): U-Boot console
         prefix (str): prefix of the image file name
         opts (list of str): extra options for mkfs.ext4
+        size (int): size of the image in bytes
 
     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)
+        fil.truncate(size)
     try:
         # U-Boot does not write to a filesystem with metadata checksums
         run(['mkfs.ext4', '-q', '-F', '-O', '^metadata_csum', *opts, img],
@@ -308,3 +309,111 @@ def test_fs_ext_oob_last_inode(ubman):
         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)
+
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_ext4_write')
+@pytest.mark.requiredtool('debugfs')
+def test_fs_ext_oob_first_data_block(ubman):
+    """ext4fs_init(): a filesystem whose first data block is not the one the
+    bitmap code assumes must not be written to"""
+    # Two block groups of 32768 4 KiB blocks. With a first data block of 1,
+    # block 32768, the first block of group 1, lies in group 0.
+    with ext4_image(ubman, 'ext_oob_fdb', ['-b', '4096'], 136 << 20) as img:
+        # s_blocks_count and s_blocks_per_group are at offsets 4 and 32 of
+        # the superblock
+        assert struct.unpack('<I', read_at(img, 1024 + 4, 4))[0] > 32768
+        assert struct.unpack('<I', read_at(img, 1024 + 32, 4))[0] == 32768
+        add_file(img, 'victim', {0: VICTIM})
+        # Make /victim a block-mapped file whose only block is 32768
+        cmds = ['sif victim flags 0', 'sif victim size 4096']
+        cmds += [f'sif victim block[{i}] 0' for i in range(1, 12)]
+        cmds += ['sif victim block[0] 32768']
+        debugfs(img, cmds)
+        assert victim_has(img, '(0):32768')
+
+        # s_first_data_block is at offset 20 of the superblock. The read
+        # path looks for the group descriptors in the block after it, so
+        # copy them from block 1 to block 2, a reserved GDT block.
+        write_at(img, 1024 + 20, struct.pack('<I', 1))
+        write_at(img, 2 * 4096, read_at(img, 4096, 4096))
+
+        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 '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_parent_dentry(ubman):
+    """ext4fs_update_parent_dentry(): a directory block whose last entry
+    claims a length past the end of the block must be rejected rather than
+    walked past"""
+    with ext4_image(ubman, 'ext_oob_den') as img:
+        blksz = block_size(img)
+        blk = file_block(img, '/', 0)
+
+        # Rewrite the root directory block: '.' and '..', then entries with
+        # no free space after their names, then a last entry that ends 4
+        # bytes past the block with too little free space for the new name
+        data = bytearray(blksz)
+        off = 0
+        entries = [(2, 12, b'.'), (2, 12, b'..')]
+        entries += [(0, 12, b'fill')] * ((blksz - 16 - 24) // 12)
+        for inode, rec_len, name in entries:
+            struct.pack_into('<IHBB', data, off, inode, rec_len, len(name), 2)
+            data[off + 8:off + 8 + len(name)] = name
+            off += rec_len
+        rec_len = blksz - off + 4
+        name = b'l' * (rec_len - 12)
+        struct.pack_into('<IHBB', data, off, 0, rec_len, len(name), 2)
+        data[off + 8:blksz] = name[:blksz - off - 8]
+        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} /newfile 0x1000')
+        assert 'Corrupt directory block, not writing' 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_journal_block_type(ubman):
+    """ext4fs_check_journal_state(): a journal block of an unknown type must
+    end the journal scan rather than stall it"""
+    with ext4_image(ubman, 'ext_oob_jnl') as img:
+        blksz = block_size(img)
+
+        # The journal superblock holds (big endian) magic, type, sequence,
+        # block size, maxlen, first, sequence and start
+        jsb = file_block(img, '<8>', 0) * blksz
+        magic, _, _, _, _, first, seq = struct.unpack('>7I',
+                                                      read_at(img, jsb, 28))
+        assert magic == 0xc03b3998
+
+        # Mark the journal as needing recovery from its first log block,
+        # which holds a block of an unknown type with the current sequence
+        write_at(img, jsb + 28, struct.pack('>I', first))
+        write_at(img, file_block(img, '<8>', first) * blksz,
+                 struct.pack('>3I', magic, 7, seq))
+        # s_feature_incompat is at offset 0x60 of the superblock; 4 is
+        # EXT3_FEATURE_INCOMPAT_RECOVER
+        incompat = struct.unpack('<I', read_at(img, 1024 + 0x60, 4))[0]
+        write_at(img, 1024 + 0x60, struct.pack('<I', incompat | 4))
+
+        ubman.run_command(f'host bind 0 {img}')
+        ubman.run_command(f'mw.b {ADDR:x} 41 1000')
+        with ubman.temporary_timeout(10000):
+            out = ubman.run_command(
+                f'ext4write host 0:0 {ADDR:x} /newfile 0x1000')
+        assert 'Journal Recovery Completed' in out
+        assert 'written' in out
+        assert 'alive' in ubman.run_command('echo alive')