Messages in this thread | | | Date | Fri, 27 Jan 2023 06:20:18 +0000 | Subject | Re: [PATCH 1/1] squashfs: harden sanity check in squashfs_read_xattr_id_table | From | 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);
| |