Received-SPF: softfail (google.com: domain of transitioning linux-ext4-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Subject: Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
Date:   Wed, 27 Apr 2022 09:21:46 -0700
From:   Samuel Mendoza-Jonas <samjonas@amazon.com>
To:     Jan Kara <jack@suse.cz>
CC:     Ritesh Harjani <riteshh@linux.ibm.com>,
        <linux-ext4@vger.kernel.org>,
        <syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com>
Message-ID: <20220427162146.3nj3czri4krdpy3c@u46989501580c5c.ant.amazon.com>
References: <948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com>
 <20220426183124.phrwsl77bch5uljx@u46989501580c5c.ant.amazon.com>
 <20220427111726.3wdyxbqoxs7skdzf@quack3.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220427111726.3wdyxbqoxs7skdzf@quack3.lan>
Precedence: bulk

On Wed, Apr 27, 2022 at 01:17:26PM +0200, Jan Kara wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Tue 26-04-22 11:31:24, Samuel Mendoza-Jonas wrote:
> > On Thu, Feb 10, 2022 at 09:07:11PM +0530, Ritesh Harjani wrote:
> > > jbd2_journal_wait_updates() is called with j_state_lock held. But if
> > > there is a commit in progress, then this transaction might get committed
> > > and freed via jbd2_journal_commit_transaction() ->
> > > jbd2_journal_free_transaction(), when we release j_state_lock.
> > > So check for journal->j_running_transaction everytime we release and
> > > acquire j_state_lock to avoid use-after-free issue.
> > >
> > > Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> > > Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > Hi Ritesh,
> >
> > Looking at the refactor in the commit this fixes, I believe the same
> > issue is present prior to the refactor, so this would apply before 5.17
> > as well.
> > I've posted a backport for 4.9-4.19 and 5.4-5.16 to stable here:
> > https://lore.kernel.org/stable/20220426182702.716304-1-samjonas@amazon.com/T/#t
> >
> > Please have a look and let me know if you agree.
> 
> Actually the refactor was indeed the cause for use-after-free. The original
> code in jbd2_journal_lock_updates() was like:
> 
>        /* Wait until there are no running updates */
>        while (1) {
>                transaction_t *transaction = journal->j_running_transaction;
> 
>                if (!transaction)
>                        break;
>                spin_lock(&transaction->t_handle_lock);
>                prepare_to_wait(&journal->j_wait_updates, &wait,
>                                TASK_UNINTERRUPTIBLE);
>                if (!atomic_read(&transaction->t_updates)) {
>                        spin_unlock(&transaction->t_handle_lock);
>                        finish_wait(&journal->j_wait_updates, &wait);
>                        break;
>                }
>                spin_unlock(&transaction->t_handle_lock);
>                write_unlock(&journal->j_state_lock);
>                schedule();
>                finish_wait(&journal->j_wait_updates, &wait);
>                write_lock(&journal->j_state_lock);
>        }
> 
> So you can see the code was indeed careful enough to not touch
> t_handle_lock after sleeping. The code in jbd2_journal_commit_transaction()
> did touch t_handle_lock but there it didn't matter because nobody else
> besides the task running jbd2_journal_commit_transaction() can free the
> transaction...
> 

Right you are, I misinterpreted the interaction with
jbd2_journal_commit_transaction(). Thanks for verifying, I'll follow up
stable to disregard those backports.

Cheers,
Sam

>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR