| Message ID | tencent_35864B36740976B766CA3CC936A496AA3609@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | squashfs: fix oob in squashfs_readahead | expand |
On Wed, 15 Nov 2023 12:05:35 +0800 Edward Adam Davis <eadavis@qq.com> wrote: > Before performing a read ahead operation in squashfs_read_folio() and > squashfs_readahead(), check if i_size is not 0 before continuing. I'll merge this for testing, pending Phillip's review. One thing: > --- a/fs/squashfs/block.c > +++ b/fs/squashfs/block.c > @@ -323,7 +323,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; > + res = length < 0 ? -EIO : -EFBIG; > goto out; > } Seems a bit ugly to test `length' twice for the same thing. How about if (length < 0) { res = -EIO; got out; } if (length > output->length || (index + length) > msblk->bytes_used) { res = -EFBIG; goto out; } ?
> [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@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). I already identified the cause and send a fix patch here: https://lore.kernel.org/all/20231113160901.6444-1-phillip@squashfs.org.uk/ NACK Phillip
Hi All, On 15.11.2023 05:05, Edward Adam Davis wrote: > [Syz log] > SQUASHFS error: Failed to read block 0x6fc: -5 > SQUASHFS error: Unable to read metadata cache entry [6fa] > SQUASHFS error: Unable to read metadata cache entry [6fa] > SQUASHFS error: Unable to read metadata cache entry [6fa] > ================================================================== > BUG: KASAN: slab-out-of-bounds in __readahead_batch include/linux/pagemap.h:1364 [inline] > BUG: KASAN: slab-out-of-bounds in squashfs_readahead+0x9a6/0x20d0 fs/squashfs/file.c:569 > Write of size 8 at addr ffff88801e393648 by task syz-executor100/5067 > > CPU: 1 PID: 5067 Comm: syz-executor100 Not tainted 6.6.0-syzkaller-15156-g13d88ac54ddd #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:364 [inline] > print_report+0x163/0x540 mm/kasan/report.c:475 > kasan_report+0x142/0x170 mm/kasan/report.c:588 > __readahead_batch include/linux/pagemap.h:1364 [inline] > squashfs_readahead+0x9a6/0x20d0 fs/squashfs/file.c:569 > read_pages+0x183/0x830 mm/readahead.c:160 > page_cache_ra_unbounded+0x68e/0x7c0 mm/readahead.c:269 > page_cache_sync_readahead include/linux/pagemap.h:1266 [inline] > filemap_get_pages+0x49c/0x2080 mm/filemap.c:2497 > filemap_read+0x42b/0x10b0 mm/filemap.c:2593 > __kernel_read+0x425/0x8b0 fs/read_write.c:428 > integrity_kernel_read+0xb0/0xf0 security/integrity/iint.c:221 > ima_calc_file_hash_tfm security/integrity/ima/ima_crypto.c:485 [inline] > ima_calc_file_shash security/integrity/ima/ima_crypto.c:516 [inline] > ima_calc_file_hash+0xad1/0x1b30 security/integrity/ima/ima_crypto.c:573 > ima_collect_measurement+0x554/0xb30 security/integrity/ima/ima_api.c:290 > process_measurement+0x1373/0x21c0 security/integrity/ima/ima_main.c:359 > ima_file_check+0xf1/0x170 security/integrity/ima/ima_main.c:557 > do_open fs/namei.c:3624 [inline] > path_openat+0x2893/0x3280 fs/namei.c:3779 > > [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. > > 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. > > [Fix] > Before performing a read ahead operation in squashfs_read_folio() and > squashfs_readahead(), check if i_size is not 0 before continuing. > > Optimize the return value of squashfs_read_data() and return -EFBIG when the > length is greater than output->length(or (index + length) > > msblk->bytes_used). > > Reported-and-tested-by: syzbot+604424eb051c2f696163@syzkaller.appspotmail.com > Fixes: f268eedddf35 ("squashfs: extend "page actor" to handle missing pages") > Signed-off-by: Edward Adam Davis <eadavis@qq.com> This patch, merged to linux-next as commit 1ff947abe24a ("squashfs: fix oob in squashfs_readahead"), breaks mounting squashfs volumes on all my test systems. Let me know if you need more information to debug this issue. > --- > fs/squashfs/block.c | 2 +- > fs/squashfs/file.c | 8 ++++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c > index 581ce9519339..d335f28c822c 100644 > --- a/fs/squashfs/block.c > +++ b/fs/squashfs/block.c > @@ -323,7 +323,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; > + res = length < 0 ? -EIO : -EFBIG; > goto out; > } > > diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c > index 8ba8c4c50770..5472ddd3596c 100644 > --- a/fs/squashfs/file.c > +++ b/fs/squashfs/file.c > @@ -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; > + } > + > if (page->index >= ((i_size_read(inode) + PAGE_SIZE - 1) >> > PAGE_SHIFT)) > goto out; > @@ -547,6 +552,9 @@ static void squashfs_readahead(struct readahead_control *ractl) > int i, file_end = i_size_read(inode) >> msblk->block_log; > unsigned int max_pages = 1UL << shift; > > + if (!file_end) > + return; > + > readahead_expand(ractl, start, (len | mask) + 1); > > pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL); Best regards
On Fri, 17 Nov 2023 14:17:17 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Reported-and-tested-by: syzbot+604424eb051c2f696163@syzkaller.appspotmail.com > > Fixes: f268eedddf35 ("squashfs: extend "page actor" to handle missing pages") > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > This patch, merged to linux-next as commit 1ff947abe24a ("squashfs: fix > oob in squashfs_readahead"), breaks mounting squashfs volumes on all my > test systems. Let me know if you need more information to debug this issue. Thanks. The patch has been dropped.
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
diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c index 581ce9519339..d335f28c822c 100644 --- a/fs/squashfs/block.c +++ b/fs/squashfs/block.c @@ -323,7 +323,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; + res = length < 0 ? -EIO : -EFBIG; goto out; } diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c index 8ba8c4c50770..5472ddd3596c 100644 --- a/fs/squashfs/file.c +++ b/fs/squashfs/file.c @@ -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; + } + if (page->index >= ((i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT)) goto out; @@ -547,6 +552,9 @@ static void squashfs_readahead(struct readahead_control *ractl) int i, file_end = i_size_read(inode) >> msblk->block_log; unsigned int max_pages = 1UL << shift; + if (!file_end) + return; + readahead_expand(ractl, start, (len | mask) + 1); pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL);
[Syz log] SQUASHFS error: Failed to read block 0x6fc: -5 SQUASHFS error: Unable to read metadata cache entry [6fa] SQUASHFS error: Unable to read metadata cache entry [6fa] SQUASHFS error: Unable to read metadata cache entry [6fa] ================================================================== BUG: KASAN: slab-out-of-bounds in __readahead_batch include/linux/pagemap.h:1364 [inline] BUG: KASAN: slab-out-of-bounds in squashfs_readahead+0x9a6/0x20d0 fs/squashfs/file.c:569 Write of size 8 at addr ffff88801e393648 by task syz-executor100/5067 CPU: 1 PID: 5067 Comm: syz-executor100 Not tainted 6.6.0-syzkaller-15156-g13d88ac54ddd #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106 print_address_description mm/kasan/report.c:364 [inline] print_report+0x163/0x540 mm/kasan/report.c:475 kasan_report+0x142/0x170 mm/kasan/report.c:588 __readahead_batch include/linux/pagemap.h:1364 [inline] squashfs_readahead+0x9a6/0x20d0 fs/squashfs/file.c:569 read_pages+0x183/0x830 mm/readahead.c:160 page_cache_ra_unbounded+0x68e/0x7c0 mm/readahead.c:269 page_cache_sync_readahead include/linux/pagemap.h:1266 [inline] filemap_get_pages+0x49c/0x2080 mm/filemap.c:2497 filemap_read+0x42b/0x10b0 mm/filemap.c:2593 __kernel_read+0x425/0x8b0 fs/read_write.c:428 integrity_kernel_read+0xb0/0xf0 security/integrity/iint.c:221 ima_calc_file_hash_tfm security/integrity/ima/ima_crypto.c:485 [inline] ima_calc_file_shash security/integrity/ima/ima_crypto.c:516 [inline] ima_calc_file_hash+0xad1/0x1b30 security/integrity/ima/ima_crypto.c:573 ima_collect_measurement+0x554/0xb30 security/integrity/ima/ima_api.c:290 process_measurement+0x1373/0x21c0 security/integrity/ima/ima_main.c:359 ima_file_check+0xf1/0x170 security/integrity/ima/ima_main.c:557 do_open fs/namei.c:3624 [inline] path_openat+0x2893/0x3280 fs/namei.c:3779 [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. 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. [Fix] Before performing a read ahead operation in squashfs_read_folio() and squashfs_readahead(), check if i_size is not 0 before continuing. Optimize the return value of squashfs_read_data() and return -EFBIG when the length is greater than output->length(or (index + length) > msblk->bytes_used). Reported-and-tested-by: syzbot+604424eb051c2f696163@syzkaller.appspotmail.com Fixes: f268eedddf35 ("squashfs: extend "page actor" to handle missing pages") Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/squashfs/block.c | 2 +- fs/squashfs/file.c | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)