[PATCH net 0/1 v1] rtnetlink: require unique netns identifier

Hey,

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing. For legacy reasons the kernel will
pick the IFLA_NET_NS_PID property first and then look for the
IFLA_NET_NS_FD property but there is no reason to extend this type of
behavior to network namespace ids. The regression potential is quite
minimal since the rtnetlink requests in question either won't allow
IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.

We obviously cannot prevent users from passing both IFLA_NET_NS_PID and
IFLA_NET_NS_FD since we have supported this somehow for a long time. So
the check I'm proposing is to only fail when both IFLA_IF_NETNSID, and
IFLA_NET_NS_PID or IFLA_NET_NS_FD are passed and they refer to different
network namespaces.

Thanks!
Christian

ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace

Christian Brauner (1):
  rtnetlink: require unique netns identifier

 net/core/rtnetlink.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

-- 
2.14.1

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing. For legacy reasons the kernel will
pick the IFLA_NET_NS_PID property first and then look for the
IFLA_NET_NS_FD property but there is no reason to extend this type of
behavior to network namespace ids. The regression potential is quite
minimal since the rtnetlink requests in question either won't allow
IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.

Signed-off-by: Christian Brauner <redacted>
---
ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace
---
 net/core/rtnetlink.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 56af8e41abfc..df2363f28574 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -1951,6 +1951,57 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
 	return net;
 }
 
+/* Verify that rtnetlink requests supporting network namespace ids
+ * do not pass additional properties referring to different network
+ * namespaces.
+ */
+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;
+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net)
+		return -1;
+
+	if (tb[IFLA_NET_NS_PID]) {
+		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
+		if (net != unique_net)
+			goto on_error;
+	}
+
+	if (tb[IFLA_NET_NS_FD]) {
+		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
+		if (net != unique_net)
+			goto on_error;
+	}
+
+	ret = 0;
+
+on_error:
+	put_net(unique_net);
+
+	if (net && !IS_ERR(net))
+		put_net(net);
+
+	if (ret != 0)
+		NL_SET_ERR_MSG(extack, "multiple network namespaces specified");
+
+	return ret;
+}
+
 static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 {
 	if (dev) {

@@ -2553,6 +2604,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
+	err = rtnl_ensure_unique_netns_attr(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		goto errout;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else

@@ -2649,6 +2704,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns_attr(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 

@@ -2802,6 +2861,10 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns_attr(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else

@@ -3045,6 +3108,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns_attr(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IF_NETNSID]) {
 		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
 		tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);

-- 
2.14.1

On Sat,  3 Feb 2018 14:29:04 +0100
Christian Brauner [off-list ref] wrote:

+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;

Isn't this an error?

+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net)
+		return -1;

Other paths are returning errno, so why -1 here?

On 2/3/18 12:17 PM, Stephen Hemminger wrote:

On Sat,  3 Feb 2018 14:29:04 +0100
Christian Brauner [off-list ref] wrote:

quoted

+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;

Isn't this an error?

quoted

+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net)
+		return -1;

Other paths are returning errno, so why -1 here?

extack needs to be filled in too.

On Sat, Feb 03, 2018 at 11:17:01AM -0800, Stephen Hemminger wrote:

On Sat,  3 Feb 2018 14:29:04 +0100
Christian Brauner [off-list ref] wrote:

quoted

+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;

Isn't this an error?

My reasoning was that having no explicit network namespace identifying
attributes the caller operates on the current network namespace which is
uniquely identified.

quoted

+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net)
+		return -1;

Other paths are returning errno, so why -1 here?

Yes, this should be -EINVAL as well.

Thanks!
Christian

On Sat, Feb 03, 2018 at 07:09:55PM -0700, David Ahern wrote:

On 2/3/18 12:17 PM, Stephen Hemminger wrote:

quoted

On Sat,  3 Feb 2018 14:29:04 +0100
Christian Brauner [off-list ref] wrote:

quoted

+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;

Isn't this an error?

quoted

+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net)
+		return -1;

Other paths are returning errno, so why -1 here?

extack needs to be filled in too.

Yeah, it should report that an invalid network namespace identifier has
been specified.

Thanks!
Christian

On 2/4/18 5:11 AM, Christian Brauner wrote:

On Sat, Feb 03, 2018 at 11:17:01AM -0800, Stephen Hemminger wrote:

quoted

On Sat,  3 Feb 2018 14:29:04 +0100
Christian Brauner [off-list ref] wrote:

quoted

+static int rtnl_ensure_unique_netns_attr(const struct sock *sk,
+					 struct nlattr *tb[],
+					 struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;

Isn't this an error?

My reasoning was that having no explicit network namespace identifying
attributes the caller operates on the current network namespace which is
uniquely identified.

agreed. those are not required attributes.