Re: [Squashfs-devel] [PATCH] squashfs: fix oob in squashfs_readahead

[Phillip L. <ph...@sq...>]

> [Bug]
> path_openat() called open_last_lookups() before calling do_open() and 
> open_last_lookups() will eventually call squashfs_read_inode() to set 
> inode->i_size, but before setting i_size, it is necessary to obtain file_size 
> from the disk.
> 
> However, during the value retrieval process, the length of the value retrieved
> from the disk was greater than output->length, resulting(-EIO) in the failure of 
> squashfs_read_data(), further leading to i_size has not been initialized, 
> i.e. its value is 0.
> 

NACK

This analysis is completely *wrong*.  First, if there was I/O error reading
the inode it would never be created, and squasfs_readahead() would
never be called on it, because it will never exist.

Second i_size isn't unintialised and it isn't 0 in value.  Where
you got this bogus information from is because in your test patches,
i.e.

https://lore.kernel.org/all/000...@go.../

You have

+	if (!file_end) {
+		printk("i:%p, is:%d, %s\n", inode, i_size_read(inode), __func__);
+		res = -EINVAL;
+		goto out;
+	}
+

You have used %d, and the result of i_size_read(inode) overflows, giving the
bogus 0 value.

The actual value is 1407374883553280, or 0x5000000000000, which is
too big to fit into an unsigned int.

> This resulted in the failure of squashfs_read_data(), where "SQUASHFS error: 
> Failed to read block 0x6fc: -5" was output in the syz log.
> This also resulted in the failure of squashfs_cache_get(), outputting "SQUASHFS
> error: Unable to read metadata cache entry [6fa]" in the syz log.
> 

NO, *that* is caused by the failure to read some other inodes which
as a result are correctly not created.  Nothing to do with the oops here.

> [Fix]
> Before performing a read ahead operation in squashfs_read_folio() and 
> squashfs_readahead(), check if i_size is not 0 before continuing.
> 

A third NO, it is only 0 because the variable overflowed.

Additionally, let's look at your "fix" here.

> @@ -461,6 +461,11 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
>  	TRACE("Entered squashfs_readpage, page index %lx, start block %llx\n",
>  				page->index, squashfs_i(inode)->start);
>  
> +	if (!file_end) {
> +		res = -EINVAL;
> +		goto out;
> +	}
> +

file_end is computed by

	int file_end = i_size_read(inode) >> msblk->block_log;

So your "fix" will reject *any* file less than msblk->block_log in
size as invalid, including perfectly valid zero size files (empty
files are valid too).

I already identified the cause and send a fix patch here:

https://lore.kernel.org/all/202...@sq.../

NACK

Phillip