On Fri, Aug 18, 2023 at 06:26:02PM -0700, Jakub Kicinski wrote:
> veth and vxcan need to make sure the ifindexes of the peer
> are not negative, core does not validate this.
>
> Using iproute2 with user-space-level checking removed:
>
> Before:
>
> # ./ip link add index 10 type veth peer index -1
> # ip link show
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> 2: enp1s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
> link/ether 52:54:00:74:b2:03 brd ff:ff:ff:ff:ff:ff
> 10: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> link/ether 8a:90:ff:57:6d:5d brd ff:ff:ff:ff:ff:ff
> -1: veth0@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> link/ether ae:ed:18:e6:fa:7f brd ff:ff:ff:ff:ff:ff
>
> Now:
>
> $ ./ip link add index 10 type veth peer index -1
> Error: ifindex can't be negative.
>
> This problem surfaced in net-next because an explicit WARN()
> was added, the root cause is older.
>
> Fixes: e6f8f1a739b6 ("veth: Allow to create peer link with given ifindex")
> Fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
> Reported-by: syzbot+5ba06978f34abb058571@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
There is another report here [1] with a reproducer [2]. Even with this
patch, the reproducer can still trigger the warning on net-next. Don't
we also need to reject a negative ifindex in the ancillary header? At
least with the following diff the warning does not trigger anymore:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 7aba4d63b069..4a2ec33bfb51 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3560,6 +3560,9 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
if (ifm->ifi_index > 0) {
link_specified = true;
dev = __dev_get_by_index(net, ifm->ifi_index);
+ } else if (ifm->ifi_index < 0) {
+ NL_SET_ERR_MSG(extack, "ifindex can't be negative");
+ return -EINVAL;
} else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]) {
link_specified = true;
dev = rtnl_dev_get(net, tb);
[1] https://syzkaller.appspot.com/text?tag=CrashReport&x=178edad3a80000
[2] https://syzkaller.appspot.com/text?tag=ReproC&x=166ed6bba80000
