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@xxxxxxxxx> wrote:
>
> From: Hyunjung Ko <hj351016@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx # v5.3+
> Signed-off-by: Hyunjung Ko <hj351016@xxxxxxxxx>

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