netdev - Re: [PATCH net] net_sched: hfsc: Address reentrant enqueue adding class to eltree twice

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] [day] [month] [year] [list]

Message-ID: <CANn89iKhivLw7e1YQsZORXZuSUqnTD8830K=X6=5h8vvogZ-uQ@mail.gmail.com>
Date: Thu, 22 May 2025 06:17:31 -0700
From: Eric Dumazet <edumazet@...gle.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, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org, Savy <savy@...t3mfailure.io>, Will <will@...lsroot.io>
Subject: Re: [PATCH net] net_sched: hfsc: Address reentrant enqueue adding
 class to eltree twice

On Wed, May 21, 2025 at 1:54 PM Pedro Tammela <pctammela@...atatu.com> wrote:
>
> Savy says:
>     "We are writing to report that this recent patch
>     (141d34391abbb315d68556b7c67ad97885407547) [1]
>     can be bypassed, and a UAF can still occur when HFSC is utilized with
>     NETEM.
>
>     The patch only checks the cl->cl_nactive field to determine whether
>     it is the first insertion or not [2], but this field is only
>     incremented by init_vf [3].
>
>     By using HFSC_RSC (which uses init_ed) [4], it is possible to bypass the
>     check and insert the class twice in the eltree.
>     Under normal conditions, this would lead to an infinite loop in
>     hfsc_dequeue for the reasons we already explained in this report [5].
>
>     However, if TBF is added as root qdisc and it is configured with a
>     very low rate,
>     it can be utilized to prevent packets from being dequeued.
>     This behavior can be exploited to perform subsequent insertions in the
>     HFSC eltree and cause a UAF."
>
> To fix both the UAF and the inifinite loop, with netem as an hfsc child,
> check explicitly in hfsc_enqueue whether the class is already in the eltree
> whenever the HFSC_RSC flag is set.
>
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=141d34391abbb315d68556b7c67ad97885407547
> [2] https://elixir.bootlin.com/linux/v6.15-rc5/source/net/sched/sch_hfsc.c#L1572
> [3] https://elixir.bootlin.com/linux/v6.15-rc5/source/net/sched/sch_hfsc.c#L677
> [4] https://elixir.bootlin.com/linux/v6.15-rc5/source/net/sched/sch_hfsc.c#L1574
> [5] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/T/#u
>
> Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")
> Reported-by: Savy <savy@...t3mfailure.io>
> Reported-by: Will <will@...lsroot.io>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> ---
>  net/sched/sch_hfsc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index cb8c525ea..1c4067bec 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -175,6 +175,11 @@ struct hfsc_sched {
>
>  #define        HT_INFINITY     0xffffffffffffffffULL   /* infinite time value */
>
> +static bool cl_in_el_or_vttree(struct hfsc_class *cl)
> +{
> +       return ((cl->cl_flags & HFSC_FSC) && cl->cl_nactive) ||
> +               ((cl->cl_flags & HFSC_RSC) && !RB_EMPTY_NODE(&cl->el_node));
> +}
>
>  /*
>   * eligible tree holds backlogged classes being sorted by their eligible times.
> @@ -1089,6 +1094,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>                 qdisc_purge_queue(parent->qdisc);
>         hfsc_adjust_levels(parent);
>         sch_tree_unlock(sch);
> +       RB_CLEAR_NODE(&cl->el_node);

Is it safe doing this operation without qdisc lock held ?

I would perform this operation sooner (around line 1043).

>
>         qdisc_class_hash_grow(sch, &q->clhash);
>
> @@ -1569,7 +1575,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
>                 return err;
>         }
>
> -       if (first && !cl->cl_nactive) {
> +       if (first && !cl_in_el_or_vttree(cl)) {
>                 if (cl->cl_flags & HFSC_RSC)
>                         init_ed(cl, len);
>                 if (cl->cl_flags & HFSC_FSC)
> --
> 2.43.0
>