Re: [PATCH net] net: prevent NULL pointer dereference in rt_fibinfo_free() and rt_fibinfo_free_cpus()

[Eric Dumazet]

On Tue, Sep 10, 2024 at 4:06 PM Jeongjun Park <aha310510@xxxxxxxxx> wrote:
>
> Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> >
> > On Tue, Sep 10, 2024 at 5:23 AM Jeongjun Park <aha310510@xxxxxxxxx> wrote:
> > >
> > >
> > > > Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > > > On Mon, Sep 9, 2024 at 8:48 PM Jeongjun Park <aha310510@xxxxxxxxx> wrote:
> > > >>
> > > >> rt_fibinfo_free() and rt_fibinfo_free_cpus() only check for rt and do not
> > > >> verify rt->dst and use it, which will result in NULL pointer dereference.
> > > >>
> > > >> Therefore, to prevent this, we need to add a check for rt->dst.
> > > >>
> > > >> Fixes: 0830106c5390 ("ipv4: take dst->__refcnt when caching dst in fib")
> > > >> Fixes: c5038a8327b9 ("ipv4: Cache routes in nexthop exception entries.")
> > > >> Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> > > >> ---
> > > >
> > > > As far as I can tell, your patch is a NOP, and these Fixes: tags seem
> > > > random to me.
> > >
> > > I somewhat agree with the opinion that the fixes tag is random.
> > > However, I think it is absolutely necessary to add a check for
> > > &rt->dst , because the existence of rt does not guarantee that
> > > &rt->dst will not be NULL.
> > >
> > > >
> > > > Also, I am guessing this is based on a syzbot report ?
> > >
> > > Yes, but it's not a bug reported to syzbot, it's a bug that
> > > I accidentally found in my syzkaller fuzzer. The report is too long
> > > to be included in the patch notes, so I'll attach it to this email.
> >
> > syzbot has a similar report in its queue, I put it on hold because
> > this is some unrelated memory corruption.
> >
> > rt (R14 in your case) is 0x1 at this point, which is not a valid memory pointer.
> >
> > So I am definitely saying no to your patch.
> >
>
> I see. Thanks to the explanation, I understood that this patch is wrong.
>
> However, while continuing to analyze this bug, I found out something.
> According to the rcu_dereference_protected() doc, when using
> rcu_dereference_protected(), it is specified that ptr should be protected
> using a lock, but free_fib_info_rcu() does not have any protection for
> the fib_info structure.
>
> I think this may cause a data-race, which modifies the values of rt and
> &rt->dst, causing the bug. Even if this is not the root cause, I don't
> think there is a reason why free_fib_info_rcu() should not be protected
> with fib_info_lock.
>
> What do you think about protecting the fib_info structure like the patch
> below?
>

By the time free_fib_info_rcu() is called, we have the guarantee that
no other threads/cpu can possibly use the
struct fib_info.

nexthop_put(), fib_nh_release(), ip_fib_metrics_put, kfree() do not
need this spinlock protection.

fib_info_lock has absolutely no effect here.

Also, 0x1 is not a valid pointer, there is absolutely no chance a
network layer could use 0x1 as a rt/dst pointer.

As I said, the bug is elsewhere, something is mangling some piece of memory.