From: Vlad Buslov <vladbu@nvidia•com>
To: Peilin Ye <yepeilin.cs@gmail•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
Date: Fri, 28 Apr 2023 15:43:05 +0300 [thread overview]
Message-ID: <87y1mcqiqq.fsf@nvidia.com> (raw)
In-Reply-To: <20230427173554.GA11725@bytedance>
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.
next prev parent reply other threads:[~2023-04-28 12:44 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-24 0:52 [syzbot] [net?] KASAN: slab-use-after-free Write in mini_qdisc_pair_swap syzbot
2023-03-29 1:47 ` Jakub Kicinski
2023-03-29 3:37 ` Seth Forshee
2023-03-29 19:07 ` Pedro Tammela
2023-04-03 15:58 ` Jamal Hadi Salim
2023-04-17 23:00 ` Peilin Ye
2023-04-26 23:42 ` Peilin Ye
2023-04-27 2:31 ` Pedro Tammela
2023-04-27 12:26 ` Vlad Buslov
2023-04-27 17:35 ` Peilin Ye
2023-04-28 12:43 ` Vlad Buslov [this message]
2023-05-06 0:09 ` [PATCH net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
2023-05-06 0:12 ` [PATCH net 1/6] net/sched: sch_ingress: Only create under TC_H_INGRESS Peilin Ye
2023-05-08 11:22 ` Jamal Hadi Salim
2023-05-06 0:13 ` [PATCH net 2/6] net/sched: sch_clsact: Only create under TC_H_CLSACT Peilin Ye
2023-05-08 11:23 ` Jamal Hadi Salim
2023-05-06 0:14 ` [PATCH net 3/6] net/sched: Reserve TC_H_INGRESS (TC_H_CLSACT) for ingress (clsact) Qdiscs Peilin Ye
2023-05-08 11:23 ` Jamal Hadi Salim
2023-05-06 0:14 ` [PATCH net 4/6] net/sched: Prohibit regrafting ingress or clsact Qdiscs Peilin Ye
2023-05-08 11:24 ` Jamal Hadi Salim
2023-05-06 0:15 ` [PATCH net 5/6] net/sched: Refactor qdisc_graft() for ingress and " Peilin Ye
2023-05-08 11:29 ` Jamal Hadi Salim
2023-05-08 22:24 ` Peilin Ye
2023-05-08 14:11 ` Pedro Tammela
2023-05-06 0:16 ` [PATCH net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Peilin Ye
2023-05-08 11:32 ` Jamal Hadi Salim
2023-05-08 21:58 ` Peilin Ye
2023-05-08 14:12 ` Pedro Tammela
2023-05-08 22:01 ` Peilin Ye
2023-05-09 1:33 ` Jakub Kicinski
2023-05-10 20:11 ` Peilin Ye
2023-05-10 23:15 ` Jakub Kicinski
2023-05-11 20:40 ` Peilin Ye
2023-05-11 23:20 ` Jakub Kicinski
2023-05-11 23:46 ` Peilin Ye
2023-05-12 0:11 ` Peilin Ye
2023-05-15 22:45 ` Peilin Ye
2023-05-16 19:22 ` Jakub Kicinski
2023-05-16 19:35 ` Vlad Buslov
2023-05-16 21:50 ` Jakub Kicinski
2023-05-16 22:58 ` Peilin Ye
2023-05-17 0:39 ` Jakub Kicinski
2023-05-17 8:49 ` Vlad Buslov
2023-05-17 18:48 ` Jakub Kicinski
2023-05-17 22:20 ` Peilin Ye
2023-05-17 18:48 ` Jakub Kicinski
2023-05-17 21:18 ` Peilin Ye
2023-05-24 14:31 ` [syzbot] [net?] KASAN: slab-use-after-free Write in mini_qdisc_pair_swap Pedro Tammela
2023-05-24 15:02 ` syzbot
2023-05-24 15:05 ` Pedro Tammela
2023-05-24 15:34 ` syzbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1mcqiqq.fsf@nvidia.com \
--to=vladbu@nvidia$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=hdanton@sina$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=kuba@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=pctammela@mojatatu$(echo .)com \
--cc=peilin.ye@bytedance$(echo .)com \
--cc=sforshee@digitalocean$(echo .)com \
--cc=syzbot+b53a9c0d1ea4ad62da8b@syzkaller$(echo .)appspotmail.com \
--cc=syzkaller-bugs@googlegroups$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
--cc=yepeilin.cs@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox