ACK/Cmt: [SRU][F][PATCH 0/1] CVE-2022-48733

[Koichiro Den]

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 at 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 at 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(-)
> > 
>