netdev - Re: [PATCH net-next] 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: <Z/aj8D1TRQBC7QtU@pop-os.localdomain>
Date: Wed, 9 Apr 2025 09:44:32 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
	"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: [PATCH net-next] tc: Return an error if filters try to attach
 too many actions

On Wed, Apr 09, 2025 at 04:55:23PM +0200, Toke Høiland-Jørgensen 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.
> 
> This is obviously a change in UAPI. But seeing as creating more than 32
> filters has never actually *worked*, it seems that returning an explicit
> error is better, and any use cases that get broken by this were already
> broken just in more subtle ways.
> 
> [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;

I wonder ENOSPC is a better errno than EINVAL here?

Thanks.