| Message ID | 20240930030610.591772-1-koichiro.den@canonical.com |
|---|---|
| Headers | show |
| Series | CVE-2022-48733 | expand |
On 9/30/24 11:06, Koichiro Den wrote: > [Impact] > > btrfs: fix use-after-free after failure to create a snapshot > > At ioctl.c:create_snapshot(), we allocate a pending snapshot structure and > then attach it to the transaction's list of pending snapshots. After that > we call btrfs_commit_transaction(), and if that returns an error we jump > to 'fail' label, where we kfree() the pending snapshot structure. This can > result in a later use-after-free of the pending snapshot: > > 1) We allocated the pending snapshot and added it to the transaction's > list of pending snapshots; > > 2) We call btrfs_commit_transaction(), and it fails either at the first > call to btrfs_run_delayed_refs() or btrfs_start_dirty_block_groups(). > In both cases, we don't abort the transaction and we release our > transaction handle. We jump to the 'fail' label and free the pending > snapshot structure. We return with the pending snapshot still in the > transaction's list; > > 3) Another task commits the transaction. This time there's no error at > all, and then during the transaction commit it accesses a pointer > to the pending snapshot structure that the snapshot creation task > has already freed, resulting in a user-after-free. > > This issue could actually be detected by smatch, which produced the > following warning: > > fs/btrfs/ioctl.c:843 create_snapshot() warn: '&pending_snapshot->list' not removed from list > > So fix this by not having the snapshot creation ioctl directly add the > pending snapshot to the transaction's list. Instead add the pending > snapshot to the transaction handle, and then at btrfs_commit_transaction() > we add the snapshot to the list only when we can guarantee that any error > returned after that point will result in a transaction abort, in which > case the ioctl code can safely free the pending snapshot and no one can > access it anymore. > > [Backport] > > Adjusted context due to missing commits: > - 9babda9f33fd ("btrfs: Remove async_transid from btrfs_mksubvol/create_subvol/create_snapshot") > - d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit") > > [Fix] > > Noble: not affected > Jammy: fixed via stable > Focal: Backport - adjusted context, see [Backport] > Bionic: fix sent to esm ML > Xenial: fix sent to esm ML > Trusty: won't fix Just curious, does the CVE applicable to kernel version less than 5.10? Because the fix has the tag (CC: stable@vger.kernel.org # 5.10+). Anyway, Acked-by: Guoqing Jiang <guoqing.jiang@canonical.com> Guoqing > [Test Case] > > Compile and boot tested (amd64 only). > > Ran Smatch and verified that with this backport, the following warn > message disappeared: > fs/btrfs/ioctl.c:908 create_snapshot() warn: '&pending_snapshot->list' not removed from list > > Also, did snapshot testing and verified that btrfs_commit_transaction() > triggered by it succeeded without any issues (amd64 only). > > [Where problems could occur] > > This fix affects those who use btrfs snapshot feature, an issue with > this fix would be visible to the user via unpredicted system behaviour > or a system crash induced by use-after-free. > > > Filipe Manana (1): > btrfs: fix use-after-free after failure to create a snapshot > > fs/btrfs/ioctl.c | 5 +---- > fs/btrfs/transaction.c | 24 ++++++++++++++++++++++++ > fs/btrfs/transaction.h | 2 ++ > 3 files changed, 27 insertions(+), 4 deletions(-) >
Acked-by: Ivan Hu <ivan.hu@canonical.com> On 9/30/24 11:06, Koichiro Den wrote: > [Impact] > > btrfs: fix use-after-free after failure to create a snapshot > > At ioctl.c:create_snapshot(), we allocate a pending snapshot structure and > then attach it to the transaction's list of pending snapshots. After that > we call btrfs_commit_transaction(), and if that returns an error we jump > to 'fail' label, where we kfree() the pending snapshot structure. This can > result in a later use-after-free of the pending snapshot: > > 1) We allocated the pending snapshot and added it to the transaction's > list of pending snapshots; > > 2) We call btrfs_commit_transaction(), and it fails either at the first > call to btrfs_run_delayed_refs() or btrfs_start_dirty_block_groups(). > In both cases, we don't abort the transaction and we release our > transaction handle. We jump to the 'fail' label and free the pending > snapshot structure. We return with the pending snapshot still in the > transaction's list; > > 3) Another task commits the transaction. This time there's no error at > all, and then during the transaction commit it accesses a pointer > to the pending snapshot structure that the snapshot creation task > has already freed, resulting in a user-after-free. > > This issue could actually be detected by smatch, which produced the > following warning: > > fs/btrfs/ioctl.c:843 create_snapshot() warn: '&pending_snapshot->list' not removed from list > > So fix this by not having the snapshot creation ioctl directly add the > pending snapshot to the transaction's list. Instead add the pending > snapshot to the transaction handle, and then at btrfs_commit_transaction() > we add the snapshot to the list only when we can guarantee that any error > returned after that point will result in a transaction abort, in which > case the ioctl code can safely free the pending snapshot and no one can > access it anymore. > > [Backport] > > Adjusted context due to missing commits: > - 9babda9f33fd ("btrfs: Remove async_transid from btrfs_mksubvol/create_subvol/create_snapshot") > - d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit") > > [Fix] > > Noble: not affected > Jammy: fixed via stable > Focal: Backport - adjusted context, see [Backport] > Bionic: fix sent to esm ML > Xenial: fix sent to esm ML > Trusty: won't fix > > [Test Case] > > Compile and boot tested (amd64 only). > > Ran Smatch and verified that with this backport, the following warn > message disappeared: > fs/btrfs/ioctl.c:908 create_snapshot() warn: '&pending_snapshot->list' not removed from list > > Also, did snapshot testing and verified that btrfs_commit_transaction() > triggered by it succeeded without any issues (amd64 only). > > [Where problems could occur] > > This fix affects those who use btrfs snapshot feature, an issue with > this fix would be visible to the user via unpredicted system behaviour > or a system crash induced by use-after-free. > > > Filipe Manana (1): > btrfs: fix use-after-free after failure to create a snapshot > > fs/btrfs/ioctl.c | 5 +---- > fs/btrfs/transaction.c | 24 ++++++++++++++++++++++++ > fs/btrfs/transaction.h | 2 ++ > 3 files changed, 27 insertions(+), 4 deletions(-) >
On Mon, Sep 30, 2024 at 02:45:23PM +0800, Guoqing Jiang wrote: > > > On 9/30/24 11:06, Koichiro Den wrote: > > [Impact] > > > > btrfs: fix use-after-free after failure to create a snapshot > > > > At ioctl.c:create_snapshot(), we allocate a pending snapshot structure and > > then attach it to the transaction's list of pending snapshots. After that > > we call btrfs_commit_transaction(), and if that returns an error we jump > > to 'fail' label, where we kfree() the pending snapshot structure. This can > > result in a later use-after-free of the pending snapshot: > > > > 1) We allocated the pending snapshot and added it to the transaction's > > list of pending snapshots; > > > > 2) We call btrfs_commit_transaction(), and it fails either at the first > > call to btrfs_run_delayed_refs() or btrfs_start_dirty_block_groups(). > > In both cases, we don't abort the transaction and we release our > > transaction handle. We jump to the 'fail' label and free the pending > > snapshot structure. We return with the pending snapshot still in the > > transaction's list; > > > > 3) Another task commits the transaction. This time there's no error at > > all, and then during the transaction commit it accesses a pointer > > to the pending snapshot structure that the snapshot creation task > > has already freed, resulting in a user-after-free. > > > > This issue could actually be detected by smatch, which produced the > > following warning: > > > > fs/btrfs/ioctl.c:843 create_snapshot() warn: '&pending_snapshot->list' not removed from list > > > > So fix this by not having the snapshot creation ioctl directly add the > > pending snapshot to the transaction's list. Instead add the pending > > snapshot to the transaction handle, and then at btrfs_commit_transaction() > > we add the snapshot to the list only when we can guarantee that any error > > returned after that point will result in a transaction abort, in which > > case the ioctl code can safely free the pending snapshot and no one can > > access it anymore. > > > > [Backport] > > > > Adjusted context due to missing commits: > > - 9babda9f33fd ("btrfs: Remove async_transid from btrfs_mksubvol/create_subvol/create_snapshot") > > - d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit") > > > > [Fix] > > > > Noble: not affected > > Jammy: fixed via stable > > Focal: Backport - adjusted context, see [Backport] > > Bionic: fix sent to esm ML > > Xenial: fix sent to esm ML > > Trusty: won't fix > > Just curious, does the CVE applicable to kernel version less than 5.10? > Because the fix has the tag (CC: stable@vger.kernel.org # 5.10+). Sorry I missed this. Yeah I think so. In my opinion e4a2bcaca964 ("Btrfs: if we aren't committing just end the transaction if we error out") is suspicious. And also Smatch complained for both Bionic and Xenial, other than Focal. > > Anyway, Acked-by: Guoqing Jiang <guoqing.jiang@canonical.com> > > Guoqing > > [Test Case] > > > > Compile and boot tested (amd64 only). > > > > Ran Smatch and verified that with this backport, the following warn > > message disappeared: fs/btrfs/ioctl.c:908 create_snapshot() warn: > > '&pending_snapshot->list' not removed from list > > > > Also, did snapshot testing and verified that > > btrfs_commit_transaction() triggered by it succeeded without any > > issues (amd64 only). > > > > [Where problems could occur] > > > > This fix affects those who use btrfs snapshot feature, an issue with > > this fix would be visible to the user via unpredicted system > > behaviour or a system crash induced by use-after-free. > > > > > > Filipe Manana (1): btrfs: fix use-after-free after failure to create > > a snapshot > > > > fs/btrfs/ioctl.c | 5 +---- fs/btrfs/transaction.c | 24 > > ++++++++++++++++++++++++ fs/btrfs/transaction.h | 2 ++ 3 files > > changed, 27 insertions(+), 4 deletions(-) > > >
[Impact] btrfs: fix use-after-free after failure to create a snapshot At ioctl.c:create_snapshot(), we allocate a pending snapshot structure and then attach it to the transaction's list of pending snapshots. After that we call btrfs_commit_transaction(), and if that returns an error we jump to 'fail' label, where we kfree() the pending snapshot structure. This can result in a later use-after-free of the pending snapshot: 1) We allocated the pending snapshot and added it to the transaction's list of pending snapshots; 2) We call btrfs_commit_transaction(), and it fails either at the first call to btrfs_run_delayed_refs() or btrfs_start_dirty_block_groups(). In both cases, we don't abort the transaction and we release our transaction handle. We jump to the 'fail' label and free the pending snapshot structure. We return with the pending snapshot still in the transaction's list; 3) Another task commits the transaction. This time there's no error at all, and then during the transaction commit it accesses a pointer to the pending snapshot structure that the snapshot creation task has already freed, resulting in a user-after-free. This issue could actually be detected by smatch, which produced the following warning: fs/btrfs/ioctl.c:843 create_snapshot() warn: '&pending_snapshot->list' not removed from list So fix this by not having the snapshot creation ioctl directly add the pending snapshot to the transaction's list. Instead add the pending snapshot to the transaction handle, and then at btrfs_commit_transaction() we add the snapshot to the list only when we can guarantee that any error returned after that point will result in a transaction abort, in which case the ioctl code can safely free the pending snapshot and no one can access it anymore. [Backport] Adjusted context due to missing commits: - 9babda9f33fd ("btrfs: Remove async_transid from btrfs_mksubvol/create_subvol/create_snapshot") - d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit") [Fix] Noble: not affected Jammy: fixed via stable Focal: Backport - adjusted context, see [Backport] Bionic: fix sent to esm ML Xenial: fix sent to esm ML Trusty: won't fix [Test Case] Compile and boot tested (amd64 only). Ran Smatch and verified that with this backport, the following warn message disappeared: fs/btrfs/ioctl.c:908 create_snapshot() warn: '&pending_snapshot->list' not removed from list Also, did snapshot testing and verified that btrfs_commit_transaction() triggered by it succeeded without any issues (amd64 only). [Where problems could occur] This fix affects those who use btrfs snapshot feature, an issue with this fix would be visible to the user via unpredicted system behaviour or a system crash induced by use-after-free. Filipe Manana (1): btrfs: fix use-after-free after failure to create a snapshot fs/btrfs/ioctl.c | 5 +---- fs/btrfs/transaction.c | 24 ++++++++++++++++++++++++ fs/btrfs/transaction.h | 2 ++ 3 files changed, 27 insertions(+), 4 deletions(-)