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

[Fedor Pchelkin <pchelkin@ispras.ru>]

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