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@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).
edward
