Skip to content

Commit 4832756

Browse files
Cong WangPaolo Abeni
Cong Wang
authored and
Paolo Abeni
committed
rtnetlink: fix double call of rtnl_link_get_net_ifla()
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: [email protected] Closes: https://syzkaller.appspot.com/bug?extid=21ba4d5adff0b6a7cfc6 Fixes: 0eb87b0 ("veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.") Fixes: 6b84e55 ("vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.") Fixes: fefd5d0 ("netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.") Cc: Kuniyuki Iwashima <[email protected]> Signed-off-by: Cong Wang <[email protected]> Reviewed-by: Kuniyuki Iwashima <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Paolo Abeni <[email protected]>
1 parent 7a0ea70 commit 4832756

File tree

4 files changed

+31
-46
lines changed

4 files changed

+31
-46
lines changed

‎drivers/net/can/vxcan.c

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,13 +172,12 @@ static void vxcan_setup(struct net_device *dev)
172172
/* forward declaration for rtnl_create_link() */
173173
static struct rtnl_link_ops vxcan_link_ops;
174174

175-
static int vxcan_newlink(struct net *net, struct net_device *dev,
175+
static int vxcan_newlink(struct net *peer_net, struct net_device *dev,
176176
struct nlattr *tb[], struct nlattr *data[],
177177
struct netlink_ext_ack *extack)
178178
{
179179
struct vxcan_priv *priv;
180180
struct net_device *peer;
181-
struct net *peer_net;
182181

183182
struct nlattr *peer_tb[IFLA_MAX + 1], **tbp = tb;
184183
char ifname[IFNAMSIZ];
@@ -203,20 +202,15 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
203202
name_assign_type = NET_NAME_ENUM;
204203
}
205204

206-
peer_net = rtnl_link_get_net(net, tbp);
207205
peer = rtnl_create_link(peer_net, ifname, name_assign_type,
208206
&vxcan_link_ops, tbp, extack);
209-
if (IS_ERR(peer)) {
210-
put_net(peer_net);
207+
if (IS_ERR(peer))
211208
return PTR_ERR(peer);
212-
}
213209

214210
if (ifmp && dev->ifindex)
215211
peer->ifindex = ifmp->ifi_index;
216212

217213
err = register_netdevice(peer);
218-
put_net(peer_net);
219-
peer_net = NULL;
220214
if (err < 0) {
221215
free_netdev(peer);
222216
return err;

‎drivers/net/netkit.c

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ static int netkit_validate(struct nlattr *tb[], struct nlattr *data[],
327327

328328
static struct rtnl_link_ops netkit_link_ops;
329329

330-
static int netkit_new_link(struct net *src_net, struct net_device *dev,
330+
static int netkit_new_link(struct net *peer_net, struct net_device *dev,
331331
struct nlattr *tb[], struct nlattr *data[],
332332
struct netlink_ext_ack *extack)
333333
{
@@ -342,7 +342,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
342342
struct net_device *peer;
343343
char ifname[IFNAMSIZ];
344344
struct netkit *nk;
345-
struct net *net;
346345
int err;
347346

348347
if (data) {
@@ -385,13 +384,10 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
385384
(tb[IFLA_ADDRESS] || tbp[IFLA_ADDRESS]))
386385
return -EOPNOTSUPP;
387386

388-
net = rtnl_link_get_net(src_net, tbp);
389-
peer = rtnl_create_link(net, ifname, ifname_assign_type,
387+
peer = rtnl_create_link(peer_net, ifname, ifname_assign_type,
390388
&netkit_link_ops, tbp, extack);
391-
if (IS_ERR(peer)) {
392-
put_net(net);
389+
if (IS_ERR(peer))
393390
return PTR_ERR(peer);
394-
}
395391

396392
netif_inherit_tso_max(peer, dev);
397393

@@ -408,7 +404,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
408404
bpf_mprog_bundle_init(&nk->bundle);
409405

410406
err = register_netdevice(peer);
411-
put_net(net);
412407
if (err < 0)
413408
goto err_register_peer;
414409
netif_carrier_off(peer);

‎drivers/net/veth.c

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,7 +1765,7 @@ static int veth_init_queues(struct net_device *dev, struct nlattr *tb[])
17651765
return 0;
17661766
}
17671767

1768-
static int veth_newlink(struct net *src_net, struct net_device *dev,
1768+
static int veth_newlink(struct net *peer_net, struct net_device *dev,
17691769
struct nlattr *tb[], struct nlattr *data[],
17701770
struct netlink_ext_ack *extack)
17711771
{
@@ -1776,7 +1776,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
17761776
struct nlattr *peer_tb[IFLA_MAX + 1], **tbp;
17771777
unsigned char name_assign_type;
17781778
struct ifinfomsg *ifmp;
1779-
struct net *net;
17801779

17811780
/*
17821781
* create and register peer first
@@ -1800,13 +1799,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
18001799
name_assign_type = NET_NAME_ENUM;
18011800
}
18021801

1803-
net = rtnl_link_get_net(src_net, tbp);
1804-
peer = rtnl_create_link(net, ifname, name_assign_type,
1802+
peer = rtnl_create_link(peer_net, ifname, name_assign_type,
18051803
&veth_link_ops, tbp, extack);
1806-
if (IS_ERR(peer)) {
1807-
put_net(net);
1804+
if (IS_ERR(peer))
18081805
return PTR_ERR(peer);
1809-
}
18101806

18111807
if (!ifmp || !tbp[IFLA_ADDRESS])
18121808
eth_hw_addr_random(peer);
@@ -1817,8 +1813,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
18171813
netif_inherit_tso_max(peer, dev);
18181814

18191815
err = register_netdevice(peer);
1820-
put_net(net);
1821-
net = NULL;
18221816
if (err < 0)
18231817
goto err_register_peer;
18241818

‎net/core/rtnetlink.c

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3746,6 +3746,7 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
37463746
static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
37473747
const struct rtnl_link_ops *ops,
37483748
struct net *tgt_net, struct net *link_net,
3749+
struct net *peer_net,
37493750
const struct nlmsghdr *nlh,
37503751
struct nlattr **tb, struct nlattr **data,
37513752
struct netlink_ext_ack *extack)
@@ -3776,8 +3777,13 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
37763777

37773778
dev->ifindex = ifm->ifi_index;
37783779

3780+
if (link_net)
3781+
net = link_net;
3782+
if (peer_net)
3783+
net = peer_net;
3784+
37793785
if (ops->newlink)
3780-
err = ops->newlink(link_net ? : net, dev, tb, data, extack);
3786+
err = ops->newlink(net, dev, tb, data, extack);
37813787
else
37823788
err = register_netdevice(dev);
37833789
if (err < 0) {
@@ -3812,40 +3818,33 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
38123818
goto out;
38133819
}
38143820

3815-
static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
3816-
const struct rtnl_link_ops *ops,
3817-
struct nlattr *data[],
3818-
struct netlink_ext_ack *extack)
3821+
static struct net *rtnl_get_peer_net(const struct rtnl_link_ops *ops,
3822+
struct nlattr *data[],
3823+
struct netlink_ext_ack *extack)
38193824
{
38203825
struct nlattr *tb[IFLA_MAX + 1];
3821-
struct net *net;
38223826
int err;
38233827

38243828
if (!data || !data[ops->peer_type])
3825-
return 0;
3829+
return NULL;
38263830

38273831
err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack);
38283832
if (err < 0)
3829-
return err;
3833+
return ERR_PTR(err);
38303834

38313835
if (ops->validate) {
38323836
err = ops->validate(tb, NULL, extack);
38333837
if (err < 0)
3834-
return err;
3838+
return ERR_PTR(err);
38353839
}
38363840

3837-
net = rtnl_link_get_net_ifla(tb);
3838-
if (IS_ERR(net))
3839-
return PTR_ERR(net);
3840-
if (net)
3841-
rtnl_nets_add(rtnl_nets, net);
3842-
3843-
return 0;
3841+
return rtnl_link_get_net_ifla(tb);
38443842
}
38453843

38463844
static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
38473845
const struct rtnl_link_ops *ops,
38483846
struct net *tgt_net, struct net *link_net,
3847+
struct net *peer_net,
38493848
struct rtnl_newlink_tbs *tbs,
38503849
struct nlattr **data,
38513850
struct netlink_ext_ack *extack)
@@ -3894,14 +3893,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
38943893
return -EOPNOTSUPP;
38953894
}
38963895

3897-
return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, nlh, tb, data, extack);
3896+
return rtnl_newlink_create(skb, ifm, ops, tgt_net, link_net, peer_net, nlh,
3897+
tb, data, extack);
38983898
}
38993899

39003900
static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
39013901
struct netlink_ext_ack *extack)
39023902
{
3903+
struct net *tgt_net, *link_net = NULL, *peer_net = NULL;
39033904
struct nlattr **tb, **linkinfo, **data = NULL;
3904-
struct net *tgt_net, *link_net = NULL;
39053905
struct rtnl_link_ops *ops = NULL;
39063906
struct rtnl_newlink_tbs *tbs;
39073907
struct rtnl_nets rtnl_nets;
@@ -3971,9 +3971,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
39713971
}
39723972

39733973
if (ops->peer_type) {
3974-
ret = rtnl_add_peer_net(&rtnl_nets, ops, data, extack);
3975-
if (ret < 0)
3974+
peer_net = rtnl_get_peer_net(ops, data, extack);
3975+
if (IS_ERR(peer_net))
39763976
goto put_ops;
3977+
if (peer_net)
3978+
rtnl_nets_add(&rtnl_nets, peer_net);
39773979
}
39783980
}
39793981

@@ -4004,7 +4006,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
40044006
}
40054007

40064008
rtnl_nets_lock(&rtnl_nets);
4007-
ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
4009+
ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, peer_net, tbs, data, extack);
40084010
rtnl_nets_unlock(&rtnl_nets);
40094011

40104012
put_net:

0 commit comments

Comments
 (0)