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=EL-KVC-LKC8tyY1BRSYtjEgKPPmcwzAvj+z+fw04gpQ@mail.gmail.com>
Date: Mon, 7 Apr 2025 16:08:39 -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 3:56 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> 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.
>

Removing the return -EINVAL:

$tc actions add `for i in $(seq 33); do echo action gact ok; done`
Warning: Only 32 actions supported per filter.

We do have a tdc testcase which adds 32 actions and verifies, we can
add another one which will be something like above....

cheers,
jamal

> 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
> >