Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <b28b25ab-87eb-4905-855a-7809dda11f39@samsung.com>
Date:   Fri, 17 Nov 2023 14:17:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] squashfs: fix oob in squashfs_readahead
Content-Language: en-US
To:     Edward Adam Davis <eadavis@qq.com>,
        syzbot+604424eb051c2f696163@syzkaller.appspotmail.com
Cc:     akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, phillip@squashfs.org.uk,
        squashfs-devel@lists.sourceforge.net,
        syzkaller-bugs@googlegroups.com
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <tencent_35864B36740976B766CA3CC936A496AA3609@qq.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <000000000000b1fda20609ede0d1@google.com>
        <tencent_35864B36740976B766CA3CC936A496AA3609@qq.com>
        <CGME20231117131718eucas1p13328b32942cce99a99197eb28e14a981@eucas1p1.samsung.com>
Precedence: bulk

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland