Message-ID: <68f15d67-8945-2728-1f17-5b53a80ec52d@squashfs.org.uk>
Date:   Fri, 27 Jan 2023 06:20:18 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 1/1] squashfs: harden sanity check in
 squashfs_read_xattr_id_table
To:     Fedor Pchelkin <pchelkin@ispras.ru>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-kernel@vger.kernel.org,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        lvc-project@linuxtesting.org,
        syzbot+082fa4af80a5bb1a9843@syzkaller.appspotmail.com
References: <20230117105226.329303-1-pchelkin@ispras.ru>
 <20230117105226.329303-2-pchelkin@ispras.ru>
From:   Phillip Lougher <phillip@squashfs.org.uk>
In-Reply-To: <20230117105226.329303-2-pchelkin@ispras.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/01/2023 10:52, Fedor Pchelkin wrote:
 > While mounting a corrupted filesystem, a signed integer '*xattr_ids' can
 > become less than zero. This leads to the incorrect computation of 'len'
 > and 'indexes' values which can cause null-ptr-deref in 
copy_bio_to_actor()
 > or out-of-bounds accesses in the next sanity checks inside
 > squashfs_read_xattr_id_table().
 >

NACK

Thanks for sending the patch, but, you have unfortunately identified and
fixed the wrong sanity check.  In effect you're fixing the symptom and
not the cause.

The Sysbot corrupted filesystem has an xattr_ids value of 4294967071,
which as you point out is treated as negative due to xattr_ids being a
signed int.

But 4294967071 even though it is a large number could be a perfectly
legitimate number because in theory the filesystem layout supports up
to 2^32 xattr_ids.

By extending the wrong sanity check from

 >	if (*xattr_ids == 0)

to

 >	if (*xattr_ids <= 0)

You are using the fact that the number has gone negative to reject the
filesystem.  But you are not fixing the real issues.

You have not discovered if or why the negative number is the
cause of the failure, or whether there are extra flaws.

With syzkiller fuzzer generated exploits, it is essential to analyze the
issue throughly, because these exploits often rely on over-looked
dependencies/assumptions, and it can be difficult to produce a patch
that fixes all the issues and without introducing regressions.

Phillip

 >
 > Fixes: 506220d2ba21 ("squashfs: add more sanity checks in xattr id 
lookup")
 > Reported-by: syzbot+082fa4af80a5bb1a9843@syzkaller.appspotmail.com
 > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
 > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
 > ---
 >   fs/squashfs/xattr_id.c | 2 +-
 >   1 file changed, 1 insertion(+), 1 deletion(-)
 >
 > diff --git a/fs/squashfs/xattr_id.c b/fs/squashfs/xattr_id.c
 > index 087cab8c78f4..f6d78cbc3e74 100644
 > --- a/fs/squashfs/xattr_id.c
 > +++ b/fs/squashfs/xattr_id.c
 > @@ -76,7 +76,7 @@ __le64 *squashfs_read_xattr_id_table(struct 
super_block *sb, u64 table_start,
 >   	/* Sanity check values */
 >
 >   	/* there is always at least one xattr id */
 > -	if (*xattr_ids == 0)
 > +	if (*xattr_ids <= 0)
 >   		return ERR_PTR(-EINVAL);
 >
 >   	len = SQUASHFS_XATTR_BLOCK_BYTES(*xattr_ids);