Re: [PATCH 1/2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Tue 22-09-26 22:01:36, Matthias Goergens wrote:
> isofs_read_level3_size() walks the multi-extent directory records of a
> file and dereferences de->size and de->flags for each one without ever
> checking the record's length byte.  A record with a short length placed
> near the end of a block makes those fixed-field reads run past the
> record, and for a record at the end of the last block of a page, past
> the buffer.
> 
> readdir, lookup and the NFS get_parent path have validated every record
> with isofs_dir_record_valid() since commit e2ee4078ec58 ("isofs:
> validate directory records consistently").  Use the same helper here.
> It rejects records shorter than the fixed part, records whose name
> would not fit, and records that would run past the block, so the
> straddling-record copy below can no longer be reached with a bad
> length.
> 
> Found by fuzzing fs/isofs in a userspace harness with ASan
> (heap-buffer-overflow reads in isonum_733(de->size)).
> 
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> ---
>  fs/isofs/inode.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 337836a0a170..c9bc1f479161 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1208,6 +1208,14 @@ static int isofs_read_level3_size(struct inode *inode)
>  			continue;
>  		}
>  
> +		if (!isofs_dir_record_valid(de, offset, bufsize)) {

This is going to trigger when the block is fully packed, won't it? I think
the check belongs to a moment after we've handled transition to the next
block...

								Honza

> +			printk(KERN_NOTICE "iso9660: Corrupted directory entry in block %lu of inode %llu\n",
> +			       block, inode->i_ino);
> +			brelse(bh);
> +			kfree(tmpde);
> +			return -EIO;
> +		}
> +
>  		block_saved = block;
>  		offset_saved = offset;
>  		offset += de_len;
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR