2023-01-17 11:17:26

by Fedor Pchelkin

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

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: [email protected]
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
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


2023-01-27 06:24:10

by Phillip Lougher

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

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: [email protected]
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> ---
> 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);

2023-01-27 07:24:31

by Fedor Pchelkin

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

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: [email protected]
> > Signed-off-by: Fedor Pchelkin <[email protected]>
> > Signed-off-by: Alexey Khoroshilov <[email protected]>
> > ---
> >   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);