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

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

[Ritesh Harjani]

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>
---
 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

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

[Jan Kara]

On Thu 10-02-22 21:07:11, 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>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Theodore Ts'o]

On Thu, 10 Feb 2022 21:07:11 +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.
> 
> [...]

Applied, thanks!

[1/1] jbd2: Fix use-after-free of transaction_t race
      commit: cc16eecae687912238ee6efbff71ad31e2bc414e

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

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

[Samuel Mendoza-Jonas]

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
> 

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

[Jan Kara]

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...

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

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

[Samuel Mendoza-Jonas]

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