jfs: reserve the header and use freelist from second

Commit Message

Edward Adam Davis April 10, 2024, 7:05 a.m. UTC

[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: [e-mail]
Signed-off-by: Edward Adam Davis <[e-mail]>
---
 fs/jfs/jfs_dtree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Dave Kleikamp April 10, 2024, 4:58 p.m. UTC | #1

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: [e-mail]
> Signed-off-by: Edward Adam Davis <[e-mail]>
> ---
>   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

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;