netdev - Re: [PATCH net v3] net/sched: cls_flower: Reject invalid ct

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] [day] [month] [year] [list]

Message-ID: <20210206125121.18e19eaa@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 6 Feb 2021 12:51:21 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     wenxu@...oud.cn
Cc:     mleitner@...hat.com, netdev@...r.kernel.org, jhs@...atatu.com
Subject: Re: [PATCH net v3] net/sched: cls_flower: Reject invalid ct_state
 flags rules

On Fri,  5 Feb 2021 15:07:30 +0800 wenxu@...oud.cn wrote:
> From: wenxu <wenxu@...oud.cn>
> 
> Reject the unsupported and invalid ct_state flags of cls flower rules.
> 
> Fixes: e0ace68af2ac ("net/sched: cls_flower: Add matching on conntrack info")
> Signed-off-by: wenxu <wenxu@...oud.cn>
> ---
> v3: using NLA_POLICY_MASK and NL_SET_ERR_MSG_ATTR
> 
>  include/uapi/linux/pkt_cls.h |  7 +++++++
>  net/sched/cls_flower.c       | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index ee95f42..77df582 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -591,8 +591,15 @@ enum {
>  	TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED = 1 << 1, /* Part of an existing connection. */
>  	TCA_FLOWER_KEY_CT_FLAGS_RELATED = 1 << 2, /* Related to an established connection. */
>  	TCA_FLOWER_KEY_CT_FLAGS_TRACKED = 1 << 3, /* Conntrack has occurred. */
> +
> +	__TCA_FLOWER_KEY_CT_FLAGS_MAX,
>  };
>  
> +#define TCA_FLOWER_KEY_CT_FLAGS_MAX \
> +		((__TCA_FLOWER_KEY_CT_FLAGS_MAX - 1) << 1)
> +#define TCA_FLOWER_KEY_CT_FLAGS_MASK \
> +		(TCA_FLOWER_KEY_CT_FLAGS_MAX - 1)

This calculation is rather complicated, sorry I missed this the first
time around. And I don't think it needs to be in the uAPI.

>  enum {
>  	TCA_FLOWER_KEY_ENC_OPTS_UNSPEC,
>  	TCA_FLOWER_KEY_ENC_OPTS_GENEVE, /* Nested
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 84f9325..4aebf4e 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -30,6 +30,9 @@
>  
>  #include <uapi/linux/netfilter/nf_conntrack_common.h>

We can add a define here:

#define TCA_FLOWER_KEY_CT_FLAGS_MASK (TCA_FLOWER_KEY_CT_FLAGS_NEW | \
				       TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED
				       ...

> +#define TCA_FLOWER_KEY_CT_STATE_MASK_TYPE \
> +			NLA_POLICY_MASK(NLA_U16, TCA_FLOWER_KEY_CT_FLAGS_MASK)

nit: using a define for policy is also uncommon AFAIK..

>  struct fl_flow_key {
>  	struct flow_dissector_key_meta meta;
>  	struct flow_dissector_key_control control;
> @@ -687,7 +690,7 @@ static void *fl_get(struct tcf_proto *tp, u32 handle)
>  	[TCA_FLOWER_KEY_ENC_OPTS]	= { .type = NLA_NESTED },
>  	[TCA_FLOWER_KEY_ENC_OPTS_MASK]	= { .type = NLA_NESTED },
>  	[TCA_FLOWER_KEY_CT_STATE]	= { .type = NLA_U16 },
> -	[TCA_FLOWER_KEY_CT_STATE_MASK]	= { .type = NLA_U16 },
> +	[TCA_FLOWER_KEY_CT_STATE_MASK]	= TCA_FLOWER_KEY_CT_STATE_MASK_TYPE,

.. if the line would be long just go to the next one:

	[TCA_FLOWER_KEY_CT_STATE_MASK]	= 
		NLA_POLICY_MASK(NLA_U16, TCA_FLOWER_KEY_CT_FLAGS_MASK)

>  	[TCA_FLOWER_KEY_CT_ZONE]	= { .type = NLA_U16 },
>  	[TCA_FLOWER_KEY_CT_ZONE_MASK]	= { .type = NLA_U16 },
>  	[TCA_FLOWER_KEY_CT_MARK]	= { .type = NLA_U32 },