Skip to content

Commit

Permalink
net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve
Browse files Browse the repository at this point in the history
jira VULN-6713
cve CVE-2023-4623
commit-author Pedro Tammela <pctammela@mojatatu.com>
commit a13b67c

Christian Theune says:
   I upgraded from 6.1.38 to 6.1.55 this morning and it broke my traffic shaping script,
   leaving me with a non-functional uplink on a remote router.

A 'rt' curve cannot be used as a inner curve (parent class), but we were
allowing such configurations since the qdisc was introduced. Such
configurations would trigger a UAF as Budimir explains:
   The parent will have vttree_insert() called on it in init_vf(),
   but will not have vttree_remove() called on it in update_vf()
   because it does not have the HFSC_FSC flag set.

The qdisc always assumes that inner classes have the HFSC_FSC flag set.
This is by design as it doesn't make sense 'qdisc wise' for an 'rt'
curve to be an inner curve.

Budimir's original patch disallows users to add classes with a 'rt'
parent, but this is too strict as it breaks users that have been using
'rt' as a inner class. Another approach, taken by this patch, is to
upgrade the inner 'rt' into a 'sc', warning the user in the process.
It avoids the UAF reported by Budimir while also being more permissive
to bad scripts/users/code using 'rt' as a inner class.

Users checking the `tc class ls [...]` or `tc class get [...]` dumps would
observe the curve change and are potentially breaking with this change.

v1->v2: https://lore.kernel.org/all/20231013151057.2611860-1-pctammela@mojatatu.com/
- Correct 'Fixes' tag and merge with revert (Jakub)

	Cc: Christian Theune <ct@flyingcircus.io>
	Cc: Budimir Markovic <markovicbudimir@gmail.com>
Fixes: b3d26c5 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve")
	Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
	Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://lore.kernel.org/r/20231017143602.3191556-1-pctammela@mojatatu.com
	Signed-off-by: Jakub Kicinski <kuba@kernel.org>
(cherry picked from commit a13b67c)
	Signed-off-by: Marcin Wcisło <marcin.wcislo@conclusive.pl>
  • Loading branch information
pvts-mat committed Feb 20, 2025
1 parent 649027b commit bea021e
Showing 1 changed file with 14 additions and 4 deletions.
18 changes: 14 additions & 4 deletions net/sched/sch_hfsc.c
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,14 @@ hfsc_change_usc(struct hfsc_class *cl, struct tc_service_curve *usc,
cl->cl_flags |= HFSC_USC;
}

static void
hfsc_upgrade_rt(struct hfsc_class *cl)
{
cl->cl_fsc = cl->cl_rsc;
rtsc_init(&cl->cl_virtual, &cl->cl_fsc, cl->cl_vt, cl->cl_total);
cl->cl_flags |= HFSC_FSC;
}

static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
[TCA_HFSC_RSC] = { .len = sizeof(struct tc_service_curve) },
[TCA_HFSC_FSC] = { .len = sizeof(struct tc_service_curve) },
Expand Down Expand Up @@ -1012,10 +1020,6 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (parent == NULL)
return -ENOENT;
}
if (!(parent->cl_flags & HFSC_FSC) && parent != &q->root) {
NL_SET_ERR_MSG(extack, "Invalid parent - parent class must have FSC");
return -EINVAL;
}

if (classid == 0 || TC_H_MAJ(classid ^ sch->handle) != 0)
return -EINVAL;
Expand Down Expand Up @@ -1066,6 +1070,12 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
cl->cf_tree = RB_ROOT;

sch_tree_lock(sch);
/* Check if the inner class is a misconfigured 'rt' */
if (!(parent->cl_flags & HFSC_FSC) && parent != &q->root) {
NL_SET_ERR_MSG(extack,
"Forced curve change on parent 'rt' to 'sc'");
hfsc_upgrade_rt(parent);
}
qdisc_class_hash_insert(&q->clhash, &cl->cl_common);
list_add_tail(&cl->siblings, &parent->children);
if (parent->level == 0)
Expand Down

0 comments on commit bea021e

Please sign in to comment.