netdev - Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC

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]

Date: Wed, 26 Jun 2024 11:49:20 +0200
From: Davide Caratti <dcaratti@...hat.com>
To: Asbjørn Sloth Tønnesen <ast@...erby.net>
Cc: Ilya Maximets <i.maximets@....org>, Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.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>,
	David Ahern <dsahern@...nel.org>, Simon Horman <horms@...nel.org>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare
 fl_{set,dump}_key_flags() for ENC_FLAGS

hello Asbjørn,

thanks for your patience!

On Fri, Jun 21, 2024 at 02:45:28PM +0000, Asbjørn Sloth Tønnesen wrote:
> 
> Could you please post your iproute2 code?

sure, will clean it up and share it today in ML.
 
> > from
> > 
> > https://lore.kernel.org/netdev/20240611235355.177667-2-ast@fiberby.net/
> > 
> > Now: functional tests on TCA_FLOWER_KEY_ENC_FLAGS systematically fail. I must
> > admit that I didn't complete 100% of the analysis, but IMO there is at least an
> > endianness problem here. See below:
> > 
> > On Tue, Jun 11, 2024 at 11:53:35PM +0000, Asbjørn Sloth Tønnesen wrote:

[...]
 
> It is always preferred to have a well-defined endianness for binary protocols, even
> if it might only be used locally for now.

given the implementation of fl_set_key_flags() in patch 2,

	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));

when fl_key and fl_mask are TCA_FLOWER_KEY_ENC_FLAGS and TCA_FLOWER_KEY_ENC_FLAGS_MASK,
I assume that we want to turn them to network ordering, like it's already being done for
TCA_FLOWER_KEY_FLAGS and TCA_FLOWER_KEY_FLAGS_MASK.

So, we must htonl() the policy mask in the second hunk in patch 7,something like:

@@ -746,9 +746,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_L2_MISS]		= NLA_POLICY_MAX(NLA_U8, 1),
 	[TCA_FLOWER_KEY_CFM]		= { .type = NLA_NESTED },
 	[TCA_FLOWER_KEY_ENC_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ENC_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 };

And for the same reason, the flower code in patch 3 needs to be changed as follows:

@@ -676,8 +680,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]	= { .type = NLA_U16 },
-	[TCA_FLOWER_KEY_FLAGS]		= { .type = NLA_U32 },
-	[TCA_FLOWER_KEY_FLAGS_MASK]	= { .type = NLA_U32 },
+	[TCA_FLOWER_KEY_FLAGS]		= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
+	[TCA_FLOWER_KEY_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ICMPV4_TYPE]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_CODE]	= { .type = NLA_U8 },

Otherwise it will break the following use case (taken from tc_flower.sh kselftest):

# tc qdisc add dev lo clsact
# tc filter add dev lo ingress protocol ip pref 1 handle 101 flower ip_flags frag action continue
RTNETLINK answers: Invalid argument
We have an error talking to the kernel

because TCA_FLOWER_KEY_FLAGS_POLICY_MASK and TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK
are in host byte order _ so netlink policy mask validation will fail unless we turn
the mask to network byte order.

(And I see we don't have a tdc selftest  for 'ip_flags', this might be a
good chance to add it :-) )

-- 
davide