jfs: reserve the header and use freelist from second

[PATCH] jfs: reserve the header and use freelist from second

Posted by Edward Adam Davis 1 year, 10 months ago

[syzbot reported]
general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN PTI
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
CPU: 0 PID: 5061 Comm: syz-executor404 Not tainted 6.8.0-syzkaller-08951-gfe46a7dd189e #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
RIP: 0010:dtInsertEntry+0xd0c/0x1780 fs/jfs/jfs_dtree.c:3713
...
[Analyze]
When the pointer h has the same value as p, after writing name in UniStrncpy_to_le(),
p->header.flag will be cleared.
This will cause the previously true judgment "p->header.flag & BT-LEAF" to change
to no after writing the name operation, this leads to entering an incorrect branch
and accessing the uninitialized object ih when judging this condition for the
second time.
[Fix]
When allocating slots from the freelist, we start from the second one to preserve
the header of p from being incorrectly modified.

Reported-by: syzbot+bba84aef3a26fb93deb9@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/jfs/jfs_dtree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 031d8f570f58..deb2a5cc78d8 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -3618,7 +3618,8 @@ static void dtInsertEntry(dtpage_t * p, int index, struct component_name * key,
 	kname = key->name;
 
 	/* allocate a free slot */
-	hsi = fsi = p->header.freelist;
+	hsi = fsi = p->header.freelist = p->header.freelist == 0 ? 
+		1 : p->header.freelist;
 	h = &p->slot[fsi];
 	p->header.freelist = h->next;
 	--p->header.freecnt;
-- 
2.43.0

Re: [Jfs-discussion] [PATCH] jfs: reserve the header and use freelist from second

Posted by Dave Kleikamp 1 year, 10 months ago

On 4/10/24 2:05AM, Edward Adam Davis via Jfs-discussion wrote:
> [syzbot reported]
> general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN PTI
> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> CPU: 0 PID: 5061 Comm: syz-executor404 Not tainted 6.8.0-syzkaller-08951-gfe46a7dd189e #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
> RIP: 0010:dtInsertEntry+0xd0c/0x1780 fs/jfs/jfs_dtree.c:3713
> ...
> [Analyze]
> When the pointer h has the same value as p, after writing name in UniStrncpy_to_le(),
> p->header.flag will be cleared.
> This will cause the previously true judgment "p->header.flag & BT-LEAF" to change
> to no after writing the name operation, this leads to entering an incorrect branch
> and accessing the uninitialized object ih when judging this condition for the
> second time.
> [Fix]
> When allocating slots from the freelist, we start from the second one to preserve
> the header of p from being incorrectly modified.

The freelist is simply corrupted. It should never be set to 0. We cannot 
assume that slot[1] is on the free list. Probably the best we can do is 
to add more sanity checking to the freelist value, and/or any slot's 
next & prev value. That could potentially involve a lot more checks. 
I've been accepting patches here and there for specific syzbot failures, 
but any comprehensive sanity checking of every data structure would be a 
huge effort.

What makes a fix a little more difficult is that dtInsertEntry returns 
void and it would be difficult to gracefully recover. One could change 
it to return an error and have all the callers check that. But I'm 
afraid, just using a valid slot number would only lead to further data 
corruption.

Thanks,
Shaggy

> 
> Reported-by: syzbot+bba84aef3a26fb93deb9@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>   fs/jfs/jfs_dtree.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
> index 031d8f570f58..deb2a5cc78d8 100644
> --- a/fs/jfs/jfs_dtree.c
> +++ b/fs/jfs/jfs_dtree.c
> @@ -3618,7 +3618,8 @@ static void dtInsertEntry(dtpage_t * p, int index, struct component_name * key,
>   	kname = key->name;
>   
>   	/* allocate a free slot */
> -	hsi = fsi = p->header.freelist;
> +	hsi = fsi = p->header.freelist = p->header.freelist == 0 ?
> +		1 : p->header.freelist;
>   	h = &p->slot[fsi];
>   	p->header.freelist = h->next;
>   	--p->header.freecnt;

[PATCH V2] jfs: fix null ptr deref in dtInsertEntry

Posted by Edward Adam Davis 1 year, 10 months ago

[syzbot reported]
general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN PTI
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
CPU: 0 PID: 5061 Comm: syz-executor404 Not tainted 6.8.0-syzkaller-08951-gfe46a7dd189e #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
RIP: 0010:dtInsertEntry+0xd0c/0x1780 fs/jfs/jfs_dtree.c:3713
...
[Analyze]
In dtInsertEntry(), when the pointer h has the same value as p, after writing
name in UniStrncpy_to_le(), p->header.flag will be cleared. This will cause the
previously true judgment "p->header.flag & BT-LEAF" to change to no after writing
the name operation, this leads to entering an incorrect branch and accessing the
uninitialized object ih when judging this condition for the second time.

[Fix]
After got the page, check freelist first, if freelist == 0 then exit dtInsert()
and return -EINVAL.

Reported-by: syzbot+bba84aef3a26fb93deb9@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/jfs/jfs_dtree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 031d8f570f58..5d3127ca68a4 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -834,6 +834,8 @@ int dtInsert(tid_t tid, struct inode *ip,
 	 * the full page.
 	 */
 	DT_GETSEARCH(ip, btstack->top, bn, mp, p, index);
+	if (p->header.freelist == 0)
+		return -EINVAL;
 
 	/*
 	 *	insert entry for new key
-- 
2.43.0

Re: [PATCH V2] jfs: fix null ptr deref in dtInsertEntry

Posted by Dave Kleikamp 1 year, 7 months ago

Finally catching up on some neglected patches.

Looks good to me. Applied.

Shaggy

On 4/11/24 7:05AM, Edward Adam Davis wrote:
> [syzbot reported]
> general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN PTI
> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> CPU: 0 PID: 5061 Comm: syz-executor404 Not tainted 6.8.0-syzkaller-08951-gfe46a7dd189e #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
> RIP: 0010:dtInsertEntry+0xd0c/0x1780 fs/jfs/jfs_dtree.c:3713
> ...
> [Analyze]
> In dtInsertEntry(), when the pointer h has the same value as p, after writing
> name in UniStrncpy_to_le(), p->header.flag will be cleared. This will cause the
> previously true judgment "p->header.flag & BT-LEAF" to change to no after writing
> the name operation, this leads to entering an incorrect branch and accessing the
> uninitialized object ih when judging this condition for the second time.
> 
> [Fix]
> After got the page, check freelist first, if freelist == 0 then exit dtInsert()
> and return -EINVAL.
> 
> Reported-by: syzbot+bba84aef3a26fb93deb9@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>   fs/jfs/jfs_dtree.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
> index 031d8f570f58..5d3127ca68a4 100644
> --- a/fs/jfs/jfs_dtree.c
> +++ b/fs/jfs/jfs_dtree.c
> @@ -834,6 +834,8 @@ int dtInsert(tid_t tid, struct inode *ip,
>   	 * the full page.
>   	 */
>   	DT_GETSEARCH(ip, btstack->top, bn, mp, p, index);
> +	if (p->header.freelist == 0)
> +		return -EINVAL;
>   
>   	/*
>   	 *	insert entry for new key