Re: [PATCH] squashfs: fix oob in squashfs_readahead

[Edward Adam Davis <eadavis@qq.com>]

On Thu, 16 Nov 2023 15:14:24 +0000, Phillip Lougher wrote:
> > [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.
[There is my debuging patch]
diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 581ce9519339..1c7c5500206b 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -314,9 +314,11 @@ int squashfs_read_data(struct super_block *sb, u64 index, int length,
 		bio_uninit(bio);
 		kfree(bio);

+		printk("datal: %d \n", length);
 		compressed = SQUASHFS_COMPRESSED(length);
 		length = SQUASHFS_COMPRESSED_SIZE(length);
 		index += 2;
+		printk("datal2: %d, c:%d, i:%d \n", length, compressed, index);

 		TRACE("Block @ 0x%llx, %scompressed size %d\n", index - 2,
 		      compressed ? "" : "un", length);
@@ -324,6 +326,7 @@ int squashfs_read_data(struct super_block *sb, u64 index, int length,
 	if (length < 0 || length > output->length ||
 			(index + length) > msblk->bytes_used) {
 		res = -EIO;
+		printk("srd: l:%d, ol: %d, bu: %d \n", length, output->length, msblk->bytes_used);
 		goto out;
 	}

patch link: https://syzkaller.appspot.com/text?tag=Patch&x=1142f82f680000

[There is my test log]
[  457.030754][ T8879] datal: 65473
[  457.034334][ T8879] datal2: 32705, c:0, i:1788
[  457.039253][ T8879] srd: l:32705, ol: 8192, bu: 1870
[  457.044513][ T8879] SQUASHFS error: Failed to read block 0x6fc: -5
[  457.052034][ T8879] SQUASHFS error: Unable to read metadata cache entry [6fa]
log link: https://syzkaller.appspot.com/x/log.txt?x=137b0270e80000

[Answer your doubts]
Based on the above test, it can be clearly determined that length=32705 is 
greater than the maximum metadata size of 8192, resulting in squashfs_read_data() failed.
This will further lead to squashfs_cache_get() returns "entry->error=entry->length=-EIO", 
followed by squashfs_read_metadata() failed, which will ultimately result in i_size not 
being initialized in squashfs_read_inode().

The following are the relevant call stacks:
  23 const struct inode_operations squashfs_dir_inode_ops = {
  24         .lookup = squashfs_lookup,
  25         .listxattr = squashfs_listxattr
  26 };
  NORMAL  +0 ~0 -0  fs/squashfs/namei.c

  path_openat()->open_last_lookups()->lookup_open()
    1         if (d_in_lookup(dentry)) {
 3455                 struct dentry *res = dir_inode->i_op->lookup(dir_inode, dentry,                                                                                                                    1                                                              nd->flags);

 squashfs_lookup()->
	 squashfs_iget()->
	 squashfs_read_inode()-> 
	 init inode->i_size, example: inode->i_size = sqsh_ino->file_size;
> 
> https://lore.kernel.org/all/000000000000bb74b9060a14717c@google.com/
> 
> 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).

edward