Re: [PATCH] squashfs: fix oob in squashfs_readahead

From: Phillip Lougher
Date: Thu Nov 16 2023 - 10:06:39 EST


> [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/000000000000bb74b9060a14717c@xxxxxxxxxx/

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/20231113160901.6444-1-phillip@xxxxxxxxxxxxxxx/

NACK

Phillip