Messages in this thread | | | From | Vlad Buslov <> | Subject | Re: [syzbot] [net?] KASAN: slab-use-after-free Write in mini_qdisc_pair_swap | Date | Fri, 28 Apr 2023 15:43:05 +0300 |
| |
On Thu 27 Apr 2023 at 10:35, Peilin Ye <yepeilin.cs@gmail.com> wrote: > 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.
Thanks for the clarification.
> > 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.
Makes sense to me.
| |