[PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[Jamal Hadi Salim]

From: Pedro Tammela <pctammela@mojatatu.com>

In qdisc_tree_reduce_backlog, Qdiscs with major handle ffff: are assumed
to be either root or ingress. This assumption is bogus since it's valid
to create egress qdiscs with major handle ffff:
Budimir Markovic found that for qdiscs like DRR that maintain an active
class list, it will cause a UAF with a dangling class pointer.

In 066a3b5b2346, the concern was to avoid iterating over the ingress
qdisc since its parent is itself. The proper fix is to stop when parent
TC_H_ROOT is reached because the only way to retrieve ingress is when a
hierarchy which does not contain a ffff: major handle call into
qdisc_lookup with TC_H_MAJ(TC_H_ROOT).

In the scenario where major ffff: is an egress qdisc in any of the tree
levels, the updates will also propagate to TC_H_ROOT, which then the
iteration must stop.

Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix qdisc_tree_decrease_qlen() loop")
Reported-by: Budimir Markovic <markovicbudimir@gmail.com>
Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 2eefa4783879..a1d27bc039a3 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -791,7 +791,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 	drops = max_t(int, n, 0);
 	rcu_read_lock();
 	while ((parentid = sch->parent)) {
-		if (TC_H_MAJ(parentid) == TC_H_MAJ(TC_H_INGRESS))
+		if (parentid == TC_H_ROOT)
 			break;
 
 		if (sch->flags & TCQ_F_NOPARENT)
-- 
2.34.1

Re: [PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[Simon Horman]

On Thu, Oct 24, 2024 at 12:55:47PM -0400, Jamal Hadi Salim wrote:
> From: Pedro Tammela <pctammela@mojatatu.com>
> 
> In qdisc_tree_reduce_backlog, Qdiscs with major handle ffff: are assumed
> to be either root or ingress. This assumption is bogus since it's valid
> to create egress qdiscs with major handle ffff:
> Budimir Markovic found that for qdiscs like DRR that maintain an active
> class list, it will cause a UAF with a dangling class pointer.
> 
> In 066a3b5b2346, the concern was to avoid iterating over the ingress
> qdisc since its parent is itself. The proper fix is to stop when parent
> TC_H_ROOT is reached because the only way to retrieve ingress is when a
> hierarchy which does not contain a ffff: major handle call into
> qdisc_lookup with TC_H_MAJ(TC_H_ROOT).
> 
> In the scenario where major ffff: is an egress qdisc in any of the tree
> levels, the updates will also propagate to TC_H_ROOT, which then the
> iteration must stop.
> 
> Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix qdisc_tree_decrease_qlen() loop")
> Reported-by: Budimir Markovic <markovicbudimir@gmail.com>
> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[Cong Wang]

On Thu, Oct 24, 2024 at 12:55:47PM -0400, Jamal Hadi Salim wrote:
> From: Pedro Tammela <pctammela@mojatatu.com>
> 
> In qdisc_tree_reduce_backlog, Qdiscs with major handle ffff: are assumed
> to be either root or ingress. This assumption is bogus since it's valid
> to create egress qdiscs with major handle ffff:
> Budimir Markovic found that for qdiscs like DRR that maintain an active
> class list, it will cause a UAF with a dangling class pointer.
> 
> In 066a3b5b2346, the concern was to avoid iterating over the ingress
> qdisc since its parent is itself. The proper fix is to stop when parent
> TC_H_ROOT is reached because the only way to retrieve ingress is when a
> hierarchy which does not contain a ffff: major handle call into
> qdisc_lookup with TC_H_MAJ(TC_H_ROOT).
> 
> In the scenario where major ffff: is an egress qdisc in any of the tree
> levels, the updates will also propagate to TC_H_ROOT, which then the
> iteration must stop.
> 
> Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix qdisc_tree_decrease_qlen() loop")
> Reported-by: Budimir Markovic <markovicbudimir@gmail.com>
> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> 
>  net/sched/sch_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Can we also add a selftest since it is reproducible?

I am not saying you have to put it together with this patch, a separate patch is
certainly okay.

Thanks.

Re: [PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[Pedro Tammela]

On 26/10/2024 13:47, Cong Wang wrote:
> On Thu, Oct 24, 2024 at 12:55:47PM -0400, Jamal Hadi Salim wrote:
>> From: Pedro Tammela <pctammela@mojatatu.com>
>>
>> In qdisc_tree_reduce_backlog, Qdiscs with major handle ffff: are assumed
>> to be either root or ingress. This assumption is bogus since it's valid
>> to create egress qdiscs with major handle ffff:
>> Budimir Markovic found that for qdiscs like DRR that maintain an active
>> class list, it will cause a UAF with a dangling class pointer.
>>
>> In 066a3b5b2346, the concern was to avoid iterating over the ingress
>> qdisc since its parent is itself. The proper fix is to stop when parent
>> TC_H_ROOT is reached because the only way to retrieve ingress is when a
>> hierarchy which does not contain a ffff: major handle call into
>> qdisc_lookup with TC_H_MAJ(TC_H_ROOT).
>>
>> In the scenario where major ffff: is an egress qdisc in any of the tree
>> levels, the updates will also propagate to TC_H_ROOT, which then the
>> iteration must stop.
>>
>> Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix qdisc_tree_decrease_qlen() loop")
>> Reported-by: Budimir Markovic <markovicbudimir@gmail.com>
>> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Tested-by: Victor Nogueira <victor@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>>   net/sched/sch_api.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
> 
> Can we also add a selftest since it is reproducible?

Yep, we are just waiting for it to be in net-next so we don't crash 
downstream CIs

Re: [PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[Jakub Kicinski]

On Mon, 28 Oct 2024 11:36:00 -0300 Pedro Tammela wrote:
> > Can we also add a selftest since it is reproducible?  
> 
> Yep, we are just waiting for it to be in net-next so we don't crash 
> downstream CIs

Can you say more? It's more common to include the test in the same
series as the fix. Which CI would break?

Re: [PATCH net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 24 Oct 2024 12:55:47 -0400 you wrote:
> From: Pedro Tammela <pctammela@mojatatu.com>
> 
> In qdisc_tree_reduce_backlog, Qdiscs with major handle ffff: are assumed
> to be either root or ingress. This assumption is bogus since it's valid
> to create egress qdiscs with major handle ffff:
> Budimir Markovic found that for qdiscs like DRR that maintain an active
> class list, it will cause a UAF with a dangling class pointer.
> 
> [...]

Here is the summary with links:
  - [net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT
    https://git.kernel.org/netdev/net/c/2e95c4384438

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html