Re: [PATCH] isofs: advance to the next block when a record ends at the block end

[Jan Kara]

On Tue 22-09-26 13:48:53, Matthias Goergens wrote:
> Since commit b2eb2e288604 ("isofs: Drop support of directory entries
> straddling blocks") some ISO 9660 images list incomplete directories:
> find returns fewer entries than before, and some of the names readdir
> does return cannot be looked up.  The log fills with
> 
>   iso9660: Corrupted directory entry in block 0 of inode 55232
> 
> and on some images the walk reads a byte past the end of a buffer:
> 
>   BUG: KASAN: use-after-free in isofs_readdir+0x93b/0xc40
> 
> Affected are images with a directory record ending exactly at the end of
> a logical block, which ECMA-119 allows and current tools produce.  On
> the Debian 13.7.0 amd64 DVD-1, which has three, find returns 9163
> entries instead of 9461:
> 
>   https://cdimage.debian.org/debian-cd/13.7.0/amd64/iso-dvd/debian-13.7.0-amd64-DVD-1.iso
> 
> The KASAN report is from a shareware CD with one in its root directory:
> 
>   https://archive.org/download/simtel-1-0295/SIMTEL1_0295.ISO
> 
> A local one, which lists 46 of its 65 files:
> 
>   mkdir tree
>   for i in $(seq -f '%04g' 1 65); do echo hi > tree/FILE$i.TXT; done
>   xorrisofs -iso-level 1 -o boundary.iso tree/
> 
> genisoimage pads the tail of a block instead of filling it exactly, so
> its images have no such records.
> 
> That commit removed the reassembly of records crossing a block boundary.
> Its condition was "offset >= bufsize", so it also handled a record
> ending exactly at the end (offset == bufsize): drop the buffer,
> increment the block, reset the offset.  Nothing does that now, so
> do_isofs_readdir() and isofs_find_entry() read the next record's length
> byte from bh->b_data + bufsize, one past the buffer.  Depending on that
> byte the walk either treats the rest of the block as padding and skips
> the next block of entries, or fails the record as corrupt and stops.
> 
> Straddling records stay rejected: isofs_dir_record_valid() fails them
> because their length exceeds the remaining space, which is what that
> commit relied on.  One ending exactly at the end fits in its block and
> is accepted, so it is the advance, not the reassembly, that has to come
> back.
> 
> Do it at the top of the loop: do_isofs_readdir() reaches the next
> iteration from the multi-extent branch, "." and "..", the
> hidden/associated skip (showassoc is off by default, so no mount option
> is needed) and the fall-through, and all of them need covering.  The
> record was validated to end within the block, so offset never exceeds
> bufsize.
> 
> Fixes: b2eb2e288604 ("isofs: Drop support of directory entries straddling blocks")
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

Thanks for the analysis and the fix! In fact I got notified about the
problem last week and fixed it (sligthly differently) [1]. I've now added
you as a reporter at least to give you some credit.

BTW, it is a good observation that there's yet another instance of
directory reading code in isofs_read_level3_size(). That deserves a similar
simplification as what commit b2eb2e288604 did (just without introducing
the bug this time :)).

								Honza

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?id=bda8d8d49ca14e7f3bdc39fdc4330a48b32d6b6f


> ---
> Not tagged for stable: b2eb2e288604 is in 7.3-rc1..rc3 and has not
> appeared in a released kernel.
> 
> Validated under qemu with KASAN and UBSAN against the images above,
> using a kernel built from the commit before b2eb2e288604 as the
> reference for correct behaviour.
> 
> do_isofs_readdir(), isofs_find_entry() and isofs_read_level3_size() all
> open-code this same block-end transition.  Happy to follow up with a
> helper that shares it, kept out of here to keep the regression fix
> minimal.
> 
>  fs/isofs/dir.c   | 12 ++++++++++++
>  fs/isofs/namei.c | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
> index c7ca7603e97a..00a609c901dd 100644
> --- a/fs/isofs/dir.c
> +++ b/fs/isofs/dir.c
> @@ -104,6 +104,18 @@ static int do_isofs_readdir(struct inode *inode, struct file *file,
>  	while (ctx->pos < inode->i_size) {
>  		int de_len;
>  
> +		/*
> +		 * The previous record was validated to end within its
> +		 * block; if it ended exactly at the end, the next record
> +		 * starts at the beginning of the next block.
> +		 */
> +		if (offset == bufsize) {
> +			brelse(bh);
> +			bh = NULL;
> +			block++;
> +			offset = 0;
> +		}
> +
>  		if (!bh) {
>  			bh = isofs_bread(inode, block);
>  			if (!bh)
> diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
> index 010682f5901a..5f5ed36d12e3 100644
> --- a/fs/isofs/namei.c
> +++ b/fs/isofs/namei.c
> @@ -68,6 +68,18 @@ isofs_find_entry(struct inode *dir, struct dentry *dentry,
>  		int de_len, match, i, dlen;
>  		char *dpnt;
>  
> +		/*
> +		 * The previous record was validated to end within its
> +		 * block; if it ended exactly at the end, the next record
> +		 * starts at the beginning of the next block.
> +		 */
> +		if (offset == bufsize) {
> +			brelse(bh);
> +			bh = NULL;
> +			block++;
> +			offset = 0;
> +		}
> +
>  		if (!bh) {
>  			bh = isofs_bread(dir, block);
>  			if (!bh)
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR