| Message ID | 20241024165547.418570-1-jhs@mojatatu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2e95c4384438adeaa772caa560244b1a2efef816 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-n] net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT | expand |
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>
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.
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
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?
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!
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)