linux-ext4 - Re: [PATCH] jbd2: Fix use-after-free of transaction

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Apr 2022 13:17:26 +0200
From:   Jan Kara <jack@...e.cz>
To:     Samuel Mendoza-Jonas <samjonas@...zon.com>
Cc:     Ritesh Harjani <riteshh@...ux.ibm.com>, linux-ext4@...r.kernel.org,
        Jan Kara <jack@...e.cz>,
        syzbot+afa2ca5171d93e44b348@...kaller.appspotmail.com
Subject: Re: [PATCH] jbd2: Fix use-after-free of transaction_t race

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@...kaller.appspotmail.com
> > Signed-off-by: Ritesh Harjani <riteshh@...ux.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...

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