[Patch net] rtnetlink: catch error pointer for rtnl_link_get_net()

[Patch net] rtnetlink: catch error pointer for rtnl_link_get_net()

[Cong Wang]

From: Cong Wang <cong.wang@bytedance.com>

Currently all callers of rtnl_link_get_net() assume that it always
returns a valid netns pointer, when rtnl_link_get_net_ifla() fails,
it uses 'src_net' as a fallback.

This is not true, because rtnl_link_get_net_ifla() can return an
error pointer too, we need to handle this error case and propagate
the error code to its callers.

Add a comment to better document its return value.

Reported-by: syzbot+21ba4d5adff0b6a7cfc6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=21ba4d5adff0b6a7cfc6
Fixes: 0eb87b02a705 ("veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.")
Fixes: 6b84e558e95d ("vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.")
Fixes: fefd5d082172 ("netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.")
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 drivers/net/can/vxcan.c |  3 +++
 drivers/net/netkit.c    |  3 +++
 drivers/net/veth.c      |  3 +++
 net/core/rtnetlink.c    | 12 ++++++++++++
 4 files changed, 21 insertions(+)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index da7c72105fb6..6d03a5314034 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -204,6 +204,9 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	}
 
 	peer_net = rtnl_link_get_net(net, tbp);
+	if (IS_ERR(peer_net))
+		return PTR_ERR(peer_net);
+
 	peer = rtnl_create_link(peer_net, ifname, name_assign_type,
 				&vxcan_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index bb07725d1c72..44fe99a82ac3 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -386,6 +386,9 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 		return -EOPNOTSUPP;
 
 	net = rtnl_link_get_net(src_net, tbp);
+	if (IS_ERR(net))
+		return PTR_ERR(net);
+
 	peer = rtnl_create_link(net, ifname, ifname_assign_type,
 				&netkit_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0d6d0d749d44..3a42a982c638 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1801,6 +1801,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	}
 
 	net = rtnl_link_get_net(src_net, tbp);
+	if (IS_ERR(net))
+		return PTR_ERR(net);
+
 	peer = rtnl_create_link(net, ifname, name_assign_type,
 				&veth_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dd142f444659..6a4363276117 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2527,6 +2527,18 @@ static struct net *rtnl_link_get_net_ifla(struct nlattr *tb[])
 	return net;
 }
 
+/**
+ * rtnl_link_get_net - Get the network namespace from the netlink attributes
+ * or just @src_net.
+ *
+ * @src_net: the source network namespace
+ * @tb: the netlink attributes
+ *
+ * Returns:
+ *   The network namespace specified in the netlink attributes,
+ *   in case of error, an error pointer is returned.
+ *   Or, @src_net if no netns attributes were passed.
+ */
 struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
 {
 	struct net *net = rtnl_link_get_net_ifla(tb);
-- 
2.34.1

Re: [Patch net] rtnetlink: catch error pointer for rtnl_link_get_net()

[Kuniyuki Iwashima]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 28 Nov 2024 22:31:12 -0800
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Currently all callers of rtnl_link_get_net() assume that it always
> returns a valid netns pointer,

because I assume it's always tested in rtnl_add_peer_net()...


> when rtnl_link_get_net_ifla() fails,
> it uses 'src_net' as a fallback.
> 
> This is not true,

because rtnl_link_get_net_ifla() isn't called if (!data ||
!data[ops->peer_type]),

so the correct fix is:

---8<---
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dd142f444659..c1f4aaa40823 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3815,6 +3815,10 @@ static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
 	struct net *net;
 	int err;
 
+	net = rtnl_link_get_net_ifla(tb);
+	if (IS_ERR(net))
+		return PTR_ERR(net);
+
 	if (!data || !data[ops->peer_type])
 		return 0;
 
@@ -3828,9 +3832,6 @@ static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
 			return err;
 	}
 
-	net = rtnl_link_get_net_ifla(tb);
-	if (IS_ERR(net))
-		return PTR_ERR(net);
 	if (net)
 		rtnl_nets_add(rtnl_nets, net);
 
---8<---


> because rtnl_link_get_net_ifla() can return an
> error pointer too, we need to handle this error case and propagate
> the error code to its callers.
> 
> Add a comment to better document its return value.

Re: [Patch net] rtnetlink: catch error pointer for rtnl_link_get_net()

[Cong Wang]

On Fri, Nov 29, 2024 at 04:36:09PM +0900, Kuniyuki Iwashima wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Thu, 28 Nov 2024 22:31:12 -0800
> > From: Cong Wang <cong.wang@bytedance.com>
> > 
> > Currently all callers of rtnl_link_get_net() assume that it always
> > returns a valid netns pointer,
> 
> because I assume it's always tested in rtnl_add_peer_net()...

Why is this assumption?

I seriouly doubt you can assume that, because for example in veth_newlink()
'peer_tb' is parsed within the same function and it is right before
rtnl_link_get_net().

> 
> 
> > when rtnl_link_get_net_ifla() fails,
> > it uses 'src_net' as a fallback.
> > 
> > This is not true,
> 
> because rtnl_link_get_net_ifla() isn't called if (!data ||
> !data[ops->peer_type]),
> 
> so the correct fix is:

It is not.

> 
> ---8<---
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index dd142f444659..c1f4aaa40823 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3815,6 +3815,10 @@ static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
>  	struct net *net;
>  	int err;
>  
> +	net = rtnl_link_get_net_ifla(tb);
> +	if (IS_ERR(net))
> +		return PTR_ERR(net);
> +
>  	if (!data || !data[ops->peer_type])

'tb' is not yet parsed at this point and you still want to pass it to
rtnl_link_get_net_ifla()? In fact, it is even uninitialized.

Thanks.

[Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[Cong Wang]

From: Cong Wang <cong.wang@bytedance.com>

Currently rtnl_link_get_net_ifla() gets called twice when we create
peer devices, once in rtnl_add_peer_net() and once in each ->newlink()
implementation.

This looks safer, however, it leads to a classic Time-of-Check to
Time-of-Use (TOCTOU) bug since IFLA_NET_NS_PID is very dynamic. And
because of the lack of checking error pointer of the second call, it
also leads to a kernel crash as reported by syzbot.

Fix this by getting rid of the second call, which already becomes
redudant after Kuniyuki's work. We have to propagate the result of the
first rtnl_link_get_net_ifla() down to each ->newlink().

Reported-by: syzbot+21ba4d5adff0b6a7cfc6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=21ba4d5adff0b6a7cfc6
Fixes: 0eb87b02a705 ("veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.")
Fixes: 6b84e558e95d ("vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.")
Fixes: fefd5d082172 ("netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.")
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 drivers/net/can/vxcan.c | 10 ++--------
 drivers/net/netkit.c    | 11 +++--------
 drivers/net/veth.c      | 12 +++--------
 net/core/rtnetlink.c    | 44 +++++++++++++++++++++--------------------
 4 files changed, 31 insertions(+), 46 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index da7c72105fb6..ca8811941085 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -172,13 +172,12 @@ static void vxcan_setup(struct net_device *dev)
 /* forward declaration for rtnl_create_link() */
 static struct rtnl_link_ops vxcan_link_ops;
 
-static int vxcan_newlink(struct net *net, struct net_device *dev,
+static int vxcan_newlink(struct net *peer_net, struct net_device *dev,
 			 struct nlattr *tb[], struct nlattr *data[],
 			 struct netlink_ext_ack *extack)
 {
 	struct vxcan_priv *priv;
 	struct net_device *peer;
-	struct net *peer_net;
 
 	struct nlattr *peer_tb[IFLA_MAX + 1], **tbp = tb;
 	char ifname[IFNAMSIZ];
@@ -203,20 +202,15 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 		name_assign_type = NET_NAME_ENUM;
 	}
 
-	peer_net = rtnl_link_get_net(net, tbp);
 	peer = rtnl_create_link(peer_net, ifname, name_assign_type,
 				&vxcan_link_ops, tbp, extack);
-	if (IS_ERR(peer)) {
-		put_net(peer_net);
+	if (IS_ERR(peer))
 		return PTR_ERR(peer);
-	}
 
 	if (ifmp && dev->ifindex)
 		peer->ifindex = ifmp->ifi_index;
 
 	err = register_netdevice(peer);
-	put_net(peer_net);
-	peer_net = NULL;
 	if (err < 0) {
 		free_netdev(peer);
 		return err;
diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index bb07725d1c72..c1d881dc6409 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -327,7 +327,7 @@ static int netkit_validate(struct nlattr *tb[], struct nlattr *data[],
 
 static struct rtnl_link_ops netkit_link_ops;
 
-static int netkit_new_link(struct net *src_net, struct net_device *dev,
+static int netkit_new_link(struct net *peer_net, struct net_device *dev,
 			   struct nlattr *tb[], struct nlattr *data[],
 			   struct netlink_ext_ack *extack)
 {
@@ -342,7 +342,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	struct net_device *peer;
 	char ifname[IFNAMSIZ];
 	struct netkit *nk;
-	struct net *net;
 	int err;
 
 	if (data) {
@@ -385,13 +384,10 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	    (tb[IFLA_ADDRESS] || tbp[IFLA_ADDRESS]))
 		return -EOPNOTSUPP;
 
-	net = rtnl_link_get_net(src_net, tbp);
-	peer = rtnl_create_link(net, ifname, ifname_assign_type,
+	peer = rtnl_create_link(peer_net, ifname, ifname_assign_type,
 				&netkit_link_ops, tbp, extack);
-	if (IS_ERR(peer)) {
-		put_net(net);
+	if (IS_ERR(peer))
 		return PTR_ERR(peer);
-	}
 
 	netif_inherit_tso_max(peer, dev);
 
@@ -408,7 +404,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 	bpf_mprog_bundle_init(&nk->bundle);
 
 	err = register_netdevice(peer);
-	put_net(net);
 	if (err < 0)
 		goto err_register_peer;
 	netif_carrier_off(peer);
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0d6d0d749d44..07ebb800edf1 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1765,7 +1765,7 @@ static int veth_init_queues(struct net_device *dev, struct nlattr *tb[])
 	return 0;
 }
 
-static int veth_newlink(struct net *src_net, struct net_device *dev,
+static int veth_newlink(struct net *peer_net, struct net_device *dev,
 			struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
@@ -1776,7 +1776,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	struct nlattr *peer_tb[IFLA_MAX + 1], **tbp;
 	unsigned char name_assign_type;
 	struct ifinfomsg *ifmp;
-	struct net *net;
 
 	/*
 	 * create and register peer first
@@ -1800,13 +1799,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 		name_assign_type = NET_NAME_ENUM;
 	}
 
-	net = rtnl_link_get_net(src_net, tbp);
-	peer = rtnl_create_link(net, ifname, name_assign_type,
+	peer = rtnl_create_link(peer_net, ifname, name_assign_type,
 				&veth_link_ops, tbp, extack);
-	if (IS_ERR(peer)) {
-		put_net(net);
+	if (IS_ERR(peer))
 		return PTR_ERR(peer);
-	}
 
 	if (!ifmp || !tbp[IFLA_ADDRESS])
 		eth_hw_addr_random(peer);
@@ -1817,8 +1813,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	netif_inherit_tso_max(peer, dev);
 
 	err = register_netdevice(peer);
-	put_net(net);
-	net = NULL;
 	if (err < 0)
 		goto err_register_peer;
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 58df76fe408a..ab5f201bf0ab 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3746,6 +3746,7 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
 static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 			       const struct rtnl_link_ops *ops,
 			       struct net *tgt_net, struct net *link_net,
+			       struct net *peer_net,
 			       const struct nlmsghdr *nlh,
 			       struct nlattr **tb, struct nlattr **data,
 			       struct netlink_ext_ack *extack)
@@ -3776,8 +3777,13 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 
 	dev->ifindex = ifm->ifi_index;
 
+	if (link_net)
+		net = link_net;
+	if (peer_net)
+		net = peer_net;
+
 	if (ops->newlink)
-		err = ops->newlink(link_net ? : net, dev, tb, data, extack);
+		err = ops->newlink(net, dev, tb, data, extack);
 	else
 		err = register_netdevice(dev);
 	if (err < 0) {
@@ -3812,40 +3818,33 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 	goto out;
 }
 
-static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
-			     const struct rtnl_link_ops *ops,
-			     struct nlattr *data[],
-			     struct netlink_ext_ack *extack)
+static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
+				     struct nlattr *data[],
+				     struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[IFLA_MAX + 1];
-	struct net *net;
 	int err;
 
 	if (!data || !data[ops->peer_type])
-		return 0;
+		return NULL;
 
 	err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
 
 	if (ops->validate) {
 		err = ops->validate(tb, NULL, extack);
 		if (err < 0)
-			return err;
+			return ERR_PTR(err);
 	}
 
-	net = rtnl_link_get_net_ifla(tb);
-	if (IS_ERR(net))
-		return PTR_ERR(net);
-	if (net)
-		rtnl_nets_add(rtnl_nets, net);
-
-	return 0;
+	return rtnl_link_get_net_ifla(tb);
 }
 
 static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  const struct rtnl_link_ops *ops,
 			  struct net *tgt_net, struct net *link_net,
+			  struct net *peer_net,
 			  struct rtnl_newlink_tbs *tbs,
 			  struct nlattr **data,
 			  struct netlink_ext_ack *extack)
@@ -3894,14 +3893,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EOPNOTSUPP;
 	}
 
-	return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, nlh, tb, data, extack);
+	return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, peer_net, nlh,
+				   tb, data, extack);
 }
 
 static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			struct netlink_ext_ack *extack)
 {
+	struct net *tgt_net, *link_net = NULL, *peer_net = NULL;
 	struct nlattr **tb, **linkinfo, **data = NULL;
-	struct net *tgt_net, *link_net = NULL;
 	struct rtnl_link_ops *ops = NULL;
 	struct rtnl_newlink_tbs *tbs;
 	struct rtnl_nets rtnl_nets;
@@ -3971,9 +3971,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 
 		if (ops->peer_type) {
-			ret = rtnl_add_peer_net(&rtnl_nets, ops, data, extack);
-			if (ret < 0)
+			peer_net = rtnl_get_peer_net(ops, data, extack);
+			if (IS_ERR(peer_net))
 				goto put_ops;
+			if (peer_net)
+				rtnl_nets_add(&rtnl_nets, peer_net);
 		}
 	}
 
@@ -4004,7 +4006,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	rtnl_nets_lock(&rtnl_nets);
-	ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
+	ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, peer_net, tbs, data, extack);
 	rtnl_nets_unlock(&rtnl_nets);
 
 put_net:
-- 
2.34.1

Re: [Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[Jakub Kicinski]

On Fri, 29 Nov 2024 13:25:19 -0800 Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Currently rtnl_link_get_net_ifla() gets called twice when we create
> peer devices, once in rtnl_add_peer_net() and once in each ->newlink()
> implementation.
> 
> This looks safer, however, it leads to a classic Time-of-Check to
> Time-of-Use (TOCTOU) bug since IFLA_NET_NS_PID is very dynamic. And
> because of the lack of checking error pointer of the second call, it
> also leads to a kernel crash as reported by syzbot.
> 
> Fix this by getting rid of the second call, which already becomes
> redudant after Kuniyuki's work. We have to propagate the result of the
> first rtnl_link_get_net_ifla() down to each ->newlink().

Two process notes:
 - please wait 24h between postings;
 - please don't post new versions in reply to an old one.

Re: [Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[Kuniyuki Iwashima]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 29 Nov 2024 13:25:19 -0800
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Currently rtnl_link_get_net_ifla() gets called twice when we create
> peer devices, once in rtnl_add_peer_net() and once in each ->newlink()
> implementation.
> 
> This looks safer, however, it leads to a classic Time-of-Check to
> Time-of-Use (TOCTOU) bug since IFLA_NET_NS_PID is very dynamic. And
> because of the lack of checking error pointer of the second call, it
> also leads to a kernel crash as reported by syzbot.
> 
> Fix this by getting rid of the second call, which already becomes
> redudant after Kuniyuki's work. We have to propagate the result of the
> first rtnl_link_get_net_ifla() down to each ->newlink().
> 
> Reported-by: syzbot+21ba4d5adff0b6a7cfc6@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=21ba4d5adff0b6a7cfc6
> Fixes: 0eb87b02a705 ("veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.")
> Fixes: 6b84e558e95d ("vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.")
> Fixes: fefd5d082172 ("netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.")
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks for the fix and nice cleanup :)

Re: [Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 29 Nov 2024 13:25:19 -0800 you wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Currently rtnl_link_get_net_ifla() gets called twice when we create
> peer devices, once in rtnl_add_peer_net() and once in each ->newlink()
> implementation.
> 
> This looks safer, however, it leads to a classic Time-of-Check to
> Time-of-Use (TOCTOU) bug since IFLA_NET_NS_PID is very dynamic. And
> because of the lack of checking error pointer of the second call, it
> also leads to a kernel crash as reported by syzbot.
> 
> [...]

Here is the summary with links:
  - [net,v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()
    https://git.kernel.org/netdev/net/c/48327566769a

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

Re: [Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[Xiao Liang]

On Mon, Dec 16, 2024 at 4:53 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> From: Cong Wang <cong.wang@bytedance.com>
>
> Currently rtnl_link_get_net_ifla() gets called twice when we create
> peer devices, once in rtnl_add_peer_net() and once in each ->newlink()
> implementation.
>
> This looks safer, however, it leads to a classic Time-of-Check to
> Time-of-Use (TOCTOU) bug since IFLA_NET_NS_PID is very dynamic. And
> because of the lack of checking error pointer of the second call, it
> also leads to a kernel crash as reported by syzbot.
>
> Fix this by getting rid of the second call, which already becomes
> redudant after Kuniyuki's work. We have to propagate the result of the
> first rtnl_link_get_net_ifla() down to each ->newlink().
>
> Reported-by: syzbot+21ba4d5adff0b6a7cfc6@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=21ba4d5adff0b6a7cfc6
> Fixes: 0eb87b02a705 ("veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.")
> Fixes: 6b84e558e95d ("vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.")
> Fixes: fefd5d082172 ("netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.")
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
>  drivers/net/can/vxcan.c | 10 ++--------
>  drivers/net/netkit.c    | 11 +++--------
>  drivers/net/veth.c      | 12 +++--------
>  net/core/rtnetlink.c    | 44 +++++++++++++++++++++--------------------
>  4 files changed, 31 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index da7c72105fb6..ca8811941085 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -172,13 +172,12 @@ static void vxcan_setup(struct net_device *dev)
>  /* forward declaration for rtnl_create_link() */
>  static struct rtnl_link_ops vxcan_link_ops;
>
> -static int vxcan_newlink(struct net *net, struct net_device *dev,
> +static int vxcan_newlink(struct net *peer_net, struct net_device *dev,
>                          struct nlattr *tb[], struct nlattr *data[],
>                          struct netlink_ext_ack *extack)
>  {
>         struct vxcan_priv *priv;
>         struct net_device *peer;
> -       struct net *peer_net;
>
>         struct nlattr *peer_tb[IFLA_MAX + 1], **tbp = tb;
>         char ifname[IFNAMSIZ];
> @@ -203,20 +202,15 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>                 name_assign_type = NET_NAME_ENUM;
>         }
>
> -       peer_net = rtnl_link_get_net(net, tbp);
>         peer = rtnl_create_link(peer_net, ifname, name_assign_type,
>                                 &vxcan_link_ops, tbp, extack);
> -       if (IS_ERR(peer)) {
> -               put_net(peer_net);
> +       if (IS_ERR(peer))
>                 return PTR_ERR(peer);
> -       }
>
>         if (ifmp && dev->ifindex)
>                 peer->ifindex = ifmp->ifi_index;
>
>         err = register_netdevice(peer);
> -       put_net(peer_net);
> -       peer_net = NULL;
>         if (err < 0) {
>                 free_netdev(peer);
>                 return err;
> diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
> index bb07725d1c72..c1d881dc6409 100644
> --- a/drivers/net/netkit.c
> +++ b/drivers/net/netkit.c
> @@ -327,7 +327,7 @@ static int netkit_validate(struct nlattr *tb[], struct nlattr *data[],
>
>  static struct rtnl_link_ops netkit_link_ops;
>
> -static int netkit_new_link(struct net *src_net, struct net_device *dev,
> +static int netkit_new_link(struct net *peer_net, struct net_device *dev,
>                            struct nlattr *tb[], struct nlattr *data[],
>                            struct netlink_ext_ack *extack)
>  {
> @@ -342,7 +342,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
>         struct net_device *peer;
>         char ifname[IFNAMSIZ];
>         struct netkit *nk;
> -       struct net *net;
>         int err;
>
>         if (data) {
> @@ -385,13 +384,10 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
>             (tb[IFLA_ADDRESS] || tbp[IFLA_ADDRESS]))
>                 return -EOPNOTSUPP;
>
> -       net = rtnl_link_get_net(src_net, tbp);
> -       peer = rtnl_create_link(net, ifname, ifname_assign_type,
> +       peer = rtnl_create_link(peer_net, ifname, ifname_assign_type,
>                                 &netkit_link_ops, tbp, extack);
> -       if (IS_ERR(peer)) {
> -               put_net(net);
> +       if (IS_ERR(peer))
>                 return PTR_ERR(peer);
> -       }
>
>         netif_inherit_tso_max(peer, dev);
>
> @@ -408,7 +404,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
>         bpf_mprog_bundle_init(&nk->bundle);
>
>         err = register_netdevice(peer);
> -       put_net(net);
>         if (err < 0)
>                 goto err_register_peer;
>         netif_carrier_off(peer);
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 0d6d0d749d44..07ebb800edf1 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1765,7 +1765,7 @@ static int veth_init_queues(struct net_device *dev, struct nlattr *tb[])
>         return 0;
>  }
>
> -static int veth_newlink(struct net *src_net, struct net_device *dev,
> +static int veth_newlink(struct net *peer_net, struct net_device *dev,
>                         struct nlattr *tb[], struct nlattr *data[],
>                         struct netlink_ext_ack *extack)
>  {
> @@ -1776,7 +1776,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>         struct nlattr *peer_tb[IFLA_MAX + 1], **tbp;
>         unsigned char name_assign_type;
>         struct ifinfomsg *ifmp;
> -       struct net *net;
>
>         /*
>          * create and register peer first
> @@ -1800,13 +1799,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>                 name_assign_type = NET_NAME_ENUM;
>         }
>
> -       net = rtnl_link_get_net(src_net, tbp);
> -       peer = rtnl_create_link(net, ifname, name_assign_type,
> +       peer = rtnl_create_link(peer_net, ifname, name_assign_type,
>                                 &veth_link_ops, tbp, extack);
> -       if (IS_ERR(peer)) {
> -               put_net(net);
> +       if (IS_ERR(peer))
>                 return PTR_ERR(peer);
> -       }
>
>         if (!ifmp || !tbp[IFLA_ADDRESS])
>                 eth_hw_addr_random(peer);
> @@ -1817,8 +1813,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>         netif_inherit_tso_max(peer, dev);
>
>         err = register_netdevice(peer);
> -       put_net(net);
> -       net = NULL;
>         if (err < 0)
>                 goto err_register_peer;
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 58df76fe408a..ab5f201bf0ab 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3746,6 +3746,7 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
>  static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>                                const struct rtnl_link_ops *ops,
>                                struct net *tgt_net, struct net *link_net,
> +                              struct net *peer_net,
>                                const struct nlmsghdr *nlh,
>                                struct nlattr **tb, struct nlattr **data,
>                                struct netlink_ext_ack *extack)
> @@ -3776,8 +3777,13 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>
>         dev->ifindex = ifm->ifi_index;
>
> +       if (link_net)
> +               net = link_net;
> +       if (peer_net)
> +               net = peer_net;
> +
>         if (ops->newlink)
> -               err = ops->newlink(link_net ? : net, dev, tb, data, extack);
> +               err = ops->newlink(net, dev, tb, data, extack);
>         else
>                 err = register_netdevice(dev);
>         if (err < 0) {
> @@ -3812,40 +3818,33 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>         goto out;
>  }
>
> -static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
> -                            const struct rtnl_link_ops *ops,
> -                            struct nlattr *data[],
> -                            struct netlink_ext_ack *extack)
> +static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
> +                                    struct nlattr *data[],
> +                                    struct netlink_ext_ack *extack)
>  {
>         struct nlattr *tb[IFLA_MAX + 1];
> -       struct net *net;
>         int err;
>
>         if (!data || !data[ops->peer_type])
> -               return 0;
> +               return NULL;

I was adding some tests about the link netns stuff, and found
a behavior change. Prior to this patch, veth, vxcan and netkit
were trying the outer tb if peer info was not set. But returning
NULL here skips this part of logic. Say if we have:

    ip link add netns ns1 foo type veth

The peer link is changed from ns1 to current netns.

Thanks.

>
>         err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack);
>         if (err < 0)
> -               return err;
> +               return ERR_PTR(err);
>
>         if (ops->validate) {
>                 err = ops->validate(tb, NULL, extack);
>                 if (err < 0)
> -                       return err;
> +                       return ERR_PTR(err);
>         }
>
> -       net = rtnl_link_get_net_ifla(tb);
> -       if (IS_ERR(net))
> -               return PTR_ERR(net);
> -       if (net)
> -               rtnl_nets_add(rtnl_nets, net);
> -
> -       return 0;
> +       return rtnl_link_get_net_ifla(tb);
>  }
>
>  static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>                           const struct rtnl_link_ops *ops,
>                           struct net *tgt_net, struct net *link_net,
> +                         struct net *peer_net,
>                           struct rtnl_newlink_tbs *tbs,
>                           struct nlattr **data,
>                           struct netlink_ext_ack *extack)
> @@ -3894,14 +3893,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>                 return -EOPNOTSUPP;
>         }
>
> -       return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, nlh, tb, data, extack);
> +       return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, peer_net, nlh,
> +                                  tb, data, extack);
>  }
>
>  static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>                         struct netlink_ext_ack *extack)
>  {
> +       struct net *tgt_net, *link_net = NULL, *peer_net = NULL;
>         struct nlattr **tb, **linkinfo, **data = NULL;
> -       struct net *tgt_net, *link_net = NULL;
>         struct rtnl_link_ops *ops = NULL;
>         struct rtnl_newlink_tbs *tbs;
>         struct rtnl_nets rtnl_nets;
> @@ -3971,9 +3971,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>                 }
>
>                 if (ops->peer_type) {
> -                       ret = rtnl_add_peer_net(&rtnl_nets, ops, data, extack);
> -                       if (ret < 0)
> +                       peer_net = rtnl_get_peer_net(ops, data, extack);
> +                       if (IS_ERR(peer_net))
>                                 goto put_ops;
> +                       if (peer_net)
> +                               rtnl_nets_add(&rtnl_nets, peer_net);
>                 }
>         }
>
> @@ -4004,7 +4006,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>         }
>
>         rtnl_nets_lock(&rtnl_nets);
> -       ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
> +       ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, peer_net, tbs, data, extack);
>         rtnl_nets_unlock(&rtnl_nets);
>
>  put_net:
> --
> 2.34.1
>
>

Re: [Patch net v2] rtnetlink: fix double call of rtnl_link_get_net_ifla()

[Kuniyuki Iwashima]

From: Xiao Liang <shaw.leon@gmail.com>
Date: Mon, 16 Dec 2024 18:24:39 +0800
> > @@ -3812,40 +3818,33 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> >         goto out;
> >  }
> >
> > -static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
> > -                            const struct rtnl_link_ops *ops,
> > -                            struct nlattr *data[],
> > -                            struct netlink_ext_ack *extack)
> > +static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
> > +                                    struct nlattr *data[],
> > +                                    struct netlink_ext_ack *extack)
> >  {
> >         struct nlattr *tb[IFLA_MAX + 1];
> > -       struct net *net;
> >         int err;
> >
> >         if (!data || !data[ops->peer_type])
> > -               return 0;
> > +               return NULL;
> 
> I was adding some tests about the link netns stuff, and found
> a behavior change. Prior to this patch, veth, vxcan and netkit
> were trying the outer tb if peer info was not set. But returning
> NULL here skips this part of logic. Say if we have:
> 
>     ip link add netns ns1 foo type veth
> 
> The peer link is changed from ns1 to current netns.

Good catch, we need the following diff.

---8<---
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ebcfc2debf1a..d9f959c619d9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3819,6 +3819,7 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 }
 
 static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
+				     struct nlattr *tbp[],
 				     struct nlattr *data[],
 				     struct netlink_ext_ack *extack)
 {
@@ -3826,7 +3827,7 @@ static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
 	int err;
 
 	if (!data || !data[ops->peer_type])
-		return NULL;
+		return rtnl_link_get_net_ifla(tbp);
 
 	err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack);
 	if (err < 0)
@@ -3971,7 +3972,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 
 		if (ops->peer_type) {
-			peer_net = rtnl_get_peer_net(ops, data, extack);
+			peer_net = rtnl_get_peer_net(ops, tb, data, extack);
 			if (IS_ERR(peer_net)) {
 				ret = PTR_ERR(peer_net);
 				goto put_ops;
---8<---