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>
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:   Thu, 27 Apr 2023 15:26:03 +0300
In-Reply-To: <20230426233657.GA11249@bytedance>
Message-ID: <877ctxsdnb.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2023 12:39:22.0184
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: df0c6340-d632-4c6d-bfc8-08db471c6d4e
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: CY4PEPF0000B8EE.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4523
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,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE 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

Hi Peilin,

On Wed 26 Apr 2023 at 16:42, Peilin Ye <yepeilin.cs@gmail.com> wrote:
> +Cc: Vlad Buslov, Hillf Danton
>
> Hi all,
>
> On Mon, Apr 17, 2023 at 04:00:11PM -0700, Peilin Ye wrote:
>> I also reproduced this UAF using the syzkaller reproducer in the report
>> (the C reproducer did not work for me for unknown reasons).  I will look
>> into this.
>
> Currently, multiple ingress (clsact) Qdiscs can access the per-netdev
> *miniq_ingress (*miniq_egress) pointer concurrently.  This is
> unfortunately true in two senses:
>
> 1. We allow adding ingress (clsact) Qdiscs under parents other than
> TC_H_INGRESS (TC_H_CLSACT):
>
>   $ ip link add ifb0 numtxqueues 8 type ifb
>   $ echo clsact > /proc/sys/net/core/default_qdisc
>   $ tc qdisc add dev ifb0 handle 1: root mq
>   $ tc qdisc show dev ifb0
>   qdisc mq 1: root
>   qdisc clsact 0: parent 1:8
>   qdisc clsact 0: parent 1:7
>   qdisc clsact 0: parent 1:6
>   qdisc clsact 0: parent 1:5
>   qdisc clsact 0: parent 1:4
>   qdisc clsact 0: parent 1:3
>   qdisc clsact 0: parent 1:2
>   qdisc clsact 0: parent 1:1
>
> This is obviously racy and should be prohibited.  I've started working
> on patches to fix this.  The syz repro for this UAF adds ingress Qdiscs
> under TC_H_ROOT, by the way.


Hmm, didn't realize it was the case.

>
> 2. After introducing RTNL-lockless RTM_{NEW,DEL,GET}TFILTER requests
> [1], it is possible that, when replacing ingress (clsact) Qdiscs, the
> old one can access *miniq_{in,e}gress concurrently with the new one.  For
> example, the syz repro does something like the following:
>
>   Thread 1 creates sch_ingress Qdisc A (containing mini Qdisc a1 and a2),
>   then adds a cls_flower filter X to Qdisc A.
>
>   Thread 2 creates sch_ingress Qdisc B (containing mini Qdisc b1 and b2)
>   to replace Qdisc A, then adds a cls_flower filter Y to Qdisc B.
>
>   Device has 8 TXQs.
>
>  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)           
>             |                        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)
>    tcf_block_release(A)          0             |
>    qdisc_destroy(A)                            |
>    tcf_chain0_head_change_cb_del(A)            |
>  ! mini_qdisc_pair_swap(A)                     |
>             |                                  |
>            ...                                ...
>
> 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.

>
> To fix this, I'm cooking a patch that, when replacing ingress (clsact)
> Qdiscs, in qdisc_graft():
>
>   I.  We should make sure there's no on-the-fly lockless filter requests
>       for the old Qdisc, and return -EBUSY if there's any (or can/should
>       we wait in RTM_NEWQDISC handler?)
>
>   II. We should destory the old Qdisc before publishing the new one
>       (i.e. setting it to dev_ingress_queue(dev)->qdisc_sleeping, so
>       that subsequent filter requests can see it), because
>       {ingress,clsact}_destroy() also call mini_qdisc_pair_swap(), which
>       sets *miniq_{in,e}gress to NULL

Another approach would be to somehow detect concurrent Qdisc replace and
return -EAGAIN from tcf_chain_tp_insert() before calling
tcf_chain0_head_change(). This would leverage existing cls_api
functionality that automatically retries after releasing all references
to chain/tp and obtaining them again instead of messing with qdisc api.
However, since I still didn't fully grasp the issue it is hard for me to
reason whether such approach would be possible to implement in this
case.

>
> Future Qdiscs that support RTNL-lockless cls_ops, if any, won't need
> this fix, as long as their ->chain_head_change() don't access
> out-of-Qdisc-scope data, like pointers in struct net_device.
>
> Do you think this is the right way to go?  Thanks!
>
> [1] Thanks Hillf Danton for the hint:
>     https://syzkaller.appspot.com/text?tag=Patch&x=10d7cd5bc80000
>
> Thanks,
> Peilin Ye