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@xxxxxxxxxxxxxxx
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Co-developed-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@xxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR