linux-kernel - [PATCH] fs/jfs: avoid usage of list iterator after loop in lmPostGC()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-Id: <20230301-fs-jfs-avoid-iter-after-loop-v1-1-1742e06efc62@gmail.com>
Date:   Thu, 02 Mar 2023 00:02:46 +0100
From:   Jakob Koschel <jkl820.git@...il.com>
To:     Dave Kleikamp <shaggy@...nel.org>
Cc:     jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        Pietro Borrello <borrello@...g.uniroma1.it>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>, Jakob Koschel <jkl820.git@...il.com>
Subject: [PATCH] fs/jfs: avoid usage of list iterator after loop in
 lmPostGC()

If the list_for_each_entry_safe() iteration never breaks, 'tblk' would
contain an invalid pointer past the iterator loop. To ensure 'tblk' is
always valid, we only set it if the correct element was found. That
allows adding a BUG_ON in case the code works incorrectly, exposing
currently undetectable potential bugs.

Additionally, Linus proposed to avoid any use of the list iterator
variable after the loop, in the attempt to move the list iterator
variable declaration into the marcro to avoid any potential misuse after
the loop [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jkl820.git@...il.com>
---
I've found this code path looking at the output of the
use_after_iter.cocci script. From an outsider perspective it's difficult
for me to judge if the condition '!(tblk->flag & tblkGC_COMMIT)' is
guaranteed to be true for at least one entry in the list.

If not then the access 'tblk->bp->l_wqnext' in line 887 will not work
with a valid 'tblk' entry.

For now I've replaced the iterator variable with 'iter' and only set the
'tblk' (used after the iteration), in the break case where it is
guaranteed to be correct.

I'm happy to get some input on this and open for any suggestion.
---
 fs/jfs/jfs_logmgr.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 695415cbfe98..e3ca0bb6dbd5 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -804,7 +804,7 @@ static void lmPostGC(struct lbuf * bp)
 	unsigned long flags;
 	struct jfs_log *log = bp->l_log;
 	struct logpage *lp;
-	struct tblock *tblk, *temp;
+	struct tblock *tblk = NULL, *iter, *temp;
 
 	//LOGGC_LOCK(log);
 	spin_lock_irqsave(&log->gclock, flags);
@@ -814,54 +814,56 @@ static void lmPostGC(struct lbuf * bp)
 	 * remove/wakeup transactions from commit queue who were
 	 * group committed with the current log page
 	 */
-	list_for_each_entry_safe(tblk, temp, &log->cqueue, cqueue) {
-		if (!(tblk->flag & tblkGC_COMMIT))
+	list_for_each_entry_safe(iter, temp, &log->cqueue, cqueue) {
+		if (!(iter->flag & tblkGC_COMMIT)) {
+			tblk = iter;
 			break;
+		}
 		/* if transaction was marked GC_COMMIT then
 		 * it has been shipped in the current pageout
 		 * and made it to disk - it is committed.
 		 */
 
 		if (bp->l_flag & lbmERROR)
-			tblk->flag |= tblkGC_ERROR;
+			iter->flag |= tblkGC_ERROR;
 
 		/* remove it from the commit queue */
-		list_del(&tblk->cqueue);
-		tblk->flag &= ~tblkGC_QUEUE;
+		list_del(&iter->cqueue);
+		iter->flag &= ~tblkGC_QUEUE;
 
-		if (tblk == log->flush_tblk) {
+		if (iter == log->flush_tblk) {
 			/* we can stop flushing the log now */
 			clear_bit(log_FLUSH, &log->flag);
 			log->flush_tblk = NULL;
 		}
 
-		jfs_info("lmPostGC: tblk = 0x%p, flag = 0x%x", tblk,
-			 tblk->flag);
+		jfs_info("lmPostGC: tblk = 0x%p, flag = 0x%x", iter,
+			 iter->flag);
 
-		if (!(tblk->xflag & COMMIT_FORCE))
+		if (!(iter->xflag & COMMIT_FORCE))
 			/*
-			 * Hand tblk over to lazy commit thread
+			 * Hand iter over to lazy commit thread
 			 */
-			txLazyUnlock(tblk);
+			txLazyUnlock(iter);
 		else {
 			/* state transition: COMMIT -> COMMITTED */
-			tblk->flag |= tblkGC_COMMITTED;
+			iter->flag |= tblkGC_COMMITTED;
 
-			if (tblk->flag & tblkGC_READY)
+			if (iter->flag & tblkGC_READY)
 				log->gcrtc--;
 
-			LOGGC_WAKEUP(tblk);
+			LOGGC_WAKEUP(iter);
 		}
 
 		/* was page full before pageout ?
 		 * (and this is the last tblk bound with the page)
 		 */
-		if (tblk->flag & tblkGC_FREE)
+		if (iter->flag & tblkGC_FREE)
 			lbmFree(bp);
 		/* did page become full after pageout ?
 		 * (and this is the last tblk bound with the page)
 		 */
-		else if (tblk->flag & tblkGC_EOP) {
+		else if (iter->flag & tblkGC_EOP) {
 			/* finalize the page */
 			lp = (struct logpage *) bp->l_ldata;
 			bp->l_ceor = bp->l_eor;
@@ -880,6 +882,7 @@ static void lmPostGC(struct lbuf * bp)
 	 * select the latest ready transaction as new group leader and
 	 * wake her up to lead her group.
 	 */
+	BUG_ON(!tblk);
 	if ((!list_empty(&log->cqueue)) &&
 	    ((log->gcrtc > 0) || (tblk->bp->l_wqnext != NULL) ||
 	     test_bit(log_FLUSH, &log->flag) || jfs_tlocks_low))

---
base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
change-id: 20230301-fs-jfs-avoid-iter-after-loop-82a17102e548

Best regards,
-- 
Jakob Koschel <jkl820.git@...il.com>