linux-kernel - Re: [PATCH] jfs: Fix use-after-free read issue in jfs

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>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <33eea214-56ee-47da-89a7-b4375a5e860c@oracle.com>
Date: Wed, 30 Oct 2024 09:30:28 -0500
From: Dave Kleikamp <dave.kleikamp@...cle.com>
To: Qianqiang Liu <qianqiang.liu@....com>
Cc: jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        syzbot+885a4f3281b8d99c48d8@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] jfs: Fix use-after-free read issue in jfs_lazycommit

On 10/13/24 1:05AM, Qianqiang Liu wrote:
> The jfsCommit kernel thread uses the sbi->commit_state flag,
> and sbi may be freed in jfs_put_super() by another thread.
> 
> To prevent this, move commit_state to struct tblock,
> eliminating the need to access the sbi variable.

I need to give this one some more thought. The unmount isn't supposed to 
complete before all I/O has completed, but it's been quite I while since 
I went over the mechanisms to safeguard that. I'll have to look at this 
problem more closely.

Shaggy

> 
> Reported-by: syzbot+885a4f3281b8d99c48d8@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=885a4f3281b8d99c48d8
> Tested-by: syzbot+885a4f3281b8d99c48d8@...kaller.appspotmail.com
> Signed-off-by: Qianqiang Liu <qianqiang.liu@....com>
> ---
>   fs/jfs/jfs_incore.h |  8 --------
>   fs/jfs/jfs_txnmgr.c | 10 ++++------
>   fs/jfs/jfs_txnmgr.h |  8 ++++++++
>   3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
> index 10934f9a11be..7b75c801b239 100644
> --- a/fs/jfs/jfs_incore.h
> +++ b/fs/jfs/jfs_incore.h
> @@ -177,11 +177,6 @@ struct jfs_sb_info {
>   	pxd_t		ait2;		/* pxd describing AIT copy	*/
>   	uuid_t		uuid;		/* 128-bit uuid for volume	*/
>   	uuid_t		loguuid;	/* 128-bit uuid for log	*/
> -	/*
> -	 * commit_state is used for synchronization of the jfs_commit
> -	 * threads.  It is protected by LAZY_LOCK().
> -	 */
> -	int		commit_state;	/* commit state */
>   	/* Formerly in ipimap */
>   	uint		gengen;		/* inode generation generator*/
>   	uint		inostamp;	/* shows inode belongs to fileset*/
> @@ -199,9 +194,6 @@ struct jfs_sb_info {
>   	uint		minblks_trim;	/* minimum blocks, for online trim */
>   };
>   
> -/* jfs_sb_info commit_state */
> -#define IN_LAZYCOMMIT 1
> -
>   static inline struct jfs_inode_info *JFS_IP(struct inode *inode)
>   {
>   	return container_of(inode, struct jfs_inode_info, vfs_inode);
> diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
> index be17e3c43582..a4817229d573 100644
> --- a/fs/jfs/jfs_txnmgr.c
> +++ b/fs/jfs/jfs_txnmgr.c
> @@ -2700,7 +2700,6 @@ int jfs_lazycommit(void *arg)
>   	int WorkDone;
>   	struct tblock *tblk;
>   	unsigned long flags;
> -	struct jfs_sb_info *sbi;
>   
>   	set_freezable();
>   	do {
> @@ -2711,17 +2710,16 @@ int jfs_lazycommit(void *arg)
>   			list_for_each_entry(tblk, &TxAnchor.unlock_queue,
>   					    cqueue) {
>   
> -				sbi = JFS_SBI(tblk->sb);
>   				/*
>   				 * For each volume, the transactions must be
>   				 * handled in order.  If another commit thread
>   				 * is handling a tblk for this superblock,
>   				 * skip it
>   				 */
> -				if (sbi->commit_state & IN_LAZYCOMMIT)
> +				if (tblk->commit_state & IN_LAZYCOMMIT)
>   					continue;
>   
> -				sbi->commit_state |= IN_LAZYCOMMIT;
> +				tblk->commit_state |= IN_LAZYCOMMIT;
>   				WorkDone = 1;
>   
>   				/*
> @@ -2733,7 +2731,7 @@ int jfs_lazycommit(void *arg)
>   				txLazyCommit(tblk);
>   				LAZY_LOCK(flags);
>   
> -				sbi->commit_state &= ~IN_LAZYCOMMIT;
> +				tblk->commit_state &= ~IN_LAZYCOMMIT;
>   				/*
>   				 * Don't continue in the for loop.  (We can't
>   				 * anyway, it's unsafe!)  We want to go back to
> @@ -2781,7 +2779,7 @@ void txLazyUnlock(struct tblock * tblk)
>   	 * Don't wake up a commit thread if there is already one servicing
>   	 * this superblock, or if the last one we woke up hasn't started yet.
>   	 */
> -	if (!(JFS_SBI(tblk->sb)->commit_state & IN_LAZYCOMMIT) &&
> +	if (!(tblk->commit_state & IN_LAZYCOMMIT) &&
>   	    !jfs_commit_thread_waking) {
>   		jfs_commit_thread_waking = 1;
>   		wake_up(&jfs_commit_thread_wait);
> diff --git a/fs/jfs/jfs_txnmgr.h b/fs/jfs/jfs_txnmgr.h
> index ba71eb5ced56..3a0ee53f17cb 100644
> --- a/fs/jfs/jfs_txnmgr.h
> +++ b/fs/jfs/jfs_txnmgr.h
> @@ -32,6 +32,11 @@ struct tblock {
>   
>   	/* lock management */
>   	struct super_block *sb;	/* super block */
> +	/*
> +	 * commit_state is used for synchronization of the jfs_commit
> +	 * threads.  It is protected by LAZY_LOCK().
> +	 */
> +	int commit_state;	/* commit state */
>   	lid_t next;		/* index of first tlock of tid */
>   	lid_t last;		/* index of last tlock of tid */
>   	wait_queue_head_t waitor;	/* tids waiting on this tid */
> @@ -56,6 +61,9 @@ struct tblock {
>   	u32 ino;		/* inode number being created */
>   };
>   
> +/* tblock commit_state */
> +#define IN_LAZYCOMMIT 1
> +
>   extern struct tblock *TxBlock;	/* transaction block table */
>   
>   /* commit flags: tblk->xflag */