[PATCH net] net/sched: act_gact, act_police: range check the fallback control action

[PATCH net] net/sched: act_gact, act_police: range check the fallback control action

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

tcf_action_check_ctrlact() range checks the primary control action:

	if (!opcode)
		ret = action > TC_ACT_VALUE_MAX ? -EINVAL : 0;

TC_ACT_VALUE_MAX is TC_ACT_TRAP, so kernel-internal verdicts above it
cannot be set that way. But act_gact and act_police each carry a second,
independent control action supplied by user space that never reaches that
helper - TCA_GACT_PROB.paction and TCA_POLICE_RESULT. Both only reject
TC_ACT_GOTO_CHAIN, so any other value is stored verbatim and returned
verbatim from the action.

In particular user space can store TC_ACT_CONSUMED, which is
TC_ACT_VALUE_MAX + 1 and is deliberately not part of the UAPI value
range. That verdict tells every caller the action took ownership of the
skb, so nobody frees it: sch_handle_ingress(), sch_handle_egress() and
tcf_qevent_handle() all deliberately skip the free for it. The result is
one leaked sk_buff plus its data buffer per packet traversing the filter,
unbounded, for all traffic on the chain including kernel-generated
packets.

Both are trivially deterministic. act_gact clamps tcfg_pval to >= 1, so
with pval = 1 gact_determ() returns the fallback for every packet.
act_police has no mandatory rate, so rate = 0 leaves tcfp_mtu = ~0 and
tcf_police_mtu_check() always passes.

TC_ACT_CONSUMED was added by commit 720f22fed81b, after both goto-chain
guards were written (9469f375ab09 and c08f5ed5d625, Oct 2018); neither
guard was widened when the new verdict appeared.

Factor the existing range test out of tcf_action_check_ctrlact() as
tcf_action_valid() and apply it to both fallbacks. The helper cannot call
tcf_action_check_ctrlact() directly because that also allocates a
goto_chain, which is exactly what these two sites must not do.

Reproduced on v7.2-rc6: kmemleak reports one leaked 232-byte
skbuff_head_cache object plus its 704-byte data buffer per packet. With
this patch both configurations are rejected with -EINVAL and kmemleak
reports none.

Fixes: 720f22fed81b ("net: sched: refactor reinsert action")
Cc: stable@vger.kernel.org # v5.3+
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 include/net/act_api.h  | 19 +++++++++++++++++++
 net/sched/act_gact.c   |  5 +++++
 net/sched/act_police.c |  6 ++++++
 3 files changed, 30 insertions(+)

Reproducer needs CONFIG_NET_ACT_GACT + CONFIG_GACT_PROB and
CONFIG_NET_ACT_POLICE, plus CONFIG_DEBUG_KMEMLEAK and kmemleak=on to
observe it.

The bad value cannot be set with tc(8) - iproute2 only parses symbolic
action names - so the fallback has to be planted over raw netlink:
TCA_GACT_PROB.paction = 9 with ptype = PGACT_DETERM and pval = 1, or
TCA_POLICE_RESULT = 9 with rate = 0. Attach either to a clsact ingress
chain and every packet leaks its skb.

Before, one sk_buff plus its data buffer per packet:

  kmemleak: 166 new suspected memory leaks
  unreferenced object 0xffff888103baadc0 (size 232):
    kmem_cache_alloc_node_noprof+0x2f1/0x3e0
    __alloc_skb+0xe5/0x860
    alloc_skb_with_frags+0x82/0x750
    sock_alloc_send_pskb+0x658/0x7e0
    packet_sendmsg+0x1833/0x4860
    __x64_sys_sendto+0xe0/0x1c0
    do_syscall_64+0x102/0x5a0

After: both configurations are rejected at netlink time with -EINVAL
and "invalid fallback control action", and kmemleak reports no
unreferenced objects.

For the same reason tdc cannot express the bad configuration, so no
selftest accompanies this patch. A self-contained C reproducer is
available on request.

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 20d9e55f8564..fd03f6319e88 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -270,6 +270,25 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
 struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
 					 struct tcf_chain *newchain);
 
+/* Range check for a control action supplied by user space.
+ *
+ * This is the same test tcf_action_check_ctrlact() applies to the primary
+ * control action, factored out for the *fallback* control actions
+ * (act_gact's TCA_GACT_PROB.paction and act_police's TCA_POLICE_RESULT),
+ * which must not reach tcf_action_check_ctrlact() because they have no
+ * goto_chain to allocate.  Without it, user space can store kernel-internal
+ * verdicts such as TC_ACT_CONSUMED, which is TC_ACT_VALUE_MAX + 1 and is
+ * deliberately not part of the UAPI value range.
+ */
+static inline bool tcf_action_valid(int action)
+{
+	int opcode = TC_ACT_EXT_OPCODE(action);
+
+	if (!opcode)
+		return action <= TC_ACT_VALUE_MAX;
+	return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
+}
+
 #ifdef CONFIG_INET
 DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
 #endif
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index e949280eb800..565860cccba6 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -89,6 +89,11 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 		p_parm = nla_data(tb[TCA_GACT_PROB]);
 		if (p_parm->ptype >= MAX_RAND)
 			return -EINVAL;
+		if (!tcf_action_valid(p_parm->paction)) {
+			NL_SET_ERR_MSG(extack,
+				       "invalid fallback control action");
+			return -EINVAL;
+		}
 		if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN)) {
 			NL_SET_ERR_MSG(extack,
 				       "goto chain not allowed on fallback");
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index b16468a98c55..ce08f6840ef7 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -128,6 +128,12 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
 
 	if (tb[TCA_POLICE_RESULT]) {
 		tcfp_result = nla_get_u32(tb[TCA_POLICE_RESULT]);
+		if (!tcf_action_valid(tcfp_result)) {
+			NL_SET_ERR_MSG(extack,
+				       "invalid fallback control action");
+			err = -EINVAL;
+			goto failure;
+		}
 		if (TC_ACT_EXT_CMP(tcfp_result, TC_ACT_GOTO_CHAIN)) {
 			NL_SET_ERR_MSG(extack,
 				       "goto chain not allowed on fallback");
--
2.43.0

Re: [PATCH net] net/sched: act_gact, act_police: range check the fallback control action

[Jamal Hadi Salim]

On Wed, Aug 5, 2026 at 5:55 AM hyunjungg <hj351016@gmail.com> wrote:
>
> From: Hyunjung Ko <hj351016@gmail.com>
>
> tcf_action_check_ctrlact() range checks the primary control action:
>
>         if (!opcode)
>                 ret = action > TC_ACT_VALUE_MAX ? -EINVAL : 0;
>
> TC_ACT_VALUE_MAX is TC_ACT_TRAP, so kernel-internal verdicts above it
> cannot be set that way. But act_gact and act_police each carry a second,
> independent control action supplied by user space that never reaches that
> helper - TCA_GACT_PROB.paction and TCA_POLICE_RESULT. Both only reject
> TC_ACT_GOTO_CHAIN, so any other value is stored verbatim and returned
> verbatim from the action.
>
> In particular user space can store TC_ACT_CONSUMED, which is
> TC_ACT_VALUE_MAX + 1 and is deliberately not part of the UAPI value
> range. That verdict tells every caller the action took ownership of the
> skb, so nobody frees it: sch_handle_ingress(), sch_handle_egress() and
> tcf_qevent_handle() all deliberately skip the free for it. The result is
> one leaked sk_buff plus its data buffer per packet traversing the filter,
> unbounded, for all traffic on the chain including kernel-generated
> packets.
>
> Both are trivially deterministic. act_gact clamps tcfg_pval to >= 1, so
> with pval = 1 gact_determ() returns the fallback for every packet.
> act_police has no mandatory rate, so rate = 0 leaves tcfp_mtu = ~0 and
> tcf_police_mtu_check() always passes.
>
> TC_ACT_CONSUMED was added by commit 720f22fed81b, after both goto-chain
> guards were written (9469f375ab09 and c08f5ed5d625, Oct 2018); neither
> guard was widened when the new verdict appeared.
>
> Factor the existing range test out of tcf_action_check_ctrlact() as
> tcf_action_valid() and apply it to both fallbacks. The helper cannot call
> tcf_action_check_ctrlact() directly because that also allocates a
> goto_chain, which is exactly what these two sites must not do.
>
> Reproduced on v7.2-rc6: kmemleak reports one leaked 232-byte
> skbuff_head_cache object plus its 704-byte data buffer per packet. With
> this patch both configurations are rejected with -EINVAL and kmemleak
> reports none.
>
> Fixes: 720f22fed81b ("net: sched: refactor reinsert action")
> Cc: stable@vger.kernel.org # v5.3+
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

Two things:

1) We test almost _everything_, so to get a review - even if it as
trivial as this: Always, always send a test case to reproduce even if
it seems as obvious as this. Preferable will be tdc. But you can send
or point to an AI generated poc as well if you cant ask it to create a
tdc test. If the issue is sensitive - send the poc to the tc/netdev
maintainers in a separate email.

2) If you got assistance from an ai - please add assisted-by tag.

Same goes for your other patch...

cheers,
jamal

> ---
>  include/net/act_api.h  | 19 +++++++++++++++++++
>  net/sched/act_gact.c   |  5 +++++
>  net/sched/act_police.c |  6 ++++++
>  3 files changed, 30 insertions(+)
>
> Reproducer needs CONFIG_NET_ACT_GACT + CONFIG_GACT_PROB and
> CONFIG_NET_ACT_POLICE, plus CONFIG_DEBUG_KMEMLEAK and kmemleak=on to
> observe it.
>
> The bad value cannot be set with tc(8) - iproute2 only parses symbolic
> action names - so the fallback has to be planted over raw netlink:
> TCA_GACT_PROB.paction = 9 with ptype = PGACT_DETERM and pval = 1, or
> TCA_POLICE_RESULT = 9 with rate = 0. Attach either to a clsact ingress
> chain and every packet leaks its skb.
>
> Before, one sk_buff plus its data buffer per packet:
>
>   kmemleak: 166 new suspected memory leaks
>   unreferenced object 0xffff888103baadc0 (size 232):
>     kmem_cache_alloc_node_noprof+0x2f1/0x3e0
>     __alloc_skb+0xe5/0x860
>     alloc_skb_with_frags+0x82/0x750
>     sock_alloc_send_pskb+0x658/0x7e0
>     packet_sendmsg+0x1833/0x4860
>     __x64_sys_sendto+0xe0/0x1c0
>     do_syscall_64+0x102/0x5a0
>
> After: both configurations are rejected at netlink time with -EINVAL
> and "invalid fallback control action", and kmemleak reports no
> unreferenced objects.
>
> For the same reason tdc cannot express the bad configuration, so no
> selftest accompanies this patch. A self-contained C reproducer is
> available on request.
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 20d9e55f8564..fd03f6319e88 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -270,6 +270,25 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>  struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
>                                          struct tcf_chain *newchain);
>
> +/* Range check for a control action supplied by user space.
> + *
> + * This is the same test tcf_action_check_ctrlact() applies to the primary
> + * control action, factored out for the *fallback* control actions
> + * (act_gact's TCA_GACT_PROB.paction and act_police's TCA_POLICE_RESULT),
> + * which must not reach tcf_action_check_ctrlact() because they have no
> + * goto_chain to allocate.  Without it, user space can store kernel-internal
> + * verdicts such as TC_ACT_CONSUMED, which is TC_ACT_VALUE_MAX + 1 and is
> + * deliberately not part of the UAPI value range.
> + */
> +static inline bool tcf_action_valid(int action)
> +{
> +       int opcode = TC_ACT_EXT_OPCODE(action);
> +
> +       if (!opcode)
> +               return action <= TC_ACT_VALUE_MAX;
> +       return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
> +}
> +
>  #ifdef CONFIG_INET
>  DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>  #endif
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index e949280eb800..565860cccba6 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -89,6 +89,11 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
>                 p_parm = nla_data(tb[TCA_GACT_PROB]);
>                 if (p_parm->ptype >= MAX_RAND)
>                         return -EINVAL;
> +               if (!tcf_action_valid(p_parm->paction)) {
> +                       NL_SET_ERR_MSG(extack,
> +                                      "invalid fallback control action");
> +                       return -EINVAL;
> +               }
>                 if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN)) {
>                         NL_SET_ERR_MSG(extack,
>                                        "goto chain not allowed on fallback");
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index b16468a98c55..ce08f6840ef7 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -128,6 +128,12 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
>
>         if (tb[TCA_POLICE_RESULT]) {
>                 tcfp_result = nla_get_u32(tb[TCA_POLICE_RESULT]);
> +               if (!tcf_action_valid(tcfp_result)) {
> +                       NL_SET_ERR_MSG(extack,
> +                                      "invalid fallback control action");
> +                       err = -EINVAL;
> +                       goto failure;
> +               }
>                 if (TC_ACT_EXT_CMP(tcfp_result, TC_ACT_GOTO_CHAIN)) {
>                         NL_SET_ERR_MSG(extack,
>                                        "goto chain not allowed on fallback");
> --
> 2.43.0

Re: [PATCH net] net/sched: act_gact, act_police: range check the fallback control action

[Hyunjung Ko]

On Wed, Aug 5, 2026 at 1:59 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> 1) We test almost _everything_, so to get a review - even if it as
> trivial as this: Always, always send a test case to reproduce even if
> it seems as obvious as this. Preferable will be tdc. But you can send
> or point to an AI generated poc as well if you cant ask it to create a
> tdc test. If the issue is sensitive - send the poc to the tc/netdev
> maintainers in a separate email.
>
> 2) If you got assistance from an ai - please add assisted-by tag.
>
> Same goes for your other patch...

Thanks for the review. Both points addressed for both patches; v2 of
each follows shortly.

The act_ct patch now comes with a tdc case (2/2). It uses the scapy
plugin to inject the malformed IPv6 frame and matches on the clsact
drop counter, which turns out to be a clean discriminator: before the
fix act_ct returns TC_ACT_CONSUMED, so tc_run() never reaches its
TC_ACT_SHOT arm and the counter stays at zero while the skbs leak;
after the fix it reads "dropped 10".

To be straight about how far I verified that: I do not have a
scapy-capable tdc setup, so I have not run tdc.py over the case
itself. I ran the equivalent by hand under qemu on both an unpatched
and a patched kernel - same topology, same ten frames, same tc -s
qdisc show - and got "dropped 0" vs "dropped 10". The JSON is modelled
on the existing scapy cases in the same file (3992, 9c2a). Noted below
the --- line of 2/2 as well.

This patch I could not express in tdc. iproute2 only parses symbolic
control-action names, so tc(8) rejects the bad value before it ever
reaches the kernel:

  $ tc actions add action gact drop random determ ok 2
  RTNETLINK answers: Operation not permitted   <- parsed fine
  $ tc actions add action gact drop random determ 9 2
  Bad action type 9                            <- rejected by iproute2

The fallback has to be planted over raw netlink, so I have inlined a
self-contained C reproducer below the --- line of v2 instead. It sets
up the clsact chain, plants TCA_GACT_PROB.paction = 9 and then
TCA_POLICE_RESULT = 9, and reports skbuff_head_cache growth per
injected packet. If you would rather have this as a tdc case anyway I
can write a plugin that does the raw netlink setup, but that looked
like more machinery than a one-line range check warrants - happy to do
it if you disagree.

The AI assistance tag is on both patches now.

Thanks,
Hyunjung