On Fri, 25 Oct 2024 21:17:09 -0400, Kent Overstreet wrote: > On Sat, Oct 26, 2024 at 09:09:53AM +0800, Lizhi Xu wrote: > > Syzbot reported a slab-use-after-free Read in bch2_reconstruct_alloc. > > The sb counters resize will cause sb reallocation and release the old sb, > > which leads to uaf in ext. > > After disk_sb.sb is reallocated, ext should be retrieved again to avoid uaf. > > > > Reported-and-tested-by: > > [email protected] > > Closes: https://syzkaller.appspot.com/bug?extid=9fc4dac4775d07bcfe34 > > Signed-off-by: Lizhi Xu <[email protected]> > > --- > > fs/bcachefs/recovery.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c > > index 55e1504a8130..9df0969c29ce 100644 > > --- a/fs/bcachefs/recovery.c > > +++ b/fs/bcachefs/recovery.c > > @@ -59,6 +59,7 @@ static void bch2_reconstruct_alloc(struct bch_fs *c) > > > > mutex_lock(&c->sb_lock); > > struct bch_sb_field_ext *ext = bch2_sb_field_get(c->disk_sb.sb, ext); > > + void *sb; > > > > __set_bit_le64(BCH_RECOVERY_PASS_STABLE_check_allocations, > > ext->recovery_passes_required); > > __set_bit_le64(BCH_RECOVERY_PASS_STABLE_check_alloc_info, > > ext->recovery_passes_required); > > @@ -94,7 +95,10 @@ static void bch2_reconstruct_alloc(struct bch_fs *c) > > __set_bit_le64(BCH_FSCK_ERR_accounting_mismatch, ext->errors_silent); > > c->sb.compat &= ~(1ULL << BCH_COMPAT_alloc_info); > > > > + sb = c->disk_sb.sb; > > bch2_write_super(c); > > + if (sb != c->disk_sb.sb) > > + ext = bch2_sb_field_get(c->disk_sb.sb, ext); > > mutex_unlock(&c->sb_lock); > > > > c->opts.recovery_passes |= > > bch2_recovery_passes_from_stable(le64_to_cpu(ext->recovery_passes_required[0])); > > There's a simpler fix: > > > From 8e910ca20e112d7f06ba3bf631a06ddb5ce14657 Mon Sep 17 00:00:00 2001 > From: Kent Overstreet <[email protected]> > Date: Fri, 25 Oct 2024 13:13:05 -0400 > Subject: [PATCH] bcachefs: Fix UAF in bch2_reconstruct_alloc() > > write_super() -> sb_counters_from_cpu() may reallocate the superblock > > Reported-by: [email protected] > Signed-off-by: Kent Overstreet <[email protected]> > > diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c > index 454b5a32dd7f..0ebc76dd7eb5 100644 > --- a/fs/bcachefs/recovery.c > +++ b/fs/bcachefs/recovery.c > @@ -94,11 +94,10 @@ static void bch2_reconstruct_alloc(struct bch_fs *c) > __set_bit_le64(BCH_FSCK_ERR_accounting_mismatch, ext->errors_silent); > c->sb.compat &= ~(1ULL << BCH_COMPAT_alloc_info); > > - bch2_write_super(c); > - mutex_unlock(&c->sb_lock); > - > c->opts.recovery_passes |= > bch2_recovery_passes_from_stable(le64_to_cpu(ext->recovery_passes_required[0])); > > + bch2_write_super(c); > + mutex_unlock(&c->sb_lock); > > bch2_shoot_down_journal_keys(c, BTREE_ID_alloc, > 0, BTREE_MAX_DEPTH, POS_MIN, SPOS_MAX); Oh, Yes, it can fix, but my first fix is before your patch,
https://lore.kernel.org/all/[email protected]/ Subject: Re: [syzbot] [bcachefs?] KASAN: slab-use-after-free Read in bch2_reconstruct_alloc Date: Fri, 25 Oct 2024 18:02:05 +0800 [thread overview] Message-ID: <[email protected]> (raw) In-Reply-To: <[email protected]> The counters will cause sb to be realloc and release the old sb, which leads to uaf in ext #syz test: upstream master diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c index 55e1504a8130..9df0969c29ce 100644 --- a/fs/bcachefs/recovery.c +++ b/fs/bcachefs/recovery.c @@ -59,6 +59,7 @@ static void bch2_reconstruct_alloc(struct bch_fs *c) mutex_lock(&c->sb_lock); struct bch_sb_field_ext *ext = bch2_sb_field_get(c->disk_sb.sb, ext); + void *sb; __set_bit_le64(BCH_RECOVERY_PASS_STABLE_check_allocations, ext->recovery_passes_required); __set_bit_le64(BCH_RECOVERY_PASS_STABLE_check_alloc_info, ext->recovery_passes_required); @@ -94,7 +95,10 @@ static void bch2_reconstruct_alloc(struct bch_fs *c) __set_bit_le64(BCH_FSCK_ERR_accounting_mismatch, ext->errors_silent); c->sb.compat &= ~(1ULL << BCH_COMPAT_alloc_info); + sb = c->disk_sb.sb; bch2_write_super(c); + if (sb != c->disk_sb.sb) + ext = bch2_sb_field_get(c->disk_sb.sb, ext); mutex_unlock(&c->sb_lock); c->opts.recovery_passes |= bch2_recovery_passes_from_stable(le64_to_cpu(ext->recovery_passes_required[0]));
