Re: [PATCH net v2] net: openvswitch: fix TTL decrement action netlink message format

Re: [PATCH net v2] net: openvswitch: fix TTL decrement action netlink message format

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4591 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <160622121495.27296.888010441924340582.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com>
References: <160622121495.27296.888010441924340582.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com>
TO: Eelco Chaudron <echaudro@redhat.com>
TO: netdev(a)vger.kernel.org
CC: davem(a)davemloft.net
CC: dev(a)openvswitch.org
CC: kuba(a)kernel.org
CC: pshelar(a)ovn.org
CC: bindiyakurle(a)gmail.com
CC: i.maximets(a)ovn.org
CC: mcroce(a)linux.microsoft.com

Hi Eelco,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Eelco-Chaudron/net-openvswitch-fix-TTL-decrement-action-netlink-message-format/20201124-203654
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git d549699048b4b5c22dd710455bcdb76966e55aa3
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
config: i386-randconfig-m021-20201124 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
net/openvswitch/actions.c:965 dec_ttl_exception_handler() warn: can 'actions' even be NULL?
net/openvswitch/flow_netlink.c:2521 validate_and_copy_dec_ttl() warn: potential spectre issue 'attrs' [w] (local cap)
net/openvswitch/flow_netlink.c:2525 validate_and_copy_dec_ttl() warn: possible spectre second half.  'actions'

Old smatch warnings:
net/openvswitch/flow_netlink.c:487 __parse_flow_nlattrs() warn: potential spectre issue 'ovs_key_lens' [r] (local cap)
net/openvswitch/flow_netlink.c:496 __parse_flow_nlattrs() warn: potential spectre issue 'a' [w] (local cap)
net/openvswitch/flow_netlink.c:594 vxlan_tun_opt_from_nlattr() warn: potential spectre issue 'ovs_vxlan_ext_key_lens' [w] (local cap)
net/openvswitch/flow_netlink.c:677 ip_tun_from_nlattr() warn: potential spectre issue 'ovs_tunnel_key_lens' [w] (local cap)
net/openvswitch/flow_netlink.c:1181 metadata_from_nlattrs() warn: 'in_port' 4294967295 can't fit into 65535 'match->mask->key.phy.in_port'
net/openvswitch/flow_netlink.c:1402 nsh_key_put_from_nlattr() warn: potential spectre issue 'ovs_nsh_key_attr_lens' [w] (local cap)
net/openvswitch/flow_netlink.c:1745 nlattr_set() warn: potential spectre issue 'tbl' [w]
net/openvswitch/flow_netlink.c:2451 validate_and_copy_sample() warn: potential spectre issue 'attrs' [w] (local cap)
net/openvswitch/flow_netlink.c:2457 validate_and_copy_sample() warn: possible spectre second half.  'probability'
net/openvswitch/flow_netlink.c:2461 validate_and_copy_sample() warn: possible spectre second half.  'actions'
net/openvswitch/flow_netlink.c:2752 validate_set() warn: potential spectre issue 'ovs_key_lens' [w] (local cap)
net/openvswitch/flow_netlink.c:3357 ovs_nla_copy_actions() warn: passing a valid pointer to 'PTR_ERR'

vim +/actions +965 net/openvswitch/actions.c

ccb1352e76cff05 Jesse Gross    2011-10-25  954  
744676e777207f4 Matteo Croce   2020-02-15  955  static int dec_ttl_exception_handler(struct datapath *dp, struct sk_buff *skb,
744676e777207f4 Matteo Croce   2020-02-15  956  				     struct sw_flow_key *key,
744676e777207f4 Matteo Croce   2020-02-15  957  				     const struct nlattr *attr, bool last)
744676e777207f4 Matteo Croce   2020-02-15  958  {
744676e777207f4 Matteo Croce   2020-02-15  959  	/* The first action is always 'OVS_DEC_TTL_ATTR_ARG'. */
744676e777207f4 Matteo Croce   2020-02-15  960  	struct nlattr *dec_ttl_arg = nla_data(attr);
744676e777207f4 Matteo Croce   2020-02-15  961  
744676e777207f4 Matteo Croce   2020-02-15  962  	if (nla_len(dec_ttl_arg)) {
70f3347785f1b60 Eelco Chaudron 2020-11-24  963  		struct nlattr *actions = nla_data(dec_ttl_arg);
744676e777207f4 Matteo Croce   2020-02-15  964  
744676e777207f4 Matteo Croce   2020-02-15 @965  		if (actions)
70f3347785f1b60 Eelco Chaudron 2020-11-24  966  			return clone_execute(dp, skb, key, 0, nla_data(actions),
70f3347785f1b60 Eelco Chaudron 2020-11-24  967  					     nla_len(actions), last, false);
744676e777207f4 Matteo Croce   2020-02-15  968  	}
744676e777207f4 Matteo Croce   2020-02-15  969  	consume_skb(skb);
744676e777207f4 Matteo Croce   2020-02-15  970  	return 0;
744676e777207f4 Matteo Croce   2020-02-15  971  }
744676e777207f4 Matteo Croce   2020-02-15  972  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38055 bytes --]

Re: [PATCH net v2] net: openvswitch: fix TTL decrement action netlink message format

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue, 24 Nov 2020 07:34:44 -0500 you wrote:
> Currently, the openvswitch module is not accepting the correctly formated
> netlink message for the TTL decrement action. For both setting and getting
> the dec_ttl action, the actions should be nested in the
> OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h uapi.
> 
> When the original patch was sent, it was tested with a private OVS userspace
> implementation. This implementation was unfortunately not upstreamed and
> reviewed, hence an erroneous version of this patch was sent out.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: openvswitch: fix TTL decrement action netlink message format
    https://git.kernel.org/netdev/net/c/69929d4c49e1

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net v2] net: openvswitch: fix TTL decrement action netlink message format

[Eelco Chaudron]

Currently, the openvswitch module is not accepting the correctly formated
netlink message for the TTL decrement action. For both setting and getting
the dec_ttl action, the actions should be nested in the
OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h uapi.

When the original patch was sent, it was tested with a private OVS userspace
implementation. This implementation was unfortunately not upstreamed and
reviewed, hence an erroneous version of this patch was sent out.

Leaving the patch as-is would cause problems as the kernel module could
interpret additional attributes as actions and vice-versa, due to the
actions not being encapsulated/nested within the actual attribute, but
being concatinated after it.

Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

---
v2: Update commit message to better explain the issue with the existing patch

 include/uapi/linux/openvswitch.h |    2 +
 net/openvswitch/actions.c        |    7 ++--
 net/openvswitch/flow_netlink.c   |   74 ++++++++++++++++++++++++++++----------
 3 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 8300cc29dec8..8d16744edc31 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -1058,4 +1058,6 @@ enum ovs_dec_ttl_attr {
 	__OVS_DEC_TTL_ATTR_MAX
 };
 
+#define OVS_DEC_TTL_ATTR_MAX (__OVS_DEC_TTL_ATTR_MAX - 1)
+
 #endif /* _LINUX_OPENVSWITCH_H */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index b87bfc82f44f..5829a020b81c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -958,14 +958,13 @@ static int dec_ttl_exception_handler(struct datapath *dp, struct sk_buff *skb,
 {
 	/* The first action is always 'OVS_DEC_TTL_ATTR_ARG'. */
 	struct nlattr *dec_ttl_arg = nla_data(attr);
-	int rem = nla_len(attr);
 
 	if (nla_len(dec_ttl_arg)) {
-		struct nlattr *actions = nla_next(dec_ttl_arg, &rem);
+		struct nlattr *actions = nla_data(dec_ttl_arg);
 
 		if (actions)
-			return clone_execute(dp, skb, key, 0, actions, rem,
-					     last, false);
+			return clone_execute(dp, skb, key, 0, nla_data(actions),
+					     nla_len(actions), last, false);
 	}
 	consume_skb(skb);
 	return 0;
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 9d3e50c4d29f..ec0689ddc635 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2503,28 +2503,42 @@ static int validate_and_copy_dec_ttl(struct net *net,
 				     __be16 eth_type, __be16 vlan_tci,
 				     u32 mpls_label_count, bool log)
 {
-	int start, err;
-	u32 nested = true;
+	const struct nlattr *attrs[OVS_DEC_TTL_ATTR_MAX + 1];
+	int start, action_start, err, rem;
+	const struct nlattr *a, *actions;
+
+	memset(attrs, 0, sizeof(attrs));
+	nla_for_each_nested(a, attr, rem) {
+		int type = nla_type(a);
 
-	if (!nla_len(attr))
-		return ovs_nla_add_action(sfa, OVS_ACTION_ATTR_DEC_TTL,
-					  NULL, 0, log);
+		/* Ignore unknown attributes to be future proof. */
+		if (type > OVS_DEC_TTL_ATTR_MAX)
+			continue;
+
+		if (!type || attrs[type])
+			return -EINVAL;
+
+		attrs[type] = a;
+	}
+
+	actions = attrs[OVS_DEC_TTL_ATTR_ACTION];
+	if (rem || !actions || (nla_len(actions) && nla_len(actions) < NLA_HDRLEN))
+		return -EINVAL;
 
 	start = add_nested_action_start(sfa, OVS_ACTION_ATTR_DEC_TTL, log);
 	if (start < 0)
 		return start;
 
-	err = ovs_nla_add_action(sfa, OVS_DEC_TTL_ATTR_ACTION, &nested,
-				 sizeof(nested), log);
-
-	if (err)
-		return err;
+	action_start = add_nested_action_start(sfa, OVS_DEC_TTL_ATTR_ACTION, log);
+	if (action_start < 0)
+		return start;
 
-	err = __ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
+	err = __ovs_nla_copy_actions(net, actions, key, sfa, eth_type,
 				     vlan_tci, mpls_label_count, log);
 	if (err)
 		return err;
 
+	add_nested_action_end(*sfa, action_start);
 	add_nested_action_end(*sfa, start);
 	return 0;
 }
@@ -3487,20 +3501,42 @@ static int check_pkt_len_action_to_attr(const struct nlattr *attr,
 static int dec_ttl_action_to_attr(const struct nlattr *attr,
 				  struct sk_buff *skb)
 {
-	int err = 0, rem = nla_len(attr);
-	struct nlattr *start;
+	struct nlattr *start, *action_start;
+	const struct nlattr *a;
+	int err = 0, rem;
 
 	start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_DEC_TTL);
-
 	if (!start)
 		return -EMSGSIZE;
 
-	err = ovs_nla_put_actions(nla_data(attr), rem, skb);
-	if (err)
-		nla_nest_cancel(skb, start);
-	else
-		nla_nest_end(skb, start);
+	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
+		switch (nla_type(a)) {
+		case OVS_DEC_TTL_ATTR_ACTION:
+
+			action_start = nla_nest_start_noflag(skb, OVS_DEC_TTL_ATTR_ACTION);
+			if (!action_start) {
+				err = -EMSGSIZE;
+				goto out;
+			}
+
+			err = ovs_nla_put_actions(nla_data(a), nla_len(a), skb);
+			if (err)
+				goto out;
+
+			nla_nest_end(skb, action_start);
+			break;
 
+		default:
+			/* Ignore all other option to be future compatible */
+			break;
+		}
+	}
+
+	nla_nest_end(skb, start);
+	return 0;
+
+out:
+	nla_nest_cancel(skb, start);
 	return err;
 }