From 3bef198f1b17d1bb89260bad947ef084c0a2d1a6 Mon Sep 17 00:00:00 2001 From: Randy Dunlap Date: Fri, 18 Dec 2020 12:17:16 -0800 Subject: [PATCH 1/3] JFS: more checks for invalid superblock syzbot is feeding invalid superblock data to JFS for mount testing. JFS does not check several of the fields -- just assumes that they are good since the JFS_MAGIC and version fields are good. In this case (syzbot reproducer), we have s_l2bsize == 0xda0c, pad == 0xf045, and s_state == 0x50, all of which are invalid IMO. Having s_l2bsize == 0xda0c causes this UBSAN warning: UBSAN: shift-out-of-bounds in fs/jfs/jfs_mount.c:373:25 shift exponent -9716 is negative s_l2bsize can be tested for correctness. pad can be tested for non-0 and punted. s_state can be tested for its valid values and punted. Do those 3 tests and if any of them fails, report the superblock as invalid/corrupt and let fsck handle it. With this patch, chkSuper() says this when JFS_DEBUG is enabled: jfs_mount: Mount Failure: superblock is corrupt! Mount JFS Failure: -22 jfs_mount failed w/return code = -22 The obvious problem with this method is that next week there could be another syzbot test that uses different fields for invalid values, this making this like a game of whack-a-mole. syzkaller link: https://syzkaller.appspot.com/bug?extid=36315852ece4132ec193 Reported-by: syzbot+36315852ece4132ec193@syzkaller.appspotmail.com Reported-by: kernel test robot # v2 Signed-off-by: Randy Dunlap Signed-off-by: Dave Kleikamp Cc: jfs-discussion@lists.sourceforge.net --- fs/jfs/jfs_filsys.h | 1 + fs/jfs/jfs_mount.c | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/fs/jfs/jfs_filsys.h b/fs/jfs/jfs_filsys.h index 1e899298f7f0..b5d702df7111 100644 --- a/fs/jfs/jfs_filsys.h +++ b/fs/jfs/jfs_filsys.h @@ -268,5 +268,6 @@ * fsck() must be run to repair */ #define FM_EXTENDFS 0x00000008 /* file system extendfs() in progress */ +#define FM_STATE_MAX 0x0000000f /* max value of s_state */ #endif /* _H_JFS_FILSYS */ diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c index 2935d4c776ec..5d7d7170c03c 100644 --- a/fs/jfs/jfs_mount.c +++ b/fs/jfs/jfs_mount.c @@ -37,6 +37,7 @@ #include #include #include +#include #include "jfs_incore.h" #include "jfs_filsys.h" @@ -366,6 +367,15 @@ static int chkSuper(struct super_block *sb) sbi->bsize = bsize; sbi->l2bsize = le16_to_cpu(j_sb->s_l2bsize); + /* check some fields for possible corruption */ + if (sbi->l2bsize != ilog2((u32)bsize) || + j_sb->pad != 0 || + le32_to_cpu(j_sb->s_state) > FM_STATE_MAX) { + rc = -EINVAL; + jfs_err("jfs_mount: Mount Failure: superblock is corrupt!"); + goto out; + } + /* * For now, ignore s_pbsize, l2bfactor. All I/O going through buffer * cache. From 093e0687c5baacc29e4e8dd3ea205bac518e38bc Mon Sep 17 00:00:00 2001 From: Yang Li Date: Sun, 7 Feb 2021 16:32:50 +0800 Subject: [PATCH 2/3] jfs: turn diLog(), dataLog() and txLog() into void functions These functions always return '0' and no callers use the return value. So make it a void function. This eliminates the following coccicheck warning: ./fs/jfs/jfs_txnmgr.c:1365:5-7: Unneeded variable: "rc". Return "0" on line 1414 ./fs/jfs/jfs_txnmgr.c:1422:5-7: Unneeded variable: "rc". Return "0" on line 1527 Reported-by: Abaci Robot Signed-off-by: Yang Li Signed-off-by: Dave Kleikamp --- fs/jfs/jfs_txnmgr.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c index dca8edd2378c..053295cd7bc6 100644 --- a/fs/jfs/jfs_txnmgr.c +++ b/fs/jfs/jfs_txnmgr.c @@ -148,10 +148,10 @@ static struct { /* * forward references */ -static int diLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, - struct tlock * tlck, struct commit * cd); -static int dataLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, - struct tlock * tlck); +static void diLog(struct jfs_log *log, struct tblock *tblk, struct lrd *lrd, + struct tlock *tlck, struct commit *cd); +static void dataLog(struct jfs_log *log, struct tblock *tblk, struct lrd *lrd, + struct tlock *tlck); static void dtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, struct tlock * tlck); static void mapLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, @@ -159,8 +159,8 @@ static void mapLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, static void txAllocPMap(struct inode *ip, struct maplock * maplock, struct tblock * tblk); static void txForce(struct tblock * tblk); -static int txLog(struct jfs_log * log, struct tblock * tblk, - struct commit * cd); +static void txLog(struct jfs_log *log, struct tblock *tblk, + struct commit *cd); static void txUpdateMap(struct tblock * tblk); static void txRelease(struct tblock * tblk); static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, @@ -1256,8 +1256,7 @@ int txCommit(tid_t tid, /* transaction identifier */ * * txUpdateMap() resets XAD_NEW in XAD. */ - if ((rc = txLog(log, tblk, &cd))) - goto TheEnd; + txLog(log, tblk, &cd); /* * Ensure that inode isn't reused before @@ -1365,9 +1364,8 @@ int txCommit(tid_t tid, /* transaction identifier */ * * RETURN : */ -static int txLog(struct jfs_log * log, struct tblock * tblk, struct commit * cd) +static void txLog(struct jfs_log *log, struct tblock *tblk, struct commit *cd) { - int rc = 0; struct inode *ip; lid_t lid; struct tlock *tlck; @@ -1414,7 +1412,7 @@ static int txLog(struct jfs_log * log, struct tblock * tblk, struct commit * cd) } } - return rc; + return; } /* @@ -1422,10 +1420,9 @@ static int txLog(struct jfs_log * log, struct tblock * tblk, struct commit * cd) * * function: log inode tlock and format maplock to update bmap; */ -static int diLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, - struct tlock * tlck, struct commit * cd) +static void diLog(struct jfs_log *log, struct tblock *tblk, struct lrd *lrd, + struct tlock *tlck, struct commit *cd) { - int rc = 0; struct metapage *mp; pxd_t *pxd; struct pxd_lock *pxdlock; @@ -1527,7 +1524,7 @@ static int diLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, } #endif /* _JFS_WIP */ - return rc; + return; } /* @@ -1535,8 +1532,8 @@ static int diLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, * * function: log data tlock */ -static int dataLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, - struct tlock * tlck) +static void dataLog(struct jfs_log *log, struct tblock *tblk, struct lrd *lrd, + struct tlock *tlck) { struct metapage *mp; pxd_t *pxd; @@ -1562,7 +1559,7 @@ static int dataLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, metapage_homeok(mp); discard_metapage(mp); tlck->mp = NULL; - return 0; + return; } PXDaddress(pxd, mp->index); @@ -1573,7 +1570,7 @@ static int dataLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd, /* mark page as homeward bound */ tlck->flag |= tlckWRITEPAGE; - return 0; + return; } /* From 4208c398aae4c2290864ba15c3dab7111f32bec1 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Thu, 11 Feb 2021 13:01:08 +0000 Subject: [PATCH 3/3] fs/jfs: fix potential integer overflow on shift of a int The left shift of int 32 bit integer constant 1 is evaluated using 32 bit arithmetic and then assigned to a signed 64 bit integer. In the case where l2nb is 32 or more this can lead to an overflow. Avoid this by shifting the value 1LL instead. Addresses-Coverity: ("Uninitentional integer overflow") Fixes: b40c2e665cd5 ("fs/jfs: TRIM support for JFS Filesystem") Signed-off-by: Colin Ian King Signed-off-by: Dave Kleikamp --- fs/jfs/jfs_dmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c index 94b7c1cb5ceb..7aee15608619 100644 --- a/fs/jfs/jfs_dmap.c +++ b/fs/jfs/jfs_dmap.c @@ -1656,7 +1656,7 @@ s64 dbDiscardAG(struct inode *ip, int agno, s64 minlen) } else if (rc == -ENOSPC) { /* search for next smaller log2 block */ l2nb = BLKSTOL2(nblocks) - 1; - nblocks = 1 << l2nb; + nblocks = 1LL << l2nb; } else { /* Trim any already allocated blocks */ jfs_error(bmp->db_ipbmap->i_sb, "-EIO\n");