Re: [PATCH net] net: validate veth and vxcan peer ifindexes

[Ido Schimmel <idosch@idosch.org>]

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@syzkaller.appspotmail.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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