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

[Jan Kara]

On Sat 26-09-26 16:35:17, Matthias Goergens wrote:
> isofs_read_level3_size() walks the sections of a level-3 (multi-extent)
> directory record and, on a zero length byte, moves on to the next
> block with no limit.  A crafted image can put a long run of empty
> blocks between two sections of a multi-extent file, and the walk reads
> every one of them before it gives up, all the way to the end of the
> device if it has to.
> 
> Real discs do have trailing empty directory blocks, written by tools
> such as Easy CD Creator and Nero, but always at the end of the
> directory, never between two sections of a multi-extent record, so
> they do not exercise this path [1].
> 
> Jan Kara suggested treating an empty block like a section: count it
> towards the existing 100-section limit rather than adding a separate
> one [2].  Do that: both the section count and the empty-block count
> are checked against their combined total, so neither one alone can
> reach 100 while the other keeps growing.  Only a zero length byte at
> the start of a block counts as an empty block; the same byte later in
> a block still just ends that block's records, as it does today, and
> is not counted.
> 
> Hui Peng's earlier patch for this function added its own, separate
> limit on the number of empty blocks [3]; this uses the combined limit
> Jan suggested instead.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> 
> [1] https://lore.kernel.org/all/20260925043804.2091174-1-matthias.goergens@xxxxxxxxx/
> [2] https://lore.kernel.org/all/cmlro2xzle2aa7ebflvxhbcv74mea7p6qvxhin6egpdvyzyuxh@s45tpgxdal3n/
> [3] https://lore.kernel.org/all/20260919222553.3792320-1-benquike@xxxxxxxxx/
> ---
> Tested with fs/isofs built as a userspace program under ASan and UBSan,
> and in a KASAN VM on Jan's for_next:
> 
> - The real discs from [1] (DM_BXL2, Comdex_05, ITSOFTCD_39, each with
>   trailing empty directory blocks) and the level-3 images from the
>   earlier patches list and read the same with and without this patch.
> - A crafted multi-extent file with 150 empty blocks between two sections
>   now stops with "More than 100 file sections/empty blocks ?!?" instead
>   of reading all of them.
> - A crafted file with 60 sections and 50 empty blocks interleaved (110
>   in all) is rejected; without this patch it is accepted.
> 
> The image generators are at
> https://github.com/matthiasgoergens/linux/tree/reproducer/2026-09-26-isofs-empty-blocks

Thanks for the patch and the generator. Just one question below:

> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 184350d2e6ad..e884618c0c53 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1175,6 +1175,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_inode_info *ei = ISOFS_I(inode);
>  
> @@ -1202,9 +1203,14 @@ static int isofs_read_level3_size(struct inode *inode)
>  
>  		/*
>  		 * If we are at the end of a block (or at its zero-padded
> -		 * tail), move on to the next block.
> +		 * tail), move on to the next block.  A zero length byte at
> +		 * the start of a block means the whole block is empty;
> +		 * count that towards the same limit as sections below, or a
> +		 * chain of empty blocks could be walked without bound.
>  		 */
>  		if (offset >= bufsize || de->length[0] == 0) {
> +			if (offset == 0 && ++empty_blocks + i > 100)
> +				goto out_toomany;

Any reason why don't you do just "++i > 100" here and completely remove the
empty_blocks variable?

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR