netdev - Re: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoM=oC0kPOEcNdng8cmHHGA_kTL+y0mAcwTDXuLfiJhsjyg@mail.gmail.com>
Date: Mon, 7 Apr 2025 15:56:15 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Ilya Maximets <i.maximets@...hat.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net] tc: Return an error if filters try to attach too
 many actions

On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> While developing the fix for the buffer sizing issue in [0], I noticed
> that the kernel will happily accept a long list of actions for a filter,
> and then just silently truncate that list down to a maximum of 32
> actions.
>
> That seems less than ideal, so this patch changes the action parsing to
> return an error message and refuse to create the filter in this case.
> This results in an error like:
>
>  # ip link add type veth
>  # tc qdisc replace dev veth0 root handle 1: fq_codel
>  # tc -echo filter add dev veth0 parent 1: u32 match u32 0 0 $(for i in $(seq 33); do echo action pedit munge ip dport set 22; done)
> Error: Only 32 actions supported per filter.
> We have an error talking to the kernel
>
> Instead of just creating a filter with 32 actions and dropping the last
> one.
>
> Sending as an RFC as this is obviously a change in UAPI. But seeing as
> creating more than 32 filters has never actually *worked*, it could be
> argued that the change is not likely to break any existing workflows.
> But, well, OTOH: https://xkcd.com/1172/
>
> So what do people think? Worth the risk for saner behaviour?
>

I dont know anyone using that many actions per filter, but given it's
a uapi i am more inclined to keep it.
How about just removing the "return -EINVAL" then it becomes a
warning? It would need a 2-3 line change to iproute2 to recognize the
extack with positive ACK from the kernel.

cheers,
jamal


> [0] https://lore.kernel.org/r/20250407105542.16601-1-toke@redhat.com
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
>  net/sched/act_api.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 839790043256..057e20cef375 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1461,17 +1461,29 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>                     struct netlink_ext_ack *extack)
>  {
>         struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
> -       struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
> +       struct nlattr *tb[TCA_ACT_MAX_PRIO + 2];
>         struct tc_action *act;
>         size_t sz = 0;
>         int err;
>         int i;
>
> -       err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
> +       err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO + 1, nla, NULL,
>                                           extack);
>         if (err < 0)
>                 return err;
>
> +       /* The nested attributes are parsed as types, but they are really an
> +        * array of actions. So we parse one more than we can handle, and return
> +        * an error if the last one is set (as that indicates that the request
> +        * contained more than the maximum number of actions).
> +        */
> +       if (tb[TCA_ACT_MAX_PRIO + 1]) {
> +               NL_SET_ERR_MSG_FMT(extack,
> +                                  "Only %d actions supported per filter",
> +                                  TCA_ACT_MAX_PRIO);
> +               return -EINVAL;
> +       }
> +
>         for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
>                 struct tc_action_ops *a_o;
>
> --
> 2.49.0
>