[PATCH 0/1] squashfs: harden sanity check in squashfs_read_xattr_id_table

[PATCH 0/1] squashfs: harden sanity check in squashfs_read_xattr_id_table

[Fedor Pchelkin]

Hello,

Syzkaller reports the following problem [1].

null-ptr-deref in cache_first_page() happens because 'data[i]' is not
properly initialized in squashfs_read_table(): this is due to its 'length'
argument being zero (thus causing 'pages' being zero, too).
squashfs_read_table() is called twice from squashfs_read_xattr_id_table().
After the first call, 'id_table->xattr_ids' signed value is processed: if
a filesystem is corrupted, I suppose, a negative value is written to it
(in copy_bio_to_actor(), as I understand). So then 'len' and 'indexes'
values become incorrect (in this case, equal to zero), and it causes the
bug. I've added the additional check for '*xattr_ids' not being negative.

But, actually, I don't quite understand why 'xattr_ids' field has
different types in 'struct squashfs_xattr_id_table' and
'struct squashfs_sb_info'. Probably that would be an issue. For example,
in squashfs_xattr_lookup() we compare unsigned 'index' with signed
'msblk->xattr_ids'. So maybe convert 'xattr_ids' field in
'struct squashfs_sb_info' to unsigned type? And that, I think, would fix
the problem described in the patch as well.

[1] https://syzkaller.appspot.com/bug?id=8cf937ba2cde11be769fe8a1330c8d9153e3d7fa

--
Regards,

Fedor

[PATCH 1/1] squashfs: harden sanity check in squashfs_read_xattr_id_table

[Fedor Pchelkin]

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().

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

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

Re: [PATCH 1/1] squashfs: harden sanity check in squashfs_read_xattr_id_table

[Phillip Lougher]

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

Re: [PATCH 1/1] squashfs: harden sanity check in squashfs_read_xattr_id_table

[Fedor Pchelkin]

Thanks for the reply. Actually, I made a proposal about '*xattr_ds' type
being unsigned in 0/1 patch of the series, but in the end tried to fix
the issue in-place, and that was definitely a wrong decision. I'm sorry
for the misleading patch.

On 27.01.2023 09:20, Phillip Lougher wrote:
> 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);