Re: [PATCH] isofs: reject short directory records in isofs_read_level3_size()

[Jan Kara]

On Fri 25-09-26 12:38:04, Matthias Goergens wrote:
> Hi Honza,
> 
> On Thu, Sep 24, 2026 at 11:13:56PM +0800, Matthias Goergens wrote:
> > Either way, I'll first check a set of real images for empty directory
> > blocks, so we know the check doesn't reject discs that mount today.
> 
> Empty directory blocks do occur: about 1% of the disc images I checked
> on archive.org have them, mostly from Easy CD Creator 4.0 to 4.2 and
> Nero, and always at the end of a directory.  One of them hangs on
> for_next, see [1].  Two more examples, with the bytes used in each
> block of the directory:
> 
>   https://archive.org/details/Anime_Transformer  DM_BXL2.iso
>     /XTRAS    2022 1526 empty   (Easy CD Creator 4.2)
>   https://archive.org/details/comdex-edu  Comdex_05.iso
>     /IMAGES   2012 2048 empty   (Nero)
> 
> So a general check in readdir or lookup would stop those discs from
> mounting.

OK, thanks for checking! BTW how did you do the check. Have you've downloaded
all the images? Anyway, despite this being contrary to ECMA-119 standard I
agree we shouldn't start refusing such images if they worked in the past.

> In isofs_read_level3_size() it would reject none of them: no empty block
> on those discs follows a record with the multi-extent flag, so the walk
> always ends first.  What refusing buys is limited to crafted images,
> where a flagged record followed by empty blocks makes the walk skip
> block after block until a read fails, because the 100-section limit
> counts records, not blocks.
> 
> No other reader I looked at rejects them: Microsoft's published CDFS
> sample, GRUB, libarchive, libcdio, 7-Zip and libisofs all skip empty
> blocks inside a multi-extent chain and stop at the end of the directory,
> and GRUB fixed an unbounded walk that way in 4e0bab34ece7 ("fs/iso9660:
> Add check to prevent infinite loop").  Windows Server 2022 and 2025 also
> join the sections across an empty block and return the whole file.
> 
> So I'd suggest following their lead: skip empty blocks in the walk as
> readdir and lookup do, and stop at the end of the directory.
> isofs_read_level3_size() doesn't know where the directory ends today,
> though.  isofs_lookup() does and could pass it down through
> __isofs_iget(), but inodes reached from an NFS file handle can't, so
> those would still need a bound such as counting skipped blocks towards
> the 100-section limit.  If that is more plumbing than you'd like, the
> counting alone would do.  What would you prefer?

Yes. I was actually looking at propagating the directory size to
__isofs_iget() yesterday before sending my replay and concluded we cannot
easily do that in all the cases. I don't think it makes sense to plumb the
directory size for the cases where we can do it - unless the image is
corrupted we don't need it and unreliable check doesn't help for corrupted
images. So yes, just keep the code skipping empty blocks and I'd just limit
the number of sections + empty blocks to 100. No sane disk image should
exceed that.

> Whatever the bound, only a zero length at the start of a block should
> count as an empty block.  A multi-extent record can end one block with
> padding and its next section start the following one; that has to keep
> working.

Agreed.

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