Re: Use-after-free in Linux tc subsystem (v6.15)

Re: Use-after-free in Linux tc subsystem (v6.15)

[Jamal Hadi Salim]

On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
>
> On Fri, Jun 20, 2025 at 8:24 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Wed, Jun 18, 2025 at 4:17 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Mon, Jun 16, 2025 at 9:03 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Sun, Jun 15, 2025 at 10:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > On Thu, Jun 12, 2025 at 2:18 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > >
> > > > > > Hi Mingi,
> > > > > >
> > > > > > Thanks for your report!
> > > > > >
> > > > > > I won't have time to look into this until this Sunday, if you or
> > > > > > anyone else have
> > > > > > time before that, please feel free to work on a patch. Otherwise, I will take a
> > > > > > look this Sunday.
> > > > >
> > > > > I am testing the attached patch, I will take a deeper look tomorrow.
> > > >
> > > > It is more complicated than I thought. I think we need to rollback all
> > > > the previous enqueue operations, but it does not look pretty either.
> > > >
> > > > Jamal, do you like the attached fix? I don't have any better ideas
> > > > so far. :-/
> > > >
> > >
> > > I just got back - let me look at it tomorrow. Immediate reaction is i
> > > would suspect netem
> >
> > Spent time yesterday and there are two potential approaches
> > (attached), both of which fix the issue but i am not satisfied with
> > either.
> >
> > The root cause being exploited here is there are some qdisc's whose
> > peek() drops packets - but given peek() doesnt return a code, the
> > parent is unaware of what happened.
> >
> > drr_fix.diff
> > avoids making a class active  by detecting whether drr_qlen_notify was
> > called between after enqueue (even though that enqueue succeeded), in
> > that case, returns a NET_XMIT_SUCCESS | __NET_XMIT_BYPASS which ensure
> > we don't add the class to drr.
> >
> > This fixes the UAF but it would require an analogous fix for other
> > qdiscs with similar behavior (ets, hfsc, ...)
> >
> > qfq_netem_child_fix.diff
> > piggy backs on your tbf patch and detects whether
> > qdisc_tree_reduce_backlog was called after qfq's peeked its child
> > (netem in this repro) in enqueue.
> > This would also require fixing other qdiscs.
> >
> > TBH, while both approaches fix the UAF, IMO they are short term hacks
> > and i am sure Mingi and co will find yet another way to send netlink
> > messages to config a _nonsensical hierarchy of qdiscs_ (as was this
> > one!) to create yet another UAF.
> >
> > My suggestion is we go back to a proposal i made a few moons back:
> > create a mechanism to disallow certain hierarchies of qdiscs, ex in
> > this case disallow qfq from being the ancestor of "qdiscs that may
> > drop during peek" (such as netem). Then we can just keep adding more
> > "disallowed configs" that will be rejected via netlink.
> > And TBH, i feel like obsoleting qfq altogether - the author doesnt
> > even respond to emails.
> >
> > cheers,
> > jamal
>
> Hello,
>
> I think the testcase I reported earlier actually contains two
> different bugs. The first is returning SUCCESS with an empty TBF qdisc
> in tbf_segment, and the second is returning SUCCESS with an empty QFQ
> qdisc in qfq_enqueue.
>

Please join the list where a more general solution is being discussed here:
https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/

cheers,
jamal
> The first bug is present in TBF qdisc, so the UAF is also triggered
> when using a qdisc other than QFQ. Below is a POC that shows how the
> TBF qdisc can be made empty by using a CHOKE qdisc instead of a QFQ.
>
> #define _GNU_SOURCE
>
> #include <arpa/inet.h>
> #include <fcntl.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <linux/udp.h>
>
> #ifndef SOL_UDP
> #define SOL_UDP 17 // UDP protocol value for setsockopt
> #endif
>
> void loopback_send (uint64_t size) {
>     struct sockaddr iaddr = { AF_INET };
>     char data[0x1000] = {0,};
>
>     int inet_sock_fd = socket(PF_INET, SOCK_DGRAM, 0);
>
>     int gso_size = 1300;
>
>     setsockopt(inet_sock_fd, SOL_UDP, UDP_SEGMENT, &gso_size, sizeof(gso_size));
>
>     connect(inet_sock_fd, &iaddr, sizeof(iaddr));
>
>     write(inet_sock_fd, data, size);
>
>     close(inet_sock_fd);
> }
>
> int main(int argc, char **argv) {
>     system("ip link set dev lo up");
>     system("ip link set dev lo mtu 1500");
>
>     system("tc qdisc add dev lo root handle 1: drr");
>     system("tc filter add dev lo parent 1: basic classid 1:1");
>     system("tc class add dev lo parent 1: classid 1:1 drr");
>     system("tc class add dev lo parent 1: classid 1:2 drr");
>
>     system("tc qdisc add dev lo parent 1:1 handle 2: tbf rate 1Mbit
> burst 1514 latency 50ms");
>
>     system("tc qdisc add dev lo parent 2:1 handle 3: choke limit 2
> bandwidth 1kbit min 1 max 2 burst 1");
>
>     loopback_send(2000);
>
>     system("tc class del dev lo classid 1:1");
>
>     system("timeout 0.1 ping -c 1 -W0.01 localhost > /dev/null");
> }
>
> My opinion is that creating separate patches for each bug would be an
> easier way to approach the problem.
>
> I tested the suggested patch and found some possible issues.
>
> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index bf1282cb22eb..6d85da21c4b8 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -1258,7 +1258,17 @@ static int qfq_enqueue(struct sk_buff *skb,
> struct Qdisc *sch,
>     agg = cl->agg;
>     /* if the class is active, then done here */
>     if (cl_is_active(cl)) {
> -       if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
> +       const u32 pre_peek_backlog = sch->qstats.backlog;
> +
> +       skb = cl->qdisc->ops->peek(cl->qdisc);
> +       /* Address corner case where a child qdisc dropped the packet
> +        * in peek after enqueue returned success.
> +        * Qdiscs like netem may exhibit this behaviour.
> +        */
> +       if (unlikely(sch->qstats.backlog < pre_peek_backlog))
> +           return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> +
> +       if (unlikely(skb) &&
>             list_first_entry(&agg->active, struct qfq_class, alist)
>             == cl && cl->deficit < len)
>             list_move_tail(&cl->alist, &agg->active);
>
> First, in the proposed patch, the peek function of qfq_enqueue checks
> qstats.backlog to know if a packet has been dropped. However, since
> qstats.backlog decreases during the normal peek process, it would be
> better to use the q.qlen.
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index c5e3673aadbe..10fb72fef98e 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -814,6 +814,11 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch,
> int n, int len)
>             WARN_ON_ONCE(parentid != TC_H_ROOT);
>             break;
>         }
> +
> +       if (unlikely((!sch->q.qlen && n) ||
> +                (!sch->qstats.backlog && len)))
> +           continue;
> +
>         cops = sch->ops->cl_ops;
>         if (notify && cops->qlen_notify) {
>             cl = cops->find(sch, parentid);
>
> Also, if the qdisc_tree_reduce_backlog function excludes cases where
> qlen is 0, as in the above patch, the normal tbf qdisc features may
> not work.
>
> static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch,
>                struct sk_buff **to_free)
> ...
>     sch->q.qlen += nb;
>     sch->qstats.backlog += len;
>     if (nb > 0) {
>         qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
>         consume_skb(skb);
>         return NET_XMIT_SUCCESS;
>     }
>
> For example, qlen can be 0 when calling qdisc_tree_reduce_backlog on
> tbf_segment in the code above.
>
> Additionally, I believe that using the peek function in the enqueue
> function can increase the complexity of qdisc and can lead to a number
> of issues. Therefore, I believe that avoiding the use of peek in the
> enqueue function will reduce the chance of introducing bugs.
>
> Thanks,
> Mingi

Re: Use-after-free in Linux tc subsystem (v6.15)

[Cong Wang]

On Sat, Jun 28, 2025 at 05:26:59PM -0400, Jamal Hadi Salim wrote:
> On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
> > Hello,
> >
> > I think the testcase I reported earlier actually contains two
> > different bugs. The first is returning SUCCESS with an empty TBF qdisc
> > in tbf_segment, and the second is returning SUCCESS with an empty QFQ
> > qdisc in qfq_enqueue.
> >
> 
> Please join the list where a more general solution is being discussed here:
> https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/

I think that one is different, the one here is related to GSO, the above
linked one is not. Let me think about the GSO issue, since I already
looked into it before.

Thanks.

Re: Use-after-free in Linux tc subsystem (v6.15)

[Jamal Hadi Salim]

On Sun, Jun 29, 2025 at 5:08 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sat, Jun 28, 2025 at 05:26:59PM -0400, Jamal Hadi Salim wrote:
> > On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
> > > Hello,
> > >
> > > I think the testcase I reported earlier actually contains two
> > > different bugs. The first is returning SUCCESS with an empty TBF qdisc
> > > in tbf_segment, and the second is returning SUCCESS with an empty QFQ
> > > qdisc in qfq_enqueue.
> > >
> >
> > Please join the list where a more general solution is being discussed here:
> > https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/
>
> I think that one is different, the one here is related to GSO, the above
> linked one is not. Let me think about the GSO issue, since I already
> looked into it before.

TBH, they all look the same to me - at minimal, they should be tested
against Lion's patch first. Maybe there's a GSO corner case but wasnt
clear to me.

cheers,
jamal

Re: Use-after-free in Linux tc subsystem (v6.15)

[Victor Nogueira]

On 6/30/25 08:06, Jamal Hadi Salim wrote:
> On Sun, Jun 29, 2025 at 5:08 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> On Sat, Jun 28, 2025 at 05:26:59PM -0400, Jamal Hadi Salim wrote:
>>> On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
>>>> Hello,
>>>>
>>>> I think the testcase I reported earlier actually contains two
>>>> different bugs. The first is returning SUCCESS with an empty TBF qdisc
>>>> in tbf_segment, and the second is returning SUCCESS with an empty QFQ
>>>> qdisc in qfq_enqueue.
>>>>
>>>
>>> Please join the list where a more general solution is being discussed here:
>>> https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/
>>
>> I think that one is different, the one here is related to GSO, the above
>> linked one is not. Let me think about the GSO issue, since I already
>> looked into it before.
> 
> TBH, they all look the same to me - at minimal, they should be tested
> against Lion's patch first. Maybe there's a GSO corner case but wasnt
> clear to me.

I did a quick test of Lion's patch using Mingi's C reproducer.
The patch seems to fix the UAF.

cheers,
Victor

Re: Use-after-free in Linux tc subsystem (v6.15)

[Victor Nogueira]

> On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
>>
>> On Fri, Jun 20, 2025 at 8:24 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> On Wed, Jun 18, 2025 at 4:17 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>
>>>> On Mon, Jun 16, 2025 at 9:03 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>>
>>>>> On Sun, Jun 15, 2025 at 10:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jun 12, 2025 at 2:18 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi Mingi,
>>>>>>>
>>>>>>> Thanks for your report!
>>>>>>>
>>>>>>> I won't have time to look into this until this Sunday, if you or
>>>>>>> anyone else have
>>>>>>> time before that, please feel free to work on a patch. Otherwise, I will take a
>>>>>>> look this Sunday.
>>>>>>
>>>>>> I am testing the attached patch, I will take a deeper look tomorrow.
>>>>>
>>>>> It is more complicated than I thought. I think we need to rollback all
>>>>> the previous enqueue operations, but it does not look pretty either.
>>>>>
>>>>> Jamal, do you like the attached fix? I don't have any better ideas
>>>>> so far. :-/
>>>>>
>>>>
>>>> I just got back - let me look at it tomorrow. Immediate reaction is i
>>>> would suspect netem
>>>
>>> Spent time yesterday and there are two potential approaches
>>> (attached), both of which fix the issue but i am not satisfied with
>>> either.
>>>
>>> The root cause being exploited here is there are some qdisc's whose
>>> peek() drops packets - but given peek() doesnt return a code, the
>>> parent is unaware of what happened.
>>>
>>> drr_fix.diff
>>> avoids making a class active  by detecting whether drr_qlen_notify was
>>> called between after enqueue (even though that enqueue succeeded), in
>>> that case, returns a NET_XMIT_SUCCESS | __NET_XMIT_BYPASS which ensure
>>> we don't add the class to drr.
>>>
>>> This fixes the UAF but it would require an analogous fix for other
>>> qdiscs with similar behavior (ets, hfsc, ...)
>>>
>>> qfq_netem_child_fix.diff
>>> piggy backs on your tbf patch and detects whether
>>> qdisc_tree_reduce_backlog was called after qfq's peeked its child
>>> (netem in this repro) in enqueue.
>>> This would also require fixing other qdiscs.
>>>
>>> TBH, while both approaches fix the UAF, IMO they are short term hacks
>>> and i am sure Mingi and co will find yet another way to send netlink
>>> messages to config a _nonsensical hierarchy of qdiscs_ (as was this
>>> one!) to create yet another UAF.
>>>
>>> My suggestion is we go back to a proposal i made a few moons back:
>>> create a mechanism to disallow certain hierarchies of qdiscs, ex in
>>> this case disallow qfq from being the ancestor of "qdiscs that may
>>> drop during peek" (such as netem). Then we can just keep adding more
>>> "disallowed configs" that will be rejected via netlink.
>>> And TBH, i feel like obsoleting qfq altogether - the author doesnt
>>> even respond to emails.
>>>
>>> cheers,
>>> jamal
>>
>> Hello,
>>
>> I think the testcase I reported earlier actually contains two
>> different bugs. The first is returning SUCCESS with an empty TBF qdisc
>> in tbf_segment, and the second is returning SUCCESS with an empty QFQ
>> qdisc in qfq_enqueue.

Mingi, can you create a selftest for the drr UAF case with a qfq child
that you reproduced? Since you required setsockopt for creating GSO
packets, I don't believe you'll be able to use tdc for this.

Re: Use-after-free in Linux tc subsystem (v6.15)

[Cong Wang]

On Mon, Jun 30, 2025 at 08:52:22AM -0300, Victor Nogueira wrote:
> On 6/30/25 08:06, Jamal Hadi Salim wrote:
> > On Sun, Jun 29, 2025 at 5:08 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > 
> > > On Sat, Jun 28, 2025 at 05:26:59PM -0400, Jamal Hadi Salim wrote:
> > > > On Thu, Jun 26, 2025 at 1:11 AM Mingi Cho <mgcho.minic@gmail.com> wrote:
> > > > > Hello,
> > > > > 
> > > > > I think the testcase I reported earlier actually contains two
> > > > > different bugs. The first is returning SUCCESS with an empty TBF qdisc
> > > > > in tbf_segment, and the second is returning SUCCESS with an empty QFQ
> > > > > qdisc in qfq_enqueue.
> > > > > 
> > > > 
> > > > Please join the list where a more general solution is being discussed here:
> > > > https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/
> > > 
> > > I think that one is different, the one here is related to GSO, the above
> > > linked one is not. Let me think about the GSO issue, since I already
> > > looked into it before.
> > 
> > TBH, they all look the same to me - at minimal, they should be tested
> > against Lion's patch first. Maybe there's a GSO corner case but wasnt
> > clear to me.
> 
> I did a quick test of Lion's patch using Mingi's C reproducer.
> The patch seems to fix the UAF.

Good finding! I think Mingi's reproducer still exploits the
->qlen_notify() refcnt to expose issues, but the GSO segmentation itself
is also problematic even without ->qlen_notify(), IMHO.

I have an elegant solution to move the skb_gso_segment() to the upper
layer so that child Qdisc's don't need to handle GSO segmentation any
more. It fixes the UAF too, without Lion's patch.

I will wait for Mingi's response to see if I should submit it for -net
as a bug fix, or -net-next as code refactoring.

Thanks!