[PATCH 0/7] Fix various bugs found via a fuzzing campaign

[PATCH 0/7] Fix various bugs found via a fuzzing campaign

[Theodore Ts'o]

Theodore Ts'o (7):
  e2fsck: sanity check the journal inode number
  e2fsck: fix potential out-of-bounds read in inc_ea_inode_refs()
  libext2fs: add check for too-short directory blocks
  e2fsck: check for xattr value size integer wraparound
  e2fsck: avoid out-of-bounds write for very deep extent trees
  libext2fs: check for cyclic loops in the extent tree
  libext2fs: check for invalid blocks in ext2fs_punch_blocks()

 e2fsck/extents.c           | 10 +++++++++-
 e2fsck/journal.c           |  9 ++++++++-
 e2fsck/pass1.c             | 21 +++++++++++++--------
 lib/ext2fs/alloc_stats.c   |  3 ++-
 lib/ext2fs/dir_iterate.c   |  4 ++++
 lib/ext2fs/ext2_err.et.in  |  3 +++
 lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
 lib/ext2fs/extent.c        | 11 +++++++++--
 lib/ext2fs/punch.c         |  4 ++++
 9 files changed, 63 insertions(+), 13 deletions(-)

-- 
2.31.0

[PATCH 1/7] e2fsck: sanity check the journal inode number

[Theodore Ts'o]

E2fsck replays the journal before sanity checking the full superblock.
So it's possible that the journal inode number is not valid relative
to the number of block groups.  So to avoid potentially an array
bounds overrun, sanity check this before trying to find the journal
inode.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/journal.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 2e867234..12487e3d 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -989,7 +989,14 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 	journal->j_blocksize = ctx->fs->blocksize;
 
 	if (uuid_is_null(sb->s_journal_uuid)) {
-		if (!sb->s_journal_inum) {
+		/*
+		 * The full set of superblock sanity checks haven't
+		 * been performed yet, so we need to do some basic
+		 * checks here to avoid potential array overruns.
+		 */
+		if (!sb->s_journal_inum ||
+		    (sb->s_journal_inum >
+		     (ctx->fs->group_desc_count * sb->s_inodes_per_group))) {
 			retval = EXT2_ET_BAD_INODE_NUM;
 			goto errout;
 		}
-- 
2.31.0

[PATCH 2/7] e2fsck: fix potential out-of-bounds read in inc_ea_inode_refs()

[Theodore Ts'o]

If there isn't enough space for a full extended attribute entry,
inc_ea_inode_refs() might end up reading beyond the allocated memory
buffer.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/pass1.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index dde862a8..2a17bb8a 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -389,13 +389,13 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
 static void inc_ea_inode_refs(e2fsck_t ctx, struct problem_context *pctx,
 			      struct ext2_ext_attr_entry *first, void *end)
 {
-	struct ext2_ext_attr_entry *entry;
+	struct ext2_ext_attr_entry *entry = first;
+	struct ext2_ext_attr_entry *np = EXT2_EXT_ATTR_NEXT(entry);
 
-	for (entry = first;
-	     (void *)entry < end && !EXT2_EXT_IS_LAST_ENTRY(entry);
-	     entry = EXT2_EXT_ATTR_NEXT(entry)) {
+	while ((void *) entry < end && (void *) np < end &&
+	       !EXT2_EXT_IS_LAST_ENTRY(entry)) {
 		if (!entry->e_value_inum)
-			continue;
+			goto next;
 		if (!ctx->ea_inode_refs) {
 			pctx->errcode = ea_refcount_create(0,
 							   &ctx->ea_inode_refs);
@@ -408,6 +408,9 @@ static void inc_ea_inode_refs(e2fsck_t ctx, struct problem_context *pctx,
 		}
 		ea_refcount_increment(ctx->ea_inode_refs, entry->e_value_inum,
 				      0);
+	next:
+		entry = np;
+		np = EXT2_EXT_ATTR_NEXT(entry);
 	}
 }
 
-- 
2.31.0

[PATCH 3/7] libext2fs: add check for too-short directory blocks

[Theodore Ts'o]

If there is an inline data directory which is smaller than 8 bytes
(which should never happen but for corrupted or fuzzed file systems),
ext2fs_process_dir_block() will now abort EXT2_ET_DIR_CORRUPTED to
avoid an out-of-bounds read.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 lib/ext2fs/dir_iterate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/ext2fs/dir_iterate.c b/lib/ext2fs/dir_iterate.c
index b2b77693..7798a482 100644
--- a/lib/ext2fs/dir_iterate.c
+++ b/lib/ext2fs/dir_iterate.c
@@ -221,6 +221,10 @@ int ext2fs_process_dir_block(ext2_filsys fs,
 	if (ext2fs_has_feature_metadata_csum(fs->super))
 		csum_size = sizeof(struct ext2_dir_entry_tail);
 
+	if (buflen < 8) {
+		ctx->errcode = EXT2_ET_DIR_CORRUPTED;
+		return BLOCK_ABORT;
+	}
 	while (offset < buflen - 8) {
 		dirent = (struct ext2_dir_entry *) (ctx->buf + offset);
 		if (ext2fs_get_rec_len(fs, dirent, &rec_len))
-- 
2.31.0

[PATCH 4/7] e2fsck: check for xattr value size integer wraparound

[Theodore Ts'o]

When checking an extended attrbiute block for correctness, we check if
the starting offset plus the value size exceeds the end of the block.
However, we weren't checking if the size was too large, and if it is
so large that it triggers a wraparound when we added the starting
offset, we won't notice the problem.  Add the missing check.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/pass1.c             |  5 +++--
 lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 2a17bb8a..11d7ce93 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -2556,8 +2556,9 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 			break;
 		}
 		if (entry->e_value_inum == 0) {
-			if (entry->e_value_offs + entry->e_value_size >
-			    fs->blocksize) {
+			if (entry->e_value_size > EXT2_XATTR_SIZE_MAX ||
+			    (entry->e_value_offs + entry->e_value_size >
+			     fs->blocksize)) {
 				if (fix_problem(ctx, PR_1_EA_BAD_VALUE, pctx))
 					goto clear_extattr;
 				break;
diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
index f2042ed5..c6068c48 100644
--- a/lib/ext2fs/ext2_ext_attr.h
+++ b/lib/ext2fs/ext2_ext_attr.h
@@ -57,6 +57,17 @@ struct ext2_ext_attr_entry {
 #define EXT2_XATTR_SIZE(size) \
 	(((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
 
+/*
+ * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
+ * for file system consistency errors, we use a somewhat bigger value.
+ * This allows XATTR_SIZE_MAX to grow in the future, but by using this
+ * instead of INT_MAX for certain consistency checks, we don't need to
+ * worry about arithmetic overflows.  (Actually XATTR_SIZE_MAX is
+ * defined in include/uapi/linux/limits.h, so changing it is going
+ * not going to be trivial....)
+ */
+#define EXT2_XATTR_SIZE_MAX (1 << 24)
+
 #ifdef __KERNEL__
 # ifdef CONFIG_EXT2_FS_EXT_ATTR
 extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
-- 
2.31.0

[PATCH 5/7] e2fsck: avoid out-of-bounds write for very deep extent trees

[Theodore Ts'o]

The kernel doesn't support extent trees deeper than 5
(EXT4_MAX_EXTENT_DEPTH).  For this reason we only maintain the extent
tree statistics for 5 levels.  Avoid out-of-bounds writes and reads if
the extent tree is deeper than this.

We keep these statistics to determine whether we should rebuild the
extent tree.  If the extent tree is too deep, we don't need the
statistics because we should always rebuild the it.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/extents.c | 10 +++++++++-
 e2fsck/pass1.c   |  3 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index 01879f56..86fe00e7 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -526,7 +526,8 @@ errcode_t e2fsck_check_rebuild_extents(e2fsck_t ctx, ext2_ino_t ino,
 		 */
 		if (info.curr_entry == 1 &&
 		    !(extent.e_flags & EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
-		    !eti.force_rebuild) {
+		    !eti.force_rebuild &&
+		    info.curr_level < MAX_EXTENT_DEPTH_COUNT) {
 			struct extent_tree_level *etl;
 
 			etl = eti.ext_info + info.curr_level;
@@ -580,6 +581,13 @@ errcode_t e2fsck_should_rebuild_extents(e2fsck_t ctx,
 	extents_per_block = (ctx->fs->blocksize -
 			     sizeof(struct ext3_extent_header)) /
 			    sizeof(struct ext3_extent);
+
+	/* If the extent tree is too deep, then rebuild it. */
+	if (info->max_depth > MAX_EXTENT_DEPTH_COUNT) {
+		pctx->blk = info->max_depth;
+		op = PR_1E_CAN_COLLAPSE_EXTENT_TREE;
+		goto rebuild;
+	}
 	/*
 	 * If we can consolidate a level or shorten the tree, schedule the
 	 * extent tree to be rebuilt.
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 11d7ce93..43972e7c 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -2842,7 +2842,8 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
 	if (pctx->errcode)
 		return;
 	if (!(ctx->options & E2F_OPT_FIXES_ONLY) &&
-	    !pb->eti.force_rebuild) {
+	    !pb->eti.force_rebuild &&
+	    info.curr_level < MAX_EXTENT_DEPTH_COUNT) {
 		struct extent_tree_level *etl;
 
 		etl = pb->eti.ext_info + info.curr_level;
-- 
2.31.0

[PATCH 6/7] libext2fs: check for cyclic loops in the extent tree

[Theodore Ts'o]

In the extent tree handling code in libext2fs, when we go move down
the extent tree, if a cyclic loop is detected, return an error.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 lib/ext2fs/ext2_err.et.in |  3 +++
 lib/ext2fs/extent.c       | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index cf0e00ea..bb1dcf14 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -551,4 +551,7 @@ ec	EXT2_ET_NO_GDESC,
 ec	EXT2_FILSYS_CORRUPTED,
 	"The internal ext2_filsys data structure appears to be corrupted"
 
+ec	EXT2_ET_EXTENT_CYCLE,
+	"Found cyclic loop in extent tree"
+
 	end
diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 1a206a16..82e75ccd 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -47,6 +47,7 @@ struct extent_path {
 	int		visit_num;
 	int		flags;
 	blk64_t		end_blk;
+	blk64_t		blk;
 	void		*curr;
 };
 
@@ -286,6 +287,7 @@ errcode_t ext2fs_extent_open2(ext2_filsys fs, ext2_ino_t ino,
 	handle->path[0].end_blk =
 		(EXT2_I_SIZE(handle->inode) + fs->blocksize - 1) >>
 		 EXT2_BLOCK_SIZE_BITS(fs->super);
+	handle->path[0].blk = 0;
 	handle->path[0].visit_num = 1;
 	handle->level = 0;
 	handle->magic = EXT2_ET_MAGIC_EXTENT_HANDLE;
@@ -305,14 +307,14 @@ errout:
 errcode_t ext2fs_extent_get(ext2_extent_handle_t handle,
 			    int flags, struct ext2fs_extent *extent)
 {
-	struct extent_path	*path, *newpath;
+	struct extent_path	*path, *newpath, *tp;
 	struct ext3_extent_header	*eh;
 	struct ext3_extent_idx		*ix = 0;
 	struct ext3_extent		*ex;
 	errcode_t			retval;
 	blk64_t				blk;
 	blk64_t				end_blk;
-	int				orig_op, op;
+	int				orig_op, op, l;
 	int				failed_csum = 0;
 
 	EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EXTENT_HANDLE);
@@ -467,6 +469,11 @@ retry:
 		}
 		blk = ext2fs_le32_to_cpu(ix->ei_leaf) +
 			((__u64) ext2fs_le16_to_cpu(ix->ei_leaf_hi) << 32);
+		for (l = handle->level, tp = path; l > 0; l--, tp--) {
+			if (blk == tp->blk)
+				return EXT2_ET_EXTENT_CYCLE;
+		}
+		newpath->blk = blk;
 		if ((handle->fs->flags & EXT2_FLAG_IMAGE_FILE) &&
 		    (handle->fs->io != handle->fs->image_io))
 			memset(newpath->buf, 0, handle->fs->blocksize);
-- 
2.31.0

[PATCH 7/7] libext2fs: check for invalid blocks in ext2fs_punch_blocks()

[Theodore Ts'o]

If the extent tree has out-of-range physical block numbers, don't try
to release them.

Also add a similar check in ext2fs_block_alloc_stats2() to avoid a
NULL pointer dereference.

Reported-by: Nils Bars <nils.bars@rub.de>
Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
Reported-by: Nico Schiller <nico.schiller@rub.de>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 lib/ext2fs/alloc_stats.c | 3 ++-
 lib/ext2fs/punch.c       | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
index 3949f618..6f98bcc7 100644
--- a/lib/ext2fs/alloc_stats.c
+++ b/lib/ext2fs/alloc_stats.c
@@ -62,7 +62,8 @@ void ext2fs_block_alloc_stats2(ext2_filsys fs, blk64_t blk, int inuse)
 {
 	int	group = ext2fs_group_of_blk2(fs, blk);
 
-	if (blk >= ext2fs_blocks_count(fs->super)) {
+	if (blk < fs->super->s_first_data_block ||
+	    blk >= ext2fs_blocks_count(fs->super)) {
 #ifndef OMIT_COM_ERR
 		com_err("ext2fs_block_alloc_stats", 0,
 			"Illegal block number: %lu", (unsigned long) blk);
diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
index effa1e2d..e2543e1e 100644
--- a/lib/ext2fs/punch.c
+++ b/lib/ext2fs/punch.c
@@ -200,6 +200,10 @@ static errcode_t punch_extent_blocks(ext2_filsys fs, ext2_ino_t ino,
 	__u32		cluster_freed;
 	errcode_t	retval = 0;
 
+	if (free_start < fs->super->s_first_data_block ||
+	    (free_start + free_count) >= ext2fs_blocks_count(fs->super))
+		return EXT2_ET_BAD_BLOCK_NUM;
+
 	/* No bigalloc?  Just free each block. */
 	if (EXT2FS_CLUSTER_RATIO(fs) == 1) {
 		*freed += free_count;
-- 
2.31.0

Re: [PATCH 1/7] e2fsck: sanity check the journal inode number

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:38AM -0400, Theodore Ts'o wrote:
> E2fsck replays the journal before sanity checking the full superblock.
> So it's possible that the journal inode number is not valid relative
> to the number of block groups.  So to avoid potentially an array
> bounds overrun, sanity check this before trying to find the journal
> inode.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/journal.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index 2e867234..12487e3d 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -989,7 +989,14 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
>  	journal->j_blocksize = ctx->fs->blocksize;
>  
>  	if (uuid_is_null(sb->s_journal_uuid)) {
> -		if (!sb->s_journal_inum) {
> +		/*
> +		 * The full set of superblock sanity checks haven't
> +		 * been performed yet, so we need to do some basic
> +		 * checks here to avoid potential array overruns.
> +		 */
> +		if (!sb->s_journal_inum ||
> +		    (sb->s_journal_inum >
> +		     (ctx->fs->group_desc_count * sb->s_inodes_per_group))) {
>  			retval = EXT2_ET_BAD_INODE_NUM;
>  			goto errout;
>  		}
> -- 
> 2.31.0
> 

Re: [PATCH 2/7] e2fsck: fix potential out-of-bounds read in inc_ea_inode_refs()

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:39AM -0400, Theodore Ts'o wrote:
> If there isn't enough space for a full extended attribute entry,
> inc_ea_inode_refs() might end up reading beyond the allocated memory
> buffer.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>


> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/pass1.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index dde862a8..2a17bb8a 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -389,13 +389,13 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
>  static void inc_ea_inode_refs(e2fsck_t ctx, struct problem_context *pctx,
>  			      struct ext2_ext_attr_entry *first, void *end)
>  {
> -	struct ext2_ext_attr_entry *entry;
> +	struct ext2_ext_attr_entry *entry = first;
> +	struct ext2_ext_attr_entry *np = EXT2_EXT_ATTR_NEXT(entry);
>  
> -	for (entry = first;
> -	     (void *)entry < end && !EXT2_EXT_IS_LAST_ENTRY(entry);
> -	     entry = EXT2_EXT_ATTR_NEXT(entry)) {
> +	while ((void *) entry < end && (void *) np < end &&
> +	       !EXT2_EXT_IS_LAST_ENTRY(entry)) {
>  		if (!entry->e_value_inum)
> -			continue;
> +			goto next;
>  		if (!ctx->ea_inode_refs) {
>  			pctx->errcode = ea_refcount_create(0,
>  							   &ctx->ea_inode_refs);
> @@ -408,6 +408,9 @@ static void inc_ea_inode_refs(e2fsck_t ctx, struct problem_context *pctx,
>  		}
>  		ea_refcount_increment(ctx->ea_inode_refs, entry->e_value_inum,
>  				      0);
> +	next:
> +		entry = np;
> +		np = EXT2_EXT_ATTR_NEXT(entry);
>  	}
>  }
>  
> -- 
> 2.31.0
> 

Re: [PATCH 3/7] libext2fs: add check for too-short directory blocks

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:40AM -0400, Theodore Ts'o wrote:
> If there is an inline data directory which is smaller than 8 bytes
> (which should never happen but for corrupted or fuzzed file systems),
> ext2fs_process_dir_block() will now abort EXT2_ET_DIR_CORRUPTED to
> avoid an out-of-bounds read.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  lib/ext2fs/dir_iterate.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/ext2fs/dir_iterate.c b/lib/ext2fs/dir_iterate.c
> index b2b77693..7798a482 100644
> --- a/lib/ext2fs/dir_iterate.c
> +++ b/lib/ext2fs/dir_iterate.c
> @@ -221,6 +221,10 @@ int ext2fs_process_dir_block(ext2_filsys fs,
>  	if (ext2fs_has_feature_metadata_csum(fs->super))
>  		csum_size = sizeof(struct ext2_dir_entry_tail);
>  
> +	if (buflen < 8) {
> +		ctx->errcode = EXT2_ET_DIR_CORRUPTED;
> +		return BLOCK_ABORT;
> +	}
>  	while (offset < buflen - 8) {
>  		dirent = (struct ext2_dir_entry *) (ctx->buf + offset);
>  		if (ext2fs_get_rec_len(fs, dirent, &rec_len))
> -- 
> 2.31.0
> 

Re: [PATCH 4/7] e2fsck: check for xattr value size integer wraparound

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:41AM -0400, Theodore Ts'o wrote:
> When checking an extended attrbiute block for correctness, we check if
> the starting offset plus the value size exceeds the end of the block.
> However, we weren't checking if the size was too large, and if it is
> so large that it triggers a wraparound when we added the starting
> offset, we won't notice the problem.  Add the missing check.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/pass1.c             |  5 +++--
>  lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 2a17bb8a..11d7ce93 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2556,8 +2556,9 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
>  			break;
>  		}
>  		if (entry->e_value_inum == 0) {
> -			if (entry->e_value_offs + entry->e_value_size >
> -			    fs->blocksize) {
> +			if (entry->e_value_size > EXT2_XATTR_SIZE_MAX ||
> +			    (entry->e_value_offs + entry->e_value_size >
> +			     fs->blocksize)) {
>  				if (fix_problem(ctx, PR_1_EA_BAD_VALUE, pctx))
>  					goto clear_extattr;
>  				break;
> diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
> index f2042ed5..c6068c48 100644
> --- a/lib/ext2fs/ext2_ext_attr.h
> +++ b/lib/ext2fs/ext2_ext_attr.h
> @@ -57,6 +57,17 @@ struct ext2_ext_attr_entry {
>  #define EXT2_XATTR_SIZE(size) \
>  	(((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
>  
> +/*
> + * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
> + * for file system consistency errors, we use a somewhat bigger value.
> + * This allows XATTR_SIZE_MAX to grow in the future, but by using this
> + * instead of INT_MAX for certain consistency checks, we don't need to
> + * worry about arithmetic overflows.  (Actually XATTR_SIZE_MAX is
> + * defined in include/uapi/linux/limits.h, so changing it is going
> + * not going to be trivial....)
> + */
> +#define EXT2_XATTR_SIZE_MAX (1 << 24)
> +
>  #ifdef __KERNEL__
>  # ifdef CONFIG_EXT2_FS_EXT_ATTR
>  extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
> -- 
> 2.31.0
> 

Re: [PATCH 5/7] e2fsck: avoid out-of-bounds write for very deep extent trees

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:42AM -0400, Theodore Ts'o wrote:
> The kernel doesn't support extent trees deeper than 5
> (EXT4_MAX_EXTENT_DEPTH).  For this reason we only maintain the extent
> tree statistics for 5 levels.  Avoid out-of-bounds writes and reads if
> the extent tree is deeper than this.
> 
> We keep these statistics to determine whether we should rebuild the
> extent tree.  If the extent tree is too deep, we don't need the
> statistics because we should always rebuild the it.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/extents.c | 10 +++++++++-
>  e2fsck/pass1.c   |  3 ++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index 01879f56..86fe00e7 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -526,7 +526,8 @@ errcode_t e2fsck_check_rebuild_extents(e2fsck_t ctx, ext2_ino_t ino,
>  		 */
>  		if (info.curr_entry == 1 &&
>  		    !(extent.e_flags & EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
> -		    !eti.force_rebuild) {
> +		    !eti.force_rebuild &&
> +		    info.curr_level < MAX_EXTENT_DEPTH_COUNT) {
>  			struct extent_tree_level *etl;
>  
>  			etl = eti.ext_info + info.curr_level;
> @@ -580,6 +581,13 @@ errcode_t e2fsck_should_rebuild_extents(e2fsck_t ctx,
>  	extents_per_block = (ctx->fs->blocksize -
>  			     sizeof(struct ext3_extent_header)) /
>  			    sizeof(struct ext3_extent);
> +
> +	/* If the extent tree is too deep, then rebuild it. */
> +	if (info->max_depth > MAX_EXTENT_DEPTH_COUNT) {
> +		pctx->blk = info->max_depth;
> +		op = PR_1E_CAN_COLLAPSE_EXTENT_TREE;
> +		goto rebuild;
> +	}
>  	/*
>  	 * If we can consolidate a level or shorten the tree, schedule the
>  	 * extent tree to be rebuilt.
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 11d7ce93..43972e7c 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2842,7 +2842,8 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>  	if (pctx->errcode)
>  		return;
>  	if (!(ctx->options & E2F_OPT_FIXES_ONLY) &&
> -	    !pb->eti.force_rebuild) {
> +	    !pb->eti.force_rebuild &&
> +	    info.curr_level < MAX_EXTENT_DEPTH_COUNT) {
>  		struct extent_tree_level *etl;
>  
>  		etl = pb->eti.ext_info + info.curr_level;
> -- 
> 2.31.0
> 

Re: [PATCH 6/7] libext2fs: check for cyclic loops in the extent tree

[Lukas Czerner]

On Tue, Jun 07, 2022 at 12:24:43AM -0400, Theodore Ts'o wrote:
> In the extent tree handling code in libext2fs, when we go move down
> the extent tree, if a cyclic loop is detected, return an error.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  lib/ext2fs/ext2_err.et.in |  3 +++
>  lib/ext2fs/extent.c       | 11 +++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> index cf0e00ea..bb1dcf14 100644
> --- a/lib/ext2fs/ext2_err.et.in
> +++ b/lib/ext2fs/ext2_err.et.in
> @@ -551,4 +551,7 @@ ec	EXT2_ET_NO_GDESC,
>  ec	EXT2_FILSYS_CORRUPTED,
>  	"The internal ext2_filsys data structure appears to be corrupted"
>  
> +ec	EXT2_ET_EXTENT_CYCLE,
> +	"Found cyclic loop in extent tree"
> +
>  	end
> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 1a206a16..82e75ccd 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -47,6 +47,7 @@ struct extent_path {
>  	int		visit_num;
>  	int		flags;
>  	blk64_t		end_blk;
> +	blk64_t		blk;
>  	void		*curr;
>  };
>  
> @@ -286,6 +287,7 @@ errcode_t ext2fs_extent_open2(ext2_filsys fs, ext2_ino_t ino,
>  	handle->path[0].end_blk =
>  		(EXT2_I_SIZE(handle->inode) + fs->blocksize - 1) >>
>  		 EXT2_BLOCK_SIZE_BITS(fs->super);
> +	handle->path[0].blk = 0;
>  	handle->path[0].visit_num = 1;
>  	handle->level = 0;
>  	handle->magic = EXT2_ET_MAGIC_EXTENT_HANDLE;
> @@ -305,14 +307,14 @@ errout:
>  errcode_t ext2fs_extent_get(ext2_extent_handle_t handle,
>  			    int flags, struct ext2fs_extent *extent)
>  {
> -	struct extent_path	*path, *newpath;
> +	struct extent_path	*path, *newpath, *tp;
>  	struct ext3_extent_header	*eh;
>  	struct ext3_extent_idx		*ix = 0;
>  	struct ext3_extent		*ex;
>  	errcode_t			retval;
>  	blk64_t				blk;
>  	blk64_t				end_blk;
> -	int				orig_op, op;
> +	int				orig_op, op, l;
>  	int				failed_csum = 0;
>  
>  	EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EXTENT_HANDLE);
> @@ -467,6 +469,11 @@ retry:
>  		}
>  		blk = ext2fs_le32_to_cpu(ix->ei_leaf) +
>  			((__u64) ext2fs_le16_to_cpu(ix->ei_leaf_hi) << 32);
> +		for (l = handle->level, tp = path; l > 0; l--, tp--) {
> +			if (blk == tp->blk)
> +				return EXT2_ET_EXTENT_CYCLE;
> +		}
> +		newpath->blk = blk;
>  		if ((handle->fs->flags & EXT2_FLAG_IMAGE_FILE) &&
>  		    (handle->fs->io != handle->fs->image_io))
>  			memset(newpath->buf, 0, handle->fs->blocksize);
> -- 
> 2.31.0
> 

Re: [PATCH 7/7] libext2fs: check for invalid blocks in ext2fs_punch_blocks()

[Lukas Czerner]

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>


Thanks!
-Lukas

On Tue, Jun 07, 2022 at 12:24:44AM -0400, Theodore Ts'o wrote:
> If the extent tree has out-of-range physical block numbers, don't try
> to release them.
> 
> Also add a similar check in ext2fs_block_alloc_stats2() to avoid a
> NULL pointer dereference.
> 
> Reported-by: Nils Bars <nils.bars@rub.de>
> Reported-by: Moritz Schlögel <moritz.schloegel@rub.de>
> Reported-by: Nico Schiller <nico.schiller@rub.de>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  lib/ext2fs/alloc_stats.c | 3 ++-
>  lib/ext2fs/punch.c       | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
> index 3949f618..6f98bcc7 100644
> --- a/lib/ext2fs/alloc_stats.c
> +++ b/lib/ext2fs/alloc_stats.c
> @@ -62,7 +62,8 @@ void ext2fs_block_alloc_stats2(ext2_filsys fs, blk64_t blk, int inuse)
>  {
>  	int	group = ext2fs_group_of_blk2(fs, blk);
>  
> -	if (blk >= ext2fs_blocks_count(fs->super)) {
> +	if (blk < fs->super->s_first_data_block ||
> +	    blk >= ext2fs_blocks_count(fs->super)) {
>  #ifndef OMIT_COM_ERR
>  		com_err("ext2fs_block_alloc_stats", 0,
>  			"Illegal block number: %lu", (unsigned long) blk);
> diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
> index effa1e2d..e2543e1e 100644
> --- a/lib/ext2fs/punch.c
> +++ b/lib/ext2fs/punch.c
> @@ -200,6 +200,10 @@ static errcode_t punch_extent_blocks(ext2_filsys fs, ext2_ino_t ino,
>  	__u32		cluster_freed;
>  	errcode_t	retval = 0;
>  
> +	if (free_start < fs->super->s_first_data_block ||
> +	    (free_start + free_count) >= ext2fs_blocks_count(fs->super))
> +		return EXT2_ET_BAD_BLOCK_NUM;
> +
>  	/* No bigalloc?  Just free each block. */
>  	if (EXT2FS_CLUSTER_RATIO(fs) == 1) {
>  		*freed += free_count;
> -- 
> 2.31.0
>