netdev - Re: [PATCH net v2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it becomes a inner curve

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 17 Oct 2023 08:48:12 -0700
From: Dave Taht <dave.taht@...il.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com, 
	jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, Christian Theune <ct@...ingcircus.io>, 
	Budimir Markovic <markovicbudimir@...il.com>
Subject: Re: [PATCH net v2] net/sched: sch_hfsc: upgrade 'rt' to 'sc' when it
 becomes a inner curve

On occasion I try to get those still using hfsc to give CAKE a shot,
or give me a benchmark as to why the view hfsc as better. It is a very
interesting and hard to understand shaping mechanism.

On Tue, Oct 17, 2023 at 7:36 AM Pedro Tammela <pctammela@...atatu.com> wrote:
>
> 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@...ingcircus.io>
> Cc: Budimir Markovic <markovicbudimir@...il.com>
> Fixes: b3d26c5702c7 ("net/sched: sch_hfsc: Ensure inner classes have fsc curve")
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> ---
>  net/sched/sch_hfsc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index 3554085bc2be..880c5f16b29c 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -902,6 +902,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) },
> @@ -1011,10 +1019,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;
> @@ -1065,6 +1069,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)
> --
> 2.39.2
>
>


-- 
Oct 30: https://netdevconf.info/0x17/news/the-maestro-and-the-music-bof.html
Dave Täht CSO, LibreQos