netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch net 0/4] net_sched: two security bug fixes and test cases
@ 2025-01-24  6:07 Cong Wang
  2025-01-24  6:07 ` [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Cong Wang @ 2025-01-24  6:07 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, quanglex97, mincho, Cong Wang

From: Cong Wang <cong.wang@bytedance.com>

This patchset contains two bug fixes reported in security mailing list,
and test cases for each of them.

---

Cong Wang (2):
  netem: update sch->q.qlen before qdisc_tree_reduce_backlog()
  selftests/tc-testing: add tests for qdisc_tree_reduce_backlog

Quang Le (2):
  pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  Add test case to check for pfifo_tail_enqueue() behaviour when limit
    == 0

 net/sched/sch_fifo.c                          |  3 ++
 net/sched/sch_netem.c                         |  2 +-
 .../tc-testing/tc-tests/infra/qdiscs.json     | 34 ++++++++++++++++++-
 .../tc-testing/tc-tests/qdiscs/fifo.json      | 25 ++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-01-24  6:07 [Patch net 0/4] net_sched: two security bug fixes and test cases Cong Wang
@ 2025-01-24  6:07 ` Cong Wang
  2025-01-30 11:17   ` Jamal Hadi Salim
  2025-01-24  6:07 ` [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit " Cong Wang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2025-01-24  6:07 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, quanglex97, mincho, Cong Wang

From: Quang Le <quanglex97@gmail.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@gmail.com>
Signed-off-by: Quang Le <quanglex97@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit == 0
  2025-01-24  6:07 [Patch net 0/4] net_sched: two security bug fixes and test cases Cong Wang
  2025-01-24  6:07 ` [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
@ 2025-01-24  6:07 ` Cong Wang
  2025-01-24 11:37   ` Simon Horman
  2025-01-24  6:07 ` [Patch net 3/4] netem: update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
  2025-01-24  6:07 ` [Patch net 4/4] selftests/tc-testing: add tests for qdisc_tree_reduce_backlog Cong Wang
  3 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2025-01-24  6:07 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, quanglex97, mincho, Cong Wang

From: Quang Le <quanglex97@gmail.com>

When limit == 0, pfifo_tail_enqueue() must drop new packet and
increase dropped packets count of scheduler.

Signed-off-by: Quang Le <quanglex97@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 .../tc-testing/tc-tests/qdiscs/fifo.json      | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
index ae3d286a32b2..f5e08ae9bb7d 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
@@ -313,6 +313,31 @@
         "matchPattern": "qdisc bfifo 1: root",
         "matchCount": "0",
         "teardown": [
+	]
+    },
+    {
+        "id": "d774",
+        "name": "Check pfifo_head_drop qdisc enqueue behaviour when limit == 0",
+        "category": [
+            "qdisc",
+            "pfifo_head_drop"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link add dev dummy2 mtu 1279 type dummy || true",
+            "$IP addr add 10.10.10.10/24 dev dummy2 || true",
+            "$TC qdisc add dev dummy2 root handle 1: pfifo_head_drop limit 0",
+            "$IP link set dev dummy2 up || true"
+        ],
+        "cmdUnderTest": "ping -c10 -W0.01 -I dummy2 10.10.10.1",
+        "expExitCode": "1",
+        "verifyCmd": "$TC -s qdisc show dev dummy2",
+        "matchPattern": "dropped 10",
+        "matchCount": "1",
+        "teardown": [
+            "$IP link del dev dummy2"
         ]
     }
 ]
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch net 3/4] netem: update sch->q.qlen before qdisc_tree_reduce_backlog()
  2025-01-24  6:07 [Patch net 0/4] net_sched: two security bug fixes and test cases Cong Wang
  2025-01-24  6:07 ` [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
  2025-01-24  6:07 ` [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit " Cong Wang
@ 2025-01-24  6:07 ` Cong Wang
  2025-01-24  6:07 ` [Patch net 4/4] selftests/tc-testing: add tests for qdisc_tree_reduce_backlog Cong Wang
  3 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2025-01-24  6:07 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, quanglex97, mincho, Cong Wang, Martin Ottens

From: Cong Wang <cong.wang@bytedance.com>

qdisc_tree_reduce_backlog() notifies parent qdisc only if child
qdisc becomes empty, therefore we need to reduce the backlog of the
child qdisc before calling it. Otherwise it would become a nop and
result in UAF in DRR case (which is integrated in the following patch).

Fixes: f8d4bc455047 ("net/sched: netem: account for backlog updates from child qdisc")
Cc: Martin Ottens <martin.ottens@fau.de>
Reported-by: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 net/sched/sch_netem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 71ec9986ed37..fdd79d3ccd8c 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -749,9 +749,9 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
 				if (err != NET_XMIT_SUCCESS) {
 					if (net_xmit_drop_count(err))
 						qdisc_qstats_drop(sch);
-					qdisc_tree_reduce_backlog(sch, 1, pkt_len);
 					sch->qstats.backlog -= pkt_len;
 					sch->q.qlen--;
+					qdisc_tree_reduce_backlog(sch, 1, pkt_len);
 				}
 				goto tfifo_dequeue;
 			}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch net 4/4] selftests/tc-testing: add tests for qdisc_tree_reduce_backlog
  2025-01-24  6:07 [Patch net 0/4] net_sched: two security bug fixes and test cases Cong Wang
                   ` (2 preceding siblings ...)
  2025-01-24  6:07 ` [Patch net 3/4] netem: update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
@ 2025-01-24  6:07 ` Cong Wang
  3 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2025-01-24  6:07 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, quanglex97, mincho, Cong Wang

From: Cong Wang <cong.wang@bytedance.com>

Integrate the test case provided by Mingi Cho into TDC.

All test results:

1..4
ok 1 ca5e - Check class delete notification for ffff:
ok 2 e4b7 - Check class delete notification for root ffff:
ok 3 33a9 - Check ingress is not searchable on backlog update
ok 4 a4b9 - Check class qlen notification

Cc: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 34 ++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
index d3dd65b05b5f..5810869a0636 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -94,5 +94,37 @@
             "$TC qdisc del dev $DUMMY ingress",
             "$IP addr del 10.10.10.10/24 dev $DUMMY"
         ]
-    }
+    },
+    {
+	"id": "a4b9",
+	"name": "Check class qlen notification",
+	"category": [
+	    "qdisc"
+	],
+	"plugins": {
+	    "requires": "nsPlugin"
+	},
+	"setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY root handle 1: drr",
+            "$TC filter add dev $DUMMY parent 1: basic classid 1:1",
+            "$TC class add dev $DUMMY parent 1: classid 1:1 drr",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2: netem",
+            "$TC qdisc add dev $DUMMY parent 2: handle 3: drr",
+            "$TC filter add dev $DUMMY parent 3: basic action drop",
+            "$TC class add dev $DUMMY parent 3: classid 3:1 drr",
+            "$TC class del dev $DUMMY classid 1:1",
+            "$TC class add dev $DUMMY parent 1: classid 1:1 drr"
+        ],
+        "cmdUnderTest": "ping -c1 -W0.01 -I $DUMMY 10.10.10.1",
+        "expExitCode": "1",
+        "verifyCmd": "$TC qdisc ls dev $DUMMY",
+        "matchPattern": "drr 1: root",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY root handle 1: drr",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY"
+        ]
+   }
 ]
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit == 0
  2025-01-24  6:07 ` [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit " Cong Wang
@ 2025-01-24 11:37   ` Simon Horman
  2025-01-26  3:20     ` Cong Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2025-01-24 11:37 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jhs, jiri, quanglex97, mincho, Cong Wang

On Thu, Jan 23, 2025 at 10:07:38PM -0800, Cong Wang wrote:
> From: Quang Le <quanglex97@gmail.com>
> 
> When limit == 0, pfifo_tail_enqueue() must drop new packet and
> increase dropped packets count of scheduler.
> 
> Signed-off-by: Quang Le <quanglex97@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>

Hi Cong, all,

This test is reporting "not ok" in the Netdev CI.

# not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
# Could not match regex pattern. Verify command output:
# qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
#  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
#  backlog 0b 0p requeues 0

...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit == 0
  2025-01-24 11:37   ` Simon Horman
@ 2025-01-26  3:20     ` Cong Wang
  2025-01-26  3:52       ` Cong Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2025-01-26  3:20 UTC (permalink / raw)
  To: Simon Horman; +Cc: netdev, jhs, jiri, quanglex97, mincho, Cong Wang

On Fri, Jan 24, 2025 at 11:37:43AM +0000, Simon Horman wrote:
> On Thu, Jan 23, 2025 at 10:07:38PM -0800, Cong Wang wrote:
> > From: Quang Le <quanglex97@gmail.com>
> > 
> > When limit == 0, pfifo_tail_enqueue() must drop new packet and
> > increase dropped packets count of scheduler.
> > 
> > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> 
> Hi Cong, all,
> 
> This test is reporting "not ok" in the Netdev CI.
> 
> # not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> # Could not match regex pattern. Verify command output:
> # qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
> #  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> #  backlog 0b 0p requeues 0

Oops... It worked on my side, let me take a look.

Thanks for reporting!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit == 0
  2025-01-26  3:20     ` Cong Wang
@ 2025-01-26  3:52       ` Cong Wang
  2025-01-27 17:54         ` Simon Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2025-01-26  3:52 UTC (permalink / raw)
  To: Simon Horman; +Cc: netdev, jhs, jiri, quanglex97, mincho, Cong Wang

On Sat, Jan 25, 2025 at 07:20:42PM -0800, Cong Wang wrote:
> On Fri, Jan 24, 2025 at 11:37:43AM +0000, Simon Horman wrote:
> > On Thu, Jan 23, 2025 at 10:07:38PM -0800, Cong Wang wrote:
> > > From: Quang Le <quanglex97@gmail.com>
> > > 
> > > When limit == 0, pfifo_tail_enqueue() must drop new packet and
> > > increase dropped packets count of scheduler.
> > > 
> > > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > 
> > Hi Cong, all,
> > 
> > This test is reporting "not ok" in the Netdev CI.
> > 
> > # not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> > # Could not match regex pattern. Verify command output:
> > # qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
> > #  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> > #  backlog 0b 0p requeues 0
> 
> Oops... It worked on my side, let me take a look.
> 

I ran it again for multiple times, it still worked for me:

1..16
ok 1 a519 - Add bfifo qdisc with system default parameters on egress
ok 2 585c - Add pfifo qdisc with system default parameters on egress
ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
ok 12 1298 - Add duplicate bfifo qdisc on egress
ok 13 45a0 - Delete nonexistent bfifo qdisc
ok 14 972b - Add prio qdisc on egress with invalid format for handles
ok 15 4d39 - Delete bfifo qdisc twice
ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0

Could you provide a link for me to check?

Just in case, please make sure patch 1/4 is applied before this test,
otherwise packets would not be dropped.

Meanwhile, I do need to update this patch anyway, because it hardcoded
dummy2...

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit == 0
  2025-01-26  3:52       ` Cong Wang
@ 2025-01-27 17:54         ` Simon Horman
  0 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2025-01-27 17:54 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jhs, jiri, quanglex97, mincho, Cong Wang

On Sat, Jan 25, 2025 at 07:52:39PM -0800, Cong Wang wrote:
> On Sat, Jan 25, 2025 at 07:20:42PM -0800, Cong Wang wrote:
> > On Fri, Jan 24, 2025 at 11:37:43AM +0000, Simon Horman wrote:
> > > On Thu, Jan 23, 2025 at 10:07:38PM -0800, Cong Wang wrote:
> > > > From: Quang Le <quanglex97@gmail.com>
> > > > 
> > > > When limit == 0, pfifo_tail_enqueue() must drop new packet and
> > > > increase dropped packets count of scheduler.
> > > > 
> > > > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > > > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > > 
> > > Hi Cong, all,
> > > 
> > > This test is reporting "not ok" in the Netdev CI.
> > > 
> > > # not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> > > # Could not match regex pattern. Verify command output:
> > > # qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
> > > #  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> > > #  backlog 0b 0p requeues 0
> > 
> > Oops... It worked on my side, let me take a look.
> > 
> 
> I ran it again for multiple times, it still worked for me:
> 
> 1..16
> ok 1 a519 - Add bfifo qdisc with system default parameters on egress
> ok 2 585c - Add pfifo qdisc with system default parameters on egress
> ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
> ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
> ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
> ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
> ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
> ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
> ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
> ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
> ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
> ok 12 1298 - Add duplicate bfifo qdisc on egress
> ok 13 45a0 - Delete nonexistent bfifo qdisc
> ok 14 972b - Add prio qdisc on egress with invalid format for handles
> ok 15 4d39 - Delete bfifo qdisc twice
> ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> 
> Could you provide a link for me to check?

I'll send you some links off-list.

> 
> Just in case, please make sure patch 1/4 is applied before this test,
> otherwise packets would not be dropped.

Yes, I am pretty sure that is the case.

> 
> Meanwhile, I do need to update this patch anyway, because it hardcoded
> dummy2...
> 
> Thanks.
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-01-24  6:07 ` [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
@ 2025-01-30 11:17   ` Jamal Hadi Salim
  2025-01-31 21:53     ` Cong Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Jamal Hadi Salim @ 2025-01-30 11:17 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jiri, quanglex97, mincho, Cong Wang

On Fri, Jan 24, 2025 at 1:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> From: Quang Le <quanglex97@gmail.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@gmail.com>
> Signed-off-by: Quang Le <quanglex97@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-01-30 11:17   ` Jamal Hadi Salim
@ 2025-01-31 21:53     ` Cong Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2025-01-31 21:53 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: netdev, jiri, quanglex97, mincho, Cong Wang

On Thu, Jan 30, 2025 at 06:17:36AM -0500, Jamal Hadi Salim wrote:
> On Fri, Jan 24, 2025 at 1:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > From: Quang Le <quanglex97@gmail.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@gmail.com>
> > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> 
> Fixes: 57dbb2d83d100 ?

Ah, probably. I will add the Fixes tag in v3 after I resolve the
selftest issue.

Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-01-31 21:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-24  6:07 [Patch net 0/4] net_sched: two security bug fixes and test cases Cong Wang
2025-01-24  6:07 ` [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
2025-01-30 11:17   ` Jamal Hadi Salim
2025-01-31 21:53     ` Cong Wang
2025-01-24  6:07 ` [Patch net 2/4] Add test case to check for pfifo_tail_enqueue() behaviour when limit " Cong Wang
2025-01-24 11:37   ` Simon Horman
2025-01-26  3:20     ` Cong Wang
2025-01-26  3:52       ` Cong Wang
2025-01-27 17:54         ` Simon Horman
2025-01-24  6:07 ` [Patch net 3/4] netem: update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
2025-01-24  6:07 ` [Patch net 4/4] selftests/tc-testing: add tests for qdisc_tree_reduce_backlog Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).