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;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
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> <20230427173554.GA11725@bytedance>
User-agent: mu4e 1.8.11; emacs 28.2
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
In-Reply-To: <20230427173554.GA11725@bytedance>
Message-ID: <87y1mcqiqq.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Apr 2023 12:44:35.5985
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 201ce8ef-0e4b-4fef-2b31-08db47e652a8
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BL02EPF000100D3.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6783
X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.