Re: [PATCH] jbd2: Fix use-after-free of transaction_t race

[Samuel Mendoza-Jonas <samjonas@amazon.com>]

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.

Cheers,
Sam Mendoza-Jonas


> ---
>  fs/jbd2/transaction.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 8e2f8275a253..259e00046a8b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -842,27 +842,38 @@ EXPORT_SYMBOL(jbd2_journal_restart);
>   */
>  void jbd2_journal_wait_updates(journal_t *journal)
>  {
> -	transaction_t *commit_transaction = journal->j_running_transaction;
> +	DEFINE_WAIT(wait);
> 
> -	if (!commit_transaction)
> -		return;
> +	while (1) {
> +		/*
> +		 * Note that the running transaction can get freed under us if
> +		 * this transaction is getting committed in
> +		 * jbd2_journal_commit_transaction() ->
> +		 * jbd2_journal_free_transaction(). This can only happen when we
> +		 * release j_state_lock -> schedule() -> acquire j_state_lock.
> +		 * Hence we should everytime retrieve new j_running_transaction
> +		 * value (after j_state_lock release acquire cycle), else it may
> +		 * lead to use-after-free of old freed transaction.
> +		 */
> +		transaction_t *transaction = journal->j_running_transaction;
> 
> -	spin_lock(&commit_transaction->t_handle_lock);
> -	while (atomic_read(&commit_transaction->t_updates)) {
> -		DEFINE_WAIT(wait);
> +		if (!transaction)
> +			break;
> 
> +		spin_lock(&transaction->t_handle_lock);
>  		prepare_to_wait(&journal->j_wait_updates, &wait,
> -					TASK_UNINTERRUPTIBLE);
> -		if (atomic_read(&commit_transaction->t_updates)) {
> -			spin_unlock(&commit_transaction->t_handle_lock);
> -			write_unlock(&journal->j_state_lock);
> -			schedule();
> -			write_lock(&journal->j_state_lock);
> -			spin_lock(&commit_transaction->t_handle_lock);
> +				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);
>  	}
> -	spin_unlock(&commit_transaction->t_handle_lock);
>  }
> 
>  /**
> @@ -877,8 +888,6 @@ void jbd2_journal_wait_updates(journal_t *journal)
>   */
>  void jbd2_journal_lock_updates(journal_t *journal)
>  {
> -	DEFINE_WAIT(wait);
> -
>  	jbd2_might_wait_for_commit(journal);
> 
>  	write_lock(&journal->j_state_lock);
> --
> 2.31.1
>