diff mbox series

squashfs: fix oob in squashfs_readahead

Message ID tencent_35864B36740976B766CA3CC936A496AA3609@qq.com (mailing list archive)
State New, archived
Headers show
Series squashfs: fix oob in squashfs_readahead | expand

Commit Message

Edward Adam Davis Nov. 15, 2023, 4:05 a.m. UTC
[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(-)

Comments

Andrew Morton Nov. 15, 2023, 10:39 p.m. UTC | #1
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;
	}

?
Phillip Lougher Nov. 16, 2023, 3:14 p.m. UTC | #2
> [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
Marek Szyprowski Nov. 17, 2023, 1:17 p.m. UTC | #3
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
Andrew Morton Nov. 17, 2023, 3:48 p.m. UTC | #4
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.
Edward Adam Davis Nov. 18, 2023, 2:12 a.m. UTC | #5
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 mbox series

Patch

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);