Message ID | 948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com |
---|---|
State | Awaiting Upstream |
Headers | show |
Series | jbd2: Fix use-after-free of transaction_t race | expand |
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 >
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,
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 >
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
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
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);
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(-) -- 2.31.1