Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Apr 2023 10:35:54 -0700
From:   Peilin Ye <yepeilin.cs@gmail.com>
To:     Vlad Buslov <vladbu@nvidia.com>
Cc:     Jamal Hadi Salim <jhs@mojatatu.com>,
        Pedro Tammela <pctammela@mojatatu.com>,
        Seth Forshee <sforshee@digitalocean.com>,
        Jakub Kicinski <kuba@kernel.org>,
        syzbot <syzbot+b53a9c0d1ea4ad62da8b@syzkaller.appspotmail.com>,
        davem@davemloft.net, edumazet@google.com, jiri@resnulli.us,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        pabeni@redhat.com, syzkaller-bugs@googlegroups.com,
        xiyou.wangcong@gmail.com, peilin.ye@bytedance.com, hdanton@sina.com
Subject: Re: [syzbot] [net?] KASAN: slab-use-after-free Write in
 mini_qdisc_pair_swap
Message-ID: <20230427173554.GA11725@bytedance>
References: <0000000000006cf87705f79acf1a@google.com>
 <20230328184733.6707ef73@kernel.org>
 <ZCOylfbhuk0LeVff@do-x1extreme>
 <b4d93f31-846f-3391-db5d-db8682ac3c34@mojatatu.com>
 <CAM0EoMn2LnhdeLcxCFdv+4YshthN=YHLnr1rvv4JoFgNS92hRA@mail.gmail.com>
 <20230417230011.GA41709@bytedance>
 <20230426233657.GA11249@bytedance>
 <877ctxsdnb.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <877ctxsdnb.fsf@nvidia.com>
Precedence: bulk

Hi Pedro, Vlad,

On Thu, Apr 27, 2023 at 03:26:03PM +0300, Vlad Buslov wrote:
> On Wed 26 Apr 2023 at 16:42, Peilin Ye <yepeilin.cs@gmail.com> wrote:
> > As we can see there're interleaving mini_qdisc_pair_swap() calls between
> > Qdisc A and B, causing all kinds of troubles, including the UAF (thread
> > 2 writing to mini Qdisc a1's rcu_state after Qdisc A has already been
> > freed) reported by syzbot.
> 
> Great analysis! However, it is still not quite clear to me how threads 1
> and 2 access each other RCU state when q->miniqp is a private memory of
> the Qdisc, so 1 should only see A->miniqp and 2 only B->miniqp. And both
> miniqps should be protected from deallocation by reference that lockless
> RTM_NEWTFILTER obtains.

Thanks for taking a look!

To elaborate, p_miniq is a pointer of pointer of struct mini_Qdisc,
initialized in ingress_init() to point to eth0->miniq_ingress, which
isn't private to A or B.

In other words, both A->miniqp->p_miniq and B->miniqp->p_miniq point to
eth0->miniq_ingress.

For your reference, roughly speaking, mini_qdisc_pair_swap() does this:

  miniq_old = dev->miniq_ingress;

  if (destroying) {
          dev->miniq_ingress = NULL;
  } else {
          rcu_wait();
          dev->miniq_ingress = miniq_new;
  }

  if (miniq_old)
          miniq_old->rcu_state = ...

On Wed 26 Apr 2023 at 16:42, Peilin Ye <yepeilin.cs@gmail.com> wrote:
>  Thread 1               A's refcnt   Thread 2
>   RTM_NEWQDISC (A, locked)
>    qdisc_create(A)               1
>    qdisc_graft(A)                9
>
>   RTM_NEWTFILTER (X, lockless)
>    __tcf_qdisc_find(A)          10
>    tcf_chain0_head_change(A)
>  ! mini_qdisc_pair_swap(A)

  1. A adds its first filter,
     miniq_old (eth0->miniq_ingress) is NULL,
     RCU wait starts,
     RCU wait ends,
     change eth0->miniq_ingress to A's mini Qdisc.

>             |                        RTM_NEWQDISC (B, locked)
>             |                    2    qdisc_graft(B)
>             |                    1    notify_and_destroy(A)
>             |
>             |                        RTM_NEWTFILTER (Y, lockless)
>             |                         tcf_chain0_head_change(B)
>             |                       ! mini_qdisc_pair_swap(B)

                      2. B adds its first filter,
                         miniq_old (eth0->miniq_ingress) is A's mini Qdisc,
                         RCU wait starts,

>    tcf_block_release(A)          0             |
>    qdisc_destroy(A)                            |
>    tcf_chain0_head_change_cb_del(A)            |
>  ! mini_qdisc_pair_swap(A)                     |

  3. A destroys itself,
     miniq_old (eth0->miniq_ingress) is A's mini Qdisc,
     (destroying, so no RCU wait)
     change eth0->miniq_ingress to NULL,
     update miniq_old, or A's mini Qdisc's RCU state,
     A is freed.

                      2. RCU wait ends,
		         change eth0->miniq_ingress to B's mini Qdisc,
	 use-after-free: update miniq_old, or A's mini Qdisc's RCU state.

I hope this helps.  Sorry I didn't go into details; this UAF isn't the
only thing that is unacceptable here:

Consider B.  We add a filter Y to B, expecting ingress packets on eth0
to go through Y.  Then all of a sudden, A sets eth0->miniq_ingress to
NULL during its destruction, so packets will not find Y at all on
datapath (sch_handle_ingress()).  New filter becomes invisible - this is
already buggy enough :-/

So I think B's first call to mini_qdisc_pair_swap() should happen after
A's last call (in ingress_destroy()), which is what I am trying to
achieve here.

Thanks,
Peilin Ye