[PATCH v2] isofs: bound empty directory blocks in isofs_read_level3_size()

[PATCH v2] isofs: bound empty directory blocks in isofs_read_level3_size()

[Hui Peng]

isofs_read_level3_size() walks the sections of a level-3 (multi-extent)
directory record and, on encountering a zero length byte (de_len == 0),
moves on to the next block without bound. A crafted image with a long run
of empty blocks between two sections of a multi-extent file can force the
walk to read every block to the end of the device.

Count empty blocks (de_len == 0 at offset == 0) towards the existing
100-section limit. Track empty_blocks in a separate variable from section
count 'i' rather than incrementing 'i' directly, because 'if (i == 1)'
records the start of the second section (ei->i_next_section_block/offset);
incrementing 'i' on empty blocks before the second section is read would
cause 'i' to jump past 1, leaving the next-section pointer uninitialized
(0) and reading past the first extent into unrelated disk data.

Tested in QEMU against Linux 7.3.0-rc3 with KASAN enabled using a crafted
ISO image containing 150 consecutive empty blocks between two extents:
on the unfixed kernel the loop reads all 150 blocks; whereas with this fix
applied, the loop aborts with "isofs_read_level3_size: More than 100 file
sections/empty blocks ?!?".

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Suggested-by: Jan Kara <jack@suse.cz>
Co-developed-by: Matthias Goergens <matthias.goergens@gmail.com>
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Document in code comment why empty_blocks is tracked separately from
  section count 'i' rather than incrementing 'i' directly (preserving
  the 'if (i == 1)' check for the second section pointer), answering Jan
  Kara's reviewer feedback.

Changes in v1:
- Combined section and empty-block bounds check into i + empty_blocks > 100.

 fs/isofs/inode.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 184350d2e6ad..d7b324029a1b 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1173,6 +1173,7 @@ static int isofs_read_level3_size(struct inode *inode)
 	struct buffer_head *bh = NULL;
 	unsigned long block, offset, block_saved, offset_saved;
 	int i = 0;
+	int empty_blocks = 0;
 	int more_entries = 0;
 	struct iso_directory_record *tmpde = NULL;
 	struct iso_inode_info *ei = ISOFS_I(inode);
@@ -1200,6 +1201,17 @@ static int isofs_read_level3_size(struct inode *inode)
 		de_len = *(unsigned char *) de;

 		if (de_len == 0) {
+			/*
+			 * A zero length byte at the start of a block (offset == 0)
+			 * means the whole block is empty. Count that towards the
+			 * same 100 limit as file sections, or a chain of empty
+			 * blocks could be walked without bound.
+			 * Track empty_blocks separately from section count 'i'
+			 * so that 'if (i == 1)' correctly identifies the start of
+			 * the second section even if an empty block precedes it.
+			 */
+			if (offset == 0 && ++empty_blocks + i > 100)
+				goto out_toomany;
 			brelse(bh);
 			bh = NULL;
 			++block;
@@ -1241,7 +1253,7 @@ static int isofs_read_level3_size(struct inode *inode)
 		more_entries = de->flags[-high_sierra] & 0x80;

 		i++;
-		if (i > 100)
+		if (i + empty_blocks > 100)
 			goto out_toomany;
 	} while (more_entries);
 out:
@@ -1258,7 +1270,7 @@ static int isofs_read_level3_size(struct inode *inode)
 	return -EIO;

 out_toomany:
-	printk(KERN_INFO "%s: More than 100 file sections ?!?, aborting...\n"
+	printk(KERN_INFO "%s: More than 100 file sections/empty blocks ?!?, aborting...\n"
 		"isofs_read_level3_size: inode=%llu\n",
 		__func__, inode->i_ino);
 	goto out;

Re: [PATCH v2] isofs: bound empty directory blocks in isofs_read_level3_size()

[Jan Kara]

On Wed 30-09-26 04:21:48, Hui Peng wrote:
> isofs_read_level3_size() walks the sections of a level-3 (multi-extent)
> directory record and, on encountering a zero length byte (de_len == 0),
> moves on to the next block without bound. A crafted image with a long run
> of empty blocks between two sections of a multi-extent file can force the
> walk to read every block to the end of the device.
> 
> Count empty blocks (de_len == 0 at offset == 0) towards the existing
> 100-section limit. Track empty_blocks in a separate variable from section
> count 'i' rather than incrementing 'i' directly, because 'if (i == 1)'
> records the start of the second section (ei->i_next_section_block/offset);
> incrementing 'i' on empty blocks before the second section is read would
> cause 'i' to jump past 1, leaving the next-section pointer uninitialized
> (0) and reading past the first extent into unrelated disk data.
> 
> Tested in QEMU against Linux 7.3.0-rc3 with KASAN enabled using a crafted
> ISO image containing 150 consecutive empty blocks between two extents:
> on the unfixed kernel the loop reads all 150 blocks; whereas with this fix
> applied, the loop aborts with "isofs_read_level3_size: More than 100 file
> sections/empty blocks ?!?".
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Suggested-by: Jan Kara <jack@suse.cz>
> Co-developed-by: Matthias Goergens <matthias.goergens@gmail.com>
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>

Sorry Hui, I already have a patch from Matthias for this in my tree.

								Honza

> ---
> Changes in v2:
> - Document in code comment why empty_blocks is tracked separately from
>   section count 'i' rather than incrementing 'i' directly (preserving
>   the 'if (i == 1)' check for the second section pointer), answering Jan
>   Kara's reviewer feedback.
> 
> Changes in v1:
> - Combined section and empty-block bounds check into i + empty_blocks > 100.
> 
>  fs/isofs/inode.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 184350d2e6ad..d7b324029a1b 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1173,6 +1173,7 @@ static int isofs_read_level3_size(struct inode *inode)
>  	struct buffer_head *bh = NULL;
>  	unsigned long block, offset, block_saved, offset_saved;
>  	int i = 0;
> +	int empty_blocks = 0;
>  	int more_entries = 0;
>  	struct iso_directory_record *tmpde = NULL;
>  	struct iso_inode_info *ei = ISOFS_I(inode);
> @@ -1200,6 +1201,17 @@ static int isofs_read_level3_size(struct inode *inode)
>  		de_len = *(unsigned char *) de;
> 
>  		if (de_len == 0) {
> +			/*
> +			 * A zero length byte at the start of a block (offset == 0)
> +			 * means the whole block is empty. Count that towards the
> +			 * same 100 limit as file sections, or a chain of empty
> +			 * blocks could be walked without bound.
> +			 * Track empty_blocks separately from section count 'i'
> +			 * so that 'if (i == 1)' correctly identifies the start of
> +			 * the second section even if an empty block precedes it.
> +			 */
> +			if (offset == 0 && ++empty_blocks + i > 100)
> +				goto out_toomany;
>  			brelse(bh);
>  			bh = NULL;
>  			++block;
> @@ -1241,7 +1253,7 @@ static int isofs_read_level3_size(struct inode *inode)
>  		more_entries = de->flags[-high_sierra] & 0x80;
> 
>  		i++;
> -		if (i > 100)
> +		if (i + empty_blocks > 100)
>  			goto out_toomany;
>  	} while (more_entries);
>  out:
> @@ -1258,7 +1270,7 @@ static int isofs_read_level3_size(struct inode *inode)
>  	return -EIO;
> 
>  out_toomany:
> -	printk(KERN_INFO "%s: More than 100 file sections ?!?, aborting...\n"
> +	printk(KERN_INFO "%s: More than 100 file sections/empty blocks ?!?, aborting...\n"
>  		"isofs_read_level3_size: inode=%llu\n",
>  		__func__, inode->i_ino);
>  	goto out;
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR