Re: [PATCH] net/sched: act_mirred: Fix blockcast recursion bypass leading to stack overflow

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/sched: act_mirred: Fix blockcast recursion bypass leading to stack overflow

This patch fixes a stack overflow bug where mirred blockcast actions would
bypass the recursion limit check because the recursion counter was not
incremented for blockcast actions.

> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 05e0b14b57731..5928fcf3e6514 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -444,8 +444,12 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
>  	tcf_action_update_bstats(&m->common, skb);
>  
>  	blockid = READ_ONCE(m->tcfm_blockid);
> -	if (blockid)
> -		return tcf_blockcast(skb, m, blockid, res, retval);
> +	if (blockid) {
> +		xmit->sched_mirred_nest++;

Could this leave a stale pointer in sched_mirred_dev since netdev_xmit is
persistent per-CPU (or per-task in PREEMPT_RT) memory?

If a nested mirred action later iterates over sched_mirred_dev up to the new
nesting depth to detect routing loops, could it match a stale pointer from
whatever unrelated packet previously reached this nesting level?

This might cause a false-positive loop detection and incorrectly drop a valid
packet. Should we set xmit->sched_mirred_dev[xmit->sched_mirred_nest] = NULL
before incrementing the counter?

> +		retval = tcf_blockcast(skb, m, blockid, res, retval);

This isn't a regression introduced by this patch, but it exists in the early
error returns of tcf_mirred_act() (e.g., when the target device is gone, or a
routing loop is detected) and in tcf_blockcast() (e.g., when
xa_empty(&block->ports) is true).

In these error paths, the functions simply return retval. For mirred redirect
actions, retval is initialized to m->tcf_action, which is typically
TC_ACT_STOLEN.

When TC_ACT_STOLEN is returned, the TC framework assumes the action has taken
ownership of the packet and will not free the sk_buff. Since the code does
not free the sk_buff before returning in these error paths, does this leak
the sk_buff?

Could an unprivileged user inside a network namespace trigger this
intentionally by creating a routing loop with mirred redirect rules, leading
to continuous sk_buff leaks and system memory exhaustion?

Should these error paths ensure the sk_buff is freed or force the return value
to TC_ACT_SHOT if the action is a redirect, mirroring the existing behavior in
tcf_mirred_to_dev()?

> +		xmit->sched_mirred_nest--;
> +		return retval;
> +	}