[PATCH v2] jfs: fix out-of-bounds write in add_missing_indices()

[PATCH v2] jfs: fix out-of-bounds write in add_missing_indices()

[Wang Yibo]

add_missing_indices() walks the sorted entry table of a directory dtree
page and rewrites entries whose index is out of range.  The loop
iterates up to p->header.nextindex (a u8 read from disk, up to 255) and
indexes p->slot[stbl[i]] with only a lower-bound check (stbl[i] < 0).

On a corrupted filesystem stbl[i] can be any byte value, so d->index
can be written far beyond the end of the dtree page.  For the root
directory this is worse: the dtree root lives inline in struct
jfs_inode_info (320 bytes with only 9 slots), so the write lands inside
the same slab object, corrupting arbitrary fields of the embedded
struct inode including i_security.  KASAN cannot see it because the
write stays within the jfs_inode_info allocation.

This is reachable from getdents(2) on a corrupted JFS image and was
observed as a GPF in bpf_inode_storage_free() reading the garbage
i_security pointer on inode eviction.

Validate stbl[i] against the page's slot count: DTROOTMAXSLOT for the
inline root page, and min(p->header.maxslot, DTPAGEMAXSLOT) for regular
pages (maxslot itself comes from disk and cannot be trusted either).

Three other open reports are the same bug rather than separate ones.
Two of them die in smk_of_inode() on a crafted inode->i_security, with
only the upper half of the pointer replaced by 0x0000000c:

	general protection fault in smack_inode_getattr
	general protection fault in smack_inode_permission
	general protection fault in smack_file_ioctl

That is a single 4-byte store at i_security + 4.  In struct
jfs_inode_info the _dtroot union sits at offset 728, its slots are 32
bytes with the index field at offset 28, and the embedded struct inode
starts at 1048, so i_security is at 1104 and slot[11].index lands
exactly on i_security + 4.  The value stored there is the directory's
next_index, 12.  The only code that stores next_index through an
unchecked slot index is this function.

Checked on a SMACK enabled build with the reproducer from the first
report: without the patch it dies in smack_inode_permission() called
from ksys_umount() with i_security = 0x0000000c27a22900, and with the
patch the invalid entry is rejected with -EIO and the directory scan
completes.

Fixes: 5dff41a86377 ("jfs: fix array-index-out-of-bounds read in add_missing_indices")
Reported-by: syzbot+b547ffac63b636628299@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b547ffac63b636628299
Reported-by: syzbot+ce678add490179a73d5a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ce678add490179a73d5a
Reported-by: syzbot+fc1d5d9a7ebabbc27458@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fc1d5d9a7ebabbc27458
Reported-by: syzbot+23d1112901d0870bb3e7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=23d1112901d0870bb3e7
Signed-off-by: Wang Yibo <szwyb123@gmail.com>
---
v2: add the Reported-by: lines and Closes: tags for the three
    additional reports, describe in the message how they relate to the
    first one, and add a Fixes: tag for the commit that introduced the
    lower-bound-only check.  No change to the check itself.

 fs/jfs/jfs_dtree.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 8ce6e4458cc2..119d03c410b8 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -2623,6 +2623,7 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 	s8 *stbl;
 	tid_t tid;
 	struct tlock *tlck;
+	int maxslot;
 
 	tid = txBegin(inode->i_sb, 0);
 
@@ -2636,6 +2637,11 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 
 	ASSERT(p->header.flag & BT_LEAF);
 
+	if (p->header.flag & BT_ROOT)
+		maxslot = DTROOTMAXSLOT;
+	else
+		maxslot = min_t(u8, p->header.maxslot, DTPAGEMAXSLOT);
+
 	tlck = txLock(tid, inode, mp, tlckDTREE | tlckENTRY);
 	if (BT_IS_ROOT(mp))
 		tlck->type |= tlckBTROOT;
@@ -2644,7 +2650,7 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 
 	stbl = DT_GETSTBL(p);
 	for (i = 0; i < p->header.nextindex; i++) {
-		if (stbl[i] < 0) {
+		if (stbl[i] < 0 || stbl[i] >= maxslot) {
 			jfs_err("jfs: add_missing_indices: Invalid stbl[%d] = %d for inode %ld, block = %lld",
 				i, stbl[i], (long)inode->i_ino, (long long)bn);
 			rc = -EIO;
-- 
2.25.1

[PATCH v3] jfs: fix out-of-bounds write in add_missing_indices()

[Wang Yibo]

add_missing_indices() walks the sorted entry table of a directory dtree
page and rewrites entries whose index is out of range.  The loop
iterates up to p->header.nextindex (a u8 read from disk, up to 255) and
indexes p->slot[stbl[i]] with only a lower-bound check (stbl[i] < 0).

On a corrupted filesystem stbl[i] can be any byte value, so d->index
can be written far beyond the end of the dtree page.  For the root
directory this is worse: the dtree root lives inline in struct
jfs_inode_info (320 bytes with only 9 slots), so the write lands inside
the same slab object, corrupting arbitrary fields of the embedded
struct inode including i_security.  KASAN cannot see it because the
write stays within the jfs_inode_info allocation.

This is reachable from getdents(2) on a corrupted JFS image and was
observed as a GPF in bpf_inode_storage_free() reading the garbage
i_security pointer on inode eviction.

Validate stbl[i] against the page's slot count: DTROOTMAXSLOT for the
inline root page, and min(p->header.maxslot, DTPAGEMAXSLOT) for regular
pages (maxslot itself comes from disk and cannot be trusted either).

Three other open reports are the same bug rather than separate ones.
Two of them die in smk_of_inode() on a crafted inode->i_security, with
only the upper half of the pointer replaced by 0x0000000c:

	general protection fault in smack_inode_getattr
	general protection fault in smack_inode_permission
	general protection fault in smack_file_ioctl

That is a single 4-byte store at i_security + 4.  In struct
jfs_inode_info the _dtroot union sits at offset 728, its slots are 32
bytes with the index field at offset 28, and the embedded struct inode
starts at 1048, so i_security is at 1104 and slot[11].index lands
exactly on i_security + 4.  The value stored there is the directory's
next_index, 12.  The only code that stores next_index through an
unchecked slot index is this function.

Checked on a SMACK enabled build with the reproducer from the first
report: without the patch it dies in smack_inode_permission() called
from ksys_umount() with i_security = 0x0000000c27a22900, and with the
patch the invalid entry is rejected with -EIO and the directory scan
completes.

Reported-by: syzbot+b547ffac63b636628299@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b547ffac63b636628299
Reported-by: syzbot+ce678add490179a73d5a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ce678add490179a73d5a
Reported-by: syzbot+fc1d5d9a7ebabbc27458@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fc1d5d9a7ebabbc27458
Reported-by: syzbot+23d1112901d0870bb3e7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=23d1112901d0870bb3e7
Signed-off-by: Wang Yibo <szwyb123@gmail.com>
---
v3: drop the Fixes: tag; the out-of-bounds write predates the
    commit it pointed at, so the reference was misleading.
    No change to the code.

 fs/jfs/jfs_dtree.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 8ce6e4458cc2..119d03c410b8 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -2623,6 +2623,7 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 	s8 *stbl;
 	tid_t tid;
 	struct tlock *tlck;
+	int maxslot;
 
 	tid = txBegin(inode->i_sb, 0);
 
@@ -2636,6 +2637,11 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 
 	ASSERT(p->header.flag & BT_LEAF);
 
+	if (p->header.flag & BT_ROOT)
+		maxslot = DTROOTMAXSLOT;
+	else
+		maxslot = min_t(u8, p->header.maxslot, DTPAGEMAXSLOT);
+
 	tlck = txLock(tid, inode, mp, tlckDTREE | tlckENTRY);
 	if (BT_IS_ROOT(mp))
 		tlck->type |= tlckBTROOT;
@@ -2644,7 +2650,7 @@ static int add_missing_indices(struct inode *inode, s64 bn)
 
 	stbl = DT_GETSTBL(p);
 	for (i = 0; i < p->header.nextindex; i++) {
-		if (stbl[i] < 0) {
+		if (stbl[i] < 0 || stbl[i] >= maxslot) {
 			jfs_err("jfs: add_missing_indices: Invalid stbl[%d] = %d for inode %ld, block = %lld",
 				i, stbl[i], (long)inode->i_ino, (long long)bn);
 			rc = -EIO;
-- 
2.25.1