Re: [PATCH net 2/3] net/sched: Handle TC_ACT_REDIRECT from qdisc filter chains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing handling of TC_ACT_CONSUMED in qdisc filter chains leads to Use-After-Free.
--

commit 84137b5068cebac45a2a901c08e1d2f5e6d71111
Author: Jamal Hadi Salim <jhs@mojatatu.com>

net/sched: Handle TC_ACT_REDIRECT from qdisc filter chains

When a TC filter attached to a qdisc filter chain returns TC_ACT_REDIRECT,
the redirect was silently lost and the packet fell through to the switch.
This patch adds a tcf_classify_qdisc wrapper to convert TC_ACT_REDIRECT to
TC_ACT_SHOT, preventing the packet from silently continuing.

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
[ ... ]
> @@ -156,8 +156,20 @@ static inline int tcf_classify(struct sk_buff *skb,
>  {
>  	return TC_ACT_UNSPEC;
>  }
> -
>  #endif
> +static inline int tcf_classify_qdisc(struct sk_buff *skb,
> +				     const struct tcf_proto *tp,
> +				     struct tcf_result *res, bool compat_mode)
> +{
> +	int ret = tcf_classify(skb, NULL, tp, res, compat_mode);
> +
> +	/* TC_ACT_REDIRECT from qdisc filter chains is not supported.
> +	 * Use BPF via tcx or mirred redirect instead.
> +	 */
> +	if (unlikely(ret == TC_ACT_REDIRECT))
> +		ret = TC_ACT_SHOT;
> +	return ret;
> +}

[Severity: Critical]
This is a pre-existing issue, but missing handling of TC_ACT_CONSUMED in
this helper can lead to a use-after-free.

The tcf_classify_qdisc() wrapper intentionally intercepts TC_ACT_REDIRECT
to prevent it from falling through to the qdiscs. However, it fails to
intercept TC_ACT_CONSUMED, which suffers from the exact same fall-through
vulnerability.

If a user configures a qdisc filter with an action that returns
TC_ACT_CONSUMED (such as act_ct queueing an IPv4 fragment for defrag), the
action takes ownership of the skb.

Because TC_ACT_CONSUMED bypasses the switch statements in qdisc classify
functions like cake_classify(), the qdisc treats it as a successful match
and enqueues the already consumed skb into its internal queues.

Should TC_ACT_CONSUMED also be intercepted and handled here in
tcf_classify_qdisc() to prevent this use-after-free scenario?

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
[ ... ]
> @@ -1730,7 +1730,7 @@ static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t,
>  		goto hash;
>  
>  	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> -	result = tcf_classify(skb, NULL, filter, &res, false);
> +	result = tcf_classify_qdisc(skb, filter, &res, false);
>  
>  	if (result >= 0) {
>  #ifdef CONFIG_NET_CLS_ACT

[Severity: Critical]
This is a pre-existing issue, but TC_ACT_CONSUMED bypasses the switch
statement that immediately follows this result check.

When tcf_classify_qdisc() returns TC_ACT_CONSUMED, it satisfies the
result >= 0 check but falls through the switch handling TC_ACT_STOLEN,
TC_ACT_QUEUED, and TC_ACT_TRAP. The qdisc then treats the consumed packet
as a successful match and enqueues it.

Could this be resolved by either adding TC_ACT_CONSUMED to the switch
cases here and in other qdiscs, or by having tcf_classify_qdisc() intercept
it alongside TC_ACT_REDIRECT?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630123331.186840-1-daniel@iogearbox.net?part=2