linux-kernel - Re: [PATCH resend] jfs: fix use-after-free in lbmIODone

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: <YF3oDrnN3RJBFQ0P@kroah.com>
Date:   Fri, 26 Mar 2021 14:56:30 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Anirudh Rayabharam <mail@...rudhrb.com>
Cc:     shaggy@...nel.org, jfs-discussion@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+5d2008bd1f1b722ba94e@...kaller.appspotmail.com,
        Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH resend] jfs: fix use-after-free in lbmIODone

On Mon, Mar 22, 2021 at 09:41:47PM +0530, Anirudh Rayabharam wrote:
> Fix use-after-free by waiting for ongoing IO to complete before freeing
> lbufs in lbmLogShutdown. Add a counter in struct jfs_log to keep track
> of the number of in-flight IO operations and a wait queue to wait on for
> the IO operations to complete.
> 
> Reported-by: syzbot+5d2008bd1f1b722ba94e@...kaller.appspotmail.com
> Suggested-by: Hillf Danton <hdanton@...a.com>
> Signed-off-by: Anirudh Rayabharam <mail@...rudhrb.com>
> ---
>  fs/jfs/jfs_logmgr.c | 17 ++++++++++++++---
>  fs/jfs/jfs_logmgr.h |  2 ++
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
> index 9330eff210e0..82d20c4687aa 100644
> --- a/fs/jfs/jfs_logmgr.c
> +++ b/fs/jfs/jfs_logmgr.c
> @@ -1815,6 +1815,8 @@ static int lbmLogInit(struct jfs_log * log)
>  	 */
>  	init_waitqueue_head(&log->free_wait);
>  
> +	init_waitqueue_head(&log->io_waitq);
> +
>  	log->lbuf_free = NULL;
>  
>  	for (i = 0; i < LOGPAGES;) {
> @@ -1864,6 +1866,7 @@ static void lbmLogShutdown(struct jfs_log * log)
>  	struct lbuf *lbuf;
>  
>  	jfs_info("lbmLogShutdown: log:0x%p", log);
> +	wait_event(log->io_waitq, !atomic_read(&log->io_inflight));
>  
>  	lbuf = log->lbuf_free;
>  	while (lbuf) {
> @@ -1990,6 +1993,8 @@ static int lbmRead(struct jfs_log * log, int pn, struct lbuf ** bpp)
>  	bio->bi_end_io = lbmIODone;
>  	bio->bi_private = bp;
>  	bio->bi_opf = REQ_OP_READ;
> +
> +	atomic_inc(&log->io_inflight);
>  	/*check if journaling to disk has been disabled*/
>  	if (log->no_integrity) {
>  		bio->bi_iter.bi_size = 0;
> @@ -2135,6 +2140,7 @@ static void lbmStartIO(struct lbuf * bp)
>  	bio->bi_private = bp;
>  	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC;
>  
> +	atomic_inc(&log->io_inflight);

Why use an atomic for this?  The value can change after you test for it,
as there's no lock involved.

Do you really need to keep track of all of these "inflight"?  That feels
very "heavy" to me.

jfs developers, any ideas?

thanks,

greg k-h