linux-kernel - Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8aW4pK0ZSmglofzD-Ej0W2ClwAV-1qDS95_nUqwxsw_I4w6gERwlUN-uqb-n2fiMyh5r_1pzEj4vCIBLL5vSJAAnzqTgijxAHB2pRnIFNww=@proton.me>
Date: Fri, 25 Oct 2024 23:39:38 +0000
From: Piotr Zalewski <pZ010001011111@...ton.me>
To: Piotr Zalewski <pZ010001011111@...ton.me>
Cc: Alan Huang <mmpgouride@...il.com>, Kent Overstreet <kent.overstreet@...ux.dev>, linux-bcachefs@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, skhan@...uxfoundation.org, syzbot+005ef9aa519f30d97657@...kaller.appspotmail.com
Subject: Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

Hi Alan and all,

On Wednesday, October 23rd, 2024 at 10:06 AM, Piotr Zalewski <pZ010001011111@...ton.me> wrote:

> Hi Alan,
>
> On Wednesday, October 23rd, 2024 at 9:33 AM, Alan Huang mmpgouride@...il.com wrote:
>
> > On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@...ton.me wrote:
> >
> > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in
> > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in
> > > bch2_bkey_buf_reassemble.
> >
> > It would be helpful if the commit message explained why k.k is null in this case
>
>
> I will debug this more thoroughly and provide details. For now I see that
> during GC it sees journal replay hasn't finished but journal turns out to
> be empty? Which seems weird, so maybe underlying issue should be solved on
> a deeper level.
>

There is a clean shutdown detected during recovery. Journal has no entries 
as it was confirmed by debugging bch2_fs_journal_start. seq and seq_ondisk 
members are equal so journal is "quiesced". Also, journal_keys size is 0.

So in check_allocations when keys are being marked for GC journal replay is
not done so it peeks into journal and there is nothing.

Now, maybe in case journal is empty during start, replay done should be set 
already in bch2_fs_journal_start? I tested it and it fixed the issue as well.
```
diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 2cf8f24d50cc..67b342d23346 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1287,6 +1287,9 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
        spin_lock(&j->lock);

        set_bit(JOURNAL_running, &j->flags);
+       if (!had_entries) {
+               set_bit(JOURNAL_replay_done, &j->flags);
+       }
        j->last_flush_write = jiffies;

        j->reservations.idx = j->reservations.unwritten_idx = journal_cur_seq(j);
```


(But at the same time there is a check for whether there were entries just 
above the code which sets journal running so it seems unlikely that if it's 
correct approach it's not yet there).

(In need of some pointer/explanation)

>
> > > Reported-by: syzbot+005ef9aa519f30d97657@...kaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > > Signed-off-by: Piotr Zalewski pZ010001011111@...ton.me
> > > ---
> > > fs/bcachefs/btree_iter.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > > index 0883cf6e1a3e..625167ce191f 100644
> > > --- a/fs/bcachefs/btree_iter.c
> > > +++ b/fs/bcachefs/btree_iter.c
> > > @@ -882,6 +882,8 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > > __bch2_btree_and_journal_iter_init_node_iter(trans, &jiter, l->b, l->iter, path->pos);
> > >
> > > k = bch2_btree_and_journal_iter_peek(&jiter);
> > > + if (!k.k)
> > > + goto err;
> > >
> > > bch2_bkey_buf_reassemble(out, c, k);
> > >
> > > @@ -889,6 +891,7 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > > c->opts.btree_node_prefetch)
> > > ret = btree_path_prefetch_j(trans, path, &jiter);
> > >
> > > +err:
> > > bch2_btree_and_journal_iter_exit(&jiter);
> > > return ret;
> > > }
> > > --
> > > 2.47.0
>
Best regards, Piotr Zalewski