netdev - Re: [syzbot] [net?] KASAN: slab-use-after-free Write in mini_qdisc_pair

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 26 Apr 2023 23:31:07 -0300
From:   Pedro Tammela <pctammela@...atatu.com>
To:     Peilin Ye <yepeilin.cs@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Seth Forshee <sforshee@...italocean.com>,
        Jakub Kicinski <kuba@...nel.org>,
        syzbot <syzbot+b53a9c0d1ea4ad62da8b@...kaller.appspotmail.com>,
        davem@...emloft.net, edumazet@...gle.com, jiri@...nulli.us,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        pabeni@...hat.com, syzkaller-bugs@...glegroups.com,
        xiyou.wangcong@...il.com, peilin.ye@...edance.com,
        vladbu@...dia.com, hdanton@...a.com
Subject: Re: [syzbot] [net?] KASAN: slab-use-after-free Write in
 mini_qdisc_pair_swap

On 26/04/2023 20:42, Peilin Ye 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.
> 
> 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.

Thanks for the analysis. It makes total sense.
After going through the call chains, please correct me if my ELI5 is wrong:

'clsact_init()' is called for B when dev has miniq_ingress set to 'A', 
therefore copying a pointer to the miniq_qdisc with lifetime bound to 
'A' in a miniq_qdisc_pair with lifetime bound to 'B' therefore raising 
an UAF after A is destroyed and B is manipulated.

> 
> 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?
Makes sense.

> 
>    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 >
> 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.

Probably worth a comment somewhere in the code

> 
> 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
>