[PATCH net] net/sched: act_gate: reject oversized dumps instead of wrapping them

[PATCH net] net/sched: act_gate: reject oversized dumps instead of wrapping them

[Jamal Hadi Salim]

This is a follow-up to commit cfa165cbfbed ("net/sched: act_gate: budget
the per-entry list in get_fill_size") caught by Sashiko.
cfa165cbfbed sized the add/get reply from the action's real dump but did
not bound the entry count. An oversized gate therefore installs and its
dump emits a structurally corrupt message instead of failing cleanly.

Sashiko also flagged the enlarged reply as potentially something that
will crash the kernel with a memory-amplification / OOM vector.
We were able to recreate this using panic_on_oom=1 (128 concurent
threads GET on a VM sized at 1024M). In the past we have used
panic_on_oom=1; however,  I am weighing-in that: if i have to
create a crash using panic_on_oom=1 then that is a "hardening" issue
and therefore left to net-next. See the discussion with Jakub
(https://lore.kernel.org/netdev/20260914191108.55a1a4f1@kernel.org/).
Note: The issue is resolvable using an entry-count cap, but:
an entry-count cap would also reject gate configurations that work today,
so it cannot justify a stable backport - so policy cap is for net-next;
this patch closes only the malformed uAPI the sizing fix introduced.

parse_gate_list() accepts any number of TCA_GATE_ONE_ENTRY elements; the
only bound is the nlattr header, whose u16 nla_len caps the request at
~5460 minimal 12-byte entries. That is fine for the request, but
tcf_gate_dump() emits ~36 bytes per entry, so past ~1820 entries the
TCA_GATE_ENTRY_LIST nest exceeds U16_MAX and nla_nest_end() writes a
wrapped length. The enclosing TCA_ACT_OPTIONS and per-action nests wrap
the same way, and the outermost TCA_ACT_TAB wraps first, at ~1815
entries, so an oversized reply is already corrupt from there on.

Close those four wrap-capable nests with nla_nest_end_safe(). It returns
-EMSGSIZE instead of writing a wrapped length, and each site already has
an error label that trims and fails the dump. The output is byte
identical for every message that serializes; a message that would wrap
now fails through the same path that returned a clean error before
cfa165cbfbed, so no configuration that works today changes behavior.

Conditions to recreate the bug: CONFIG_NET_SCH_ACT_GATE=y; install a gate
with 1815 or more minimal TCA_GATE_ONE_ENTRY elements (12 bytes each on
the wire, only TCA_GATE_ENTRY_INTERVAL set) and dump it with
RTM_GETACTION. Up to 1814 entries the reply is well formed; from 1815 the
outermost TCA_ACT_TAB nest length wraps (1821 for the innermost
TCA_GATE_ENTRY_LIST). CONFIG_DEBUG_NET=y (default n) additionally trips a
WARN in nla_nest_end(). Installing the gate needs CAP_NET_ADMIN in a user
namespace; the RTM_GETACTION trigger needs no capability once the gate
exists.

Fixes: cfa165cbfbed ("net/sched: act_gate: budget the per-entry list in get_fill_size")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor@mojatatu.com
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c  | 9 ++++++---
 net/sched/act_gate.c | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e45a63be397c..c7e87491a9fc 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -549,7 +549,8 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 		goto nla_put_failure;
 	err = tcf_action_dump_old(skb, a, bind, ref);
 	if (err > 0) {
-		nla_nest_end(skb, nest);
+		if (nla_nest_end_safe(skb, nest) < 0)
+			goto nla_put_failure;
 		return err;
 	}
 
@@ -1270,7 +1271,8 @@ int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
 			tcf_action_dump_1(skb, a, bind, ref);
 		if (err < 0)
 			goto errout;
-		nla_nest_end(skb, nest);
+		if (nla_nest_end_safe(skb, nest) < 0)
+			goto nla_put_failure;
 	}
 
 	return 0;
@@ -1684,7 +1686,8 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
 	if (tcf_action_dump(skb, actions, bind, ref, false) < 0)
 		goto out_nlmsg_trim;
 
-	nla_nest_end(skb, nest);
+	if (nla_nest_end_safe(skb, nest) < 0)
+		goto out_nlmsg_trim;
 
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
 
diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 6d6d45e03c07..14801c604bd9 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -654,7 +654,8 @@ static int tcf_gate_dump(struct sk_buff *skb, struct tc_action *a,
 			goto nla_put_failure;
 	}
 
-	nla_nest_end(skb, entry_list);
+	if (nla_nest_end_safe(skb, entry_list) < 0)
+		goto nla_put_failure;
 
 	tcf_tm_dump(&t, &gact->tcf_tm);
 	if (nla_put_64bit(skb, TCA_GATE_TM, sizeof(t), &t, TCA_GATE_PAD))
-- 
2.43.0

Re: [PATCH net] net/sched: act_gate: reject oversized dumps instead of wrapping them

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new nla_nest_end_safe() failures turn the filter
  notification path into a retry loop that cannot succeed.
- [Medium] The patch is incomplete on the classifier path, which is the
  only path where its inner-nest checks change behaviour.
- [Medium] For gates of about 1815 entries or more, the patch brings back
  the action lifecycle problem that cfa165cbfbed was meant to fix.
- [Low] The commit message and Fixes tag understate what the patch
  changes.

--

net/sched: act_gate: reject oversized dumps instead of wrapping them

This patch closes four nests that can wrap with nla_nest_end_safe():
TCA_GATE_ENTRY_LIST, TCA_ACT_OPTIONS, the per-action nest and
TCA_ACT_TAB. It follows up on cfa165cbfbed, which sized the gate reply
from the real dump without bounding the entry count. When a dump would
wrap, it now fails through the existing trim/error labels instead of
emitting a corrupt nla_len.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor@mojatatu.com

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index e45a63be397ce..c7e87491a9fce 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c

[ ... ]

> @@ -1270,7 +1271,8 @@ int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
>  			tcf_action_dump_1(skb, a, bind, ref);
>  		if (err < 0)
>  			goto errout;
> -		nla_nest_end(skb, nest);
> +		if (nla_nest_end_safe(skb, nest) < 0)
> +			goto nla_put_failure;

[Severity: High]
Can this check make the filter notification path retry forever without
ever succeeding?

Suppose a gate's per-action nest (about 1815 entries) or its
TCA_GATE_ENTRY_LIST (about 1821 entries) is larger than U16_MAX. Then
tcf_action_dump() here, or tcf_gate_dump(), fails no matter how big the
skb is. On the classifier path that failure is reported as -EMSGSIZE:

fl_dump() / mall_dump()
  tcf_exts_dump()
    tcf_action_dump()
      nla_nest_end_safe() fails

net/sched/cls_api.c:tcf_fill_node() {
	...
	int ret = -EMSGSIZE;
	...
}

tfilter_notify_prep() then treats that as a buffer that is too small:

net/sched/cls_api.c:tfilter_notify_prep() {
	...
	if (ret <= 0) {
		kfree_skb(skb);
		if (ret == -EMSGSIZE) {
			size += NLMSG_GOODSIZE;
			goto retry;
		}
		return ERR_PTR(-EINVAL);
	}
	...
}

The number of retries has no limit. The loop only ends when alloc_skb()
fails quietly (__GFP_NOWARN) above MAX_PAGE_ORDER, and then it returns
-ENOBUFS. On 4K pages that is around 4MB, or roughly 1100 iterations.
Each one makes a larger GFP_KERNEL high-order allocation and serializes
every gate entry again. With 64K pages the loop runs much longer.

Before this patch the same loop stopped after about 18 iterations, once
the roughly 66KB dump fit.

RTM_GETTFILTER needs no capability. Once such a filter exists, any
process in the netns can trigger this, repeatedly and from many threads.

tfilter_del_notify() also builds the notification before it deletes:

	skb = tfilter_notify_prep(net, oskb, n, tp, block, q, parent, fh,
				  RTM_DELTFILTER, portid, rtnl_held, extack);
	if (IS_ERR(skb)) {
		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
		return PTR_ERR(skb);
	}

	err = tp->ops->delete(tp, fh, last, rtnl_held, extack);

RTNLGRP_TC accepts listeners without privileges. One such listener would
now make RTM_DELTFILTER for this kind of filter fail after the long
loop, where it used to succeed. RTM_NEWTFILTER notifications would also
be lost.

Doesn't this contradict "no configuration that works today changes
behavior" in the commit message?

One option is to report nest overflow with an errno other than -EMSGSIZE
and pass it up through tcf_exts_dump() and tcf_fill_node(). Another is
to cap the retries in tfilter_notify_prep(). Would either of those work
here?

[Severity: Medium]
Can the classifier path still emit wrapped nest lengths?

This checks each action's nest on its own. The enclosing nests on the
filter path are still closed with plain nla_nest_end():

net/sched/cls_api.c:tcf_exts_dump() {
	...
			if (tcf_action_dump(skb, exts->actions, 0, 0, false)
			    < 0)
				goto nla_put_failure;
			nla_nest_end(skb, nest);
	...
}

fl_dump() and mall_dump() close TCA_OPTIONS the same way.

Take a flower or matchall filter with two gates of about 1000 entries
each. Each per-action nest is about 36KB and passes the new check, but
the exts->action nest adds up to about 73KB.

A single gate of about 1805-1814 entries is well formed on
RTM_GETACTION. On a flower filter, though, the extra key attributes can
push TCA_FLOWER_ACT/TCA_OPTIONS past U16_MAX.

tfilter_notify_prep() keeps growing the skb until the fill succeeds. So
RTM_GETTFILTER would still return a gate dump inside a nest with a
wrapped nla_len.

The wrap itself is older than this patch; it comes from the retry loop
added in 369609fc6272c2. Still, the new inner checks only change
behavior on this classifier path. tca_get_fill() already checks
TCA_ACT_TAB, and tcf_dump_walker() uses skbs of about 32KB.

Should tcf_exts_dump(), fl_dump() and mall_dump() also use
nla_nest_end_safe(), so that "reject oversized dumps instead of wrapping
them" also holds for filter dumps?

>  	}
>  
>  	return 0;
> @@ -1684,7 +1686,8 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
>  	if (tcf_action_dump(skb, actions, bind, ref, false) < 0)
>  		goto out_nlmsg_trim;
>  
> -	nla_nest_end(skb, nest);
> +	if (nla_nest_end_safe(skb, nest) < 0)
> +		goto out_nlmsg_trim;

[Severity: Medium]
For gates of about 1815 entries or more, does this bring back the
add/delete lifecycle problem that cfa165cbfbed was meant to fix?

In tcf_action_add(), tcf_action_init() has already committed the gate
to the IDR before the notification is built:

	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);

	/* only put bound actions */
	tca_put_bound_many(actions, init_res);

Consider the case where there is an RTNLGRP_TC listener (tc monitor, for
example) or NLM_F_ECHO is set. tcf_add_notify_msg()->tca_get_fill() now
fails here, so user space gets -EINVAL but the gate stays installed.

The message of cfa165cbfbed describes this exact case: "RTM_NEWACTION
then fails the add-notify with -EINVAL while the action is already
committed to the IDR".

The delete side also builds the message before it deletes:

net/sched/act_api.c:tcf_del_notify() {
	...
		skb = tcf_del_notify_msg(net, n, actions, portid, attr_size,
					 extack);
		if (IS_ERR(skb))
			return PTR_ERR(skb);
	}

	/* now do the delete */
	ret = tcf_action_delete(net, actions);
	...
}

With a listener present, RTM_DELACTION by index can no longer remove the
gate. Only a flush (NLM_F_ROOT) or netns teardown frees it. RTNLGRP_TC
accepts members without privileges, so an unprivileged listener is
enough to cause this.

In the parent tree both operations succeeded, though they sent a wrapped
notification. Is the statement "no configuration that works today
changes behavior" accurate for this case?

The init-time entry cap that would avoid this has been deferred to
net-next.

[Severity: Low]
This isn't a bug, but the commit message and Fixes tag look narrower
than what this hunk changes. The commit message says:

  this patch closes only the malformed uAPI the sizing fix introduced.

The TCA_ACT_TAB check in tca_get_fill() is core act_api code, so it
applies to every action type. That wrap could already happen without
cfa165cbfbed. Since 8e2efb3f45a5c6 ("net/sched: add get_fill_size
callbacks for actions missing them"), tcf_bpf_get_fill_size() budgets
this:

		size += nla_total_size(prog->bpf_num_ops *
				       sizeof(struct sock_filter));

For a classic BPF action with 4096 instructions that is up to 32KB.

tca_action_gd() adds up the fill sizes of up to TCA_ACT_MAX_PRIO
actions. tcf_get_notify() and tcf_del_notify_msg() then allocate
max(attr_size, NLMSG_GOODSIZE).

So an RTM_GETACTION or RTM_DELACTION that names two large classic BPF
actions by index already overflowed TCA_ACT_TAB. This patch now turns
those replies into -EINVAL.

The filter-path wrap of the gate's inner nests also goes back to
369609fc6272c2, not cfa165cbfbed.

Could the commit message mention the effect on other action types, and
could the Fixes tags point at the commits that introduced these wraps?
8e2efb3f45a5c6 and cfa165cbfbed both carry Fixes: 4e76e75d6aba and will
probably be backported together, so the practical impact is small.

>  
>  	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-H19Z.v1.20261001053234%40mojatatu.com