[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAM0EoMk8dBGaZOUUqw4fbZUVK99Q3xO=uyuCKGE7eQjDELZdQQ@mail.gmail.com>
Date: Thu, 30 Jan 2025 06:17:36 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, jiri@...nulli.us, quanglex97@...il.com,
mincho@...ori.io, Cong Wang <cong.wang@...edance.com>
Subject: Re: [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when
sch->limit == 0
On Fri, Jan 24, 2025 at 1:07 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> From: Quang Le <quanglex97@...il.com>
>
> Expected behaviour:
> In case we reach scheduler's limit, pfifo_tail_enqueue() will drop a
> packet in scheduler's queue and decrease scheduler's qlen by one.
> Then, pfifo_tail_enqueue() enqueue new packet and increase
> scheduler's qlen by one. Finally, pfifo_tail_enqueue() return
> `NET_XMIT_CN` status code.
>
> Weird behaviour:
> In case we set `sch->limit == 0` and trigger pfifo_tail_enqueue() on a
> scheduler that has no packet, the 'drop a packet' step will do nothing.
> This means the scheduler's qlen still has value equal 0.
> Then, we continue to enqueue new packet and increase scheduler's qlen by
> one. In summary, we can leverage pfifo_tail_enqueue() to increase qlen by
> one and return `NET_XMIT_CN` status code.
>
> The problem is:
> Let's say we have two qdiscs: Qdisc_A and Qdisc_B.
> - Qdisc_A's type must have '->graft()' function to create parent/child relationship.
> Let's say Qdisc_A's type is `hfsc`. Enqueue packet to this qdisc will trigger `hfsc_enqueue`.
> - Qdisc_B's type is pfifo_head_drop. Enqueue packet to this qdisc will trigger `pfifo_tail_enqueue`.
> - Qdisc_B is configured to have `sch->limit == 0`.
> - Qdisc_A is configured to route the enqueued's packet to Qdisc_B.
>
> Enqueue packet through Qdisc_A will lead to:
> - hfsc_enqueue(Qdisc_A) -> pfifo_tail_enqueue(Qdisc_B)
> - Qdisc_B->q.qlen += 1
> - pfifo_tail_enqueue() return `NET_XMIT_CN`
> - hfsc_enqueue() check for `NET_XMIT_SUCCESS` and see `NET_XMIT_CN` => hfsc_enqueue() don't increase qlen of Qdisc_A.
>
> The whole process lead to a situation where Qdisc_A->q.qlen == 0 and Qdisc_B->q.qlen == 1.
> Replace 'hfsc' with other type (for example: 'drr') still lead to the same problem.
> This violate the design where parent's qlen should equal to the sum of its childrens'qlen.
>
> Bug impact: This issue can be used for user->kernel privilege escalation when it is reachable.
>
> Reported-by: Quang Le <quanglex97@...il.com>
> Signed-off-by: Quang Le <quanglex97@...il.com>
> Signed-off-by: Cong Wang <cong.wang@...edance.com>
Fixes: 57dbb2d83d100 ?
cheers,
jamal
> ---
> net/sched/sch_fifo.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
> index b50b2c2cc09b..e6bfd39ff339 100644
> --- a/net/sched/sch_fifo.c
> +++ b/net/sched/sch_fifo.c
> @@ -40,6 +40,9 @@ static int pfifo_tail_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> {
> unsigned int prev_backlog;
>
> + if (unlikely(READ_ONCE(sch->limit) == 0))
> + return qdisc_drop(skb, sch, to_free);
> +
> if (likely(sch->q.qlen < READ_ONCE(sch->limit)))
> return qdisc_enqueue_tail(skb, sch);
>
> --
> 2.34.1
>
Powered by blists - more mailing lists