netdev - Re: [PATCH bpf-next v2 1/7] netkit, bpf: Add bpf programmable net device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <33467f55-4bbf-4078-af21-d91c6aab82ee@lunn.ch>
Date: Sat, 21 Oct 2023 00:18:53 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, martin.lau@...ux.dev,
	razor@...ckwall.org, ast@...nel.org, andrii@...nel.org,
	john.fastabend@...il.com, sdf@...gle.com, toke@...nel.org
Subject: Re: [PATCH bpf-next v2 1/7] netkit, bpf: Add bpf programmable net
 device

> +static void netkit_get_drvinfo(struct net_device *dev,
> +			       struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRV_NAME, sizeof(info->driver));
> +	strscpy(info->version, "n/a", sizeof(info->version));

If you don't put anything in version, the core will put in the git
hash of the kernel. Its more useful than "n/a".

> +	ether_setup(dev);
> +	dev->min_mtu = ETH_MIN_MTU;

ether_setup() sets min_mtu to ETH_MIN_MTU.

> +static int netkit_new_link(struct net *src_net, struct net_device *dev,
> +			   struct nlattr *tb[], struct nlattr *data[],
> +			   struct netlink_ext_ack *extack)
> +{

...

> +	err = register_netdevice(peer);
> +	put_net(net);
> +	if (err < 0)
> +		goto err_register_peer;
> +
> +	netif_carrier_off(peer);
> +
> +	err = rtnl_configure_link(peer, ifmp, 0, NULL);
> +	if (err < 0)
> +		goto err_configure_peer;

Seeing code after calling register_netdevice() often means bugs. The
interface is live, and in use before the function even returns. The
kernel can try to get an IP address, mount an NFS root etc. This might
be safe, because you have two linked interfaces here, and the other
one is not yet registered. Maybe some comment about this would be
good, or can the rtnl_configure_link() be done earlier?

> +
> +	if (mode == NETKIT_L2)
> +		eth_hw_addr_random(dev);
> +	if (tb[IFLA_IFNAME])
> +		nla_strscpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
> +	else
> +		snprintf(dev->name, IFNAMSIZ, "m%%d");
> +
> +	err = register_netdevice(dev);
> +	if (err < 0)
> +		goto err_configure_peer;

We have the same here, but now we have both peers registers, the
kernel could of configured both up in order to find its NFS root etc.
Is it safe to have packets flowing at this point? Before the remaining
configuration happens?

> +
> +	netif_carrier_off(dev);
> +
> +	nk = netdev_priv(dev);
> +	nk->primary = true;
> +	nk->policy = default_prim;
> +	nk->mode = mode;
> +	if (nk->mode == NETKIT_L2)
> +		dev_change_flags(dev, dev->flags & ~IFF_NOARP, NULL);
> +	bpf_mprog_bundle_init(&nk->bundle);
> +	RCU_INIT_POINTER(nk->active, NULL);
> +	rcu_assign_pointer(nk->peer, peer);
> +
> +	nk = netdev_priv(peer);
> +	nk->primary = false;
> +	nk->policy = default_peer;
> +	nk->mode = mode;
> +	if (nk->mode == NETKIT_L2)
> +		dev_change_flags(peer, peer->flags & ~IFF_NOARP, NULL);
> +	bpf_mprog_bundle_init(&nk->bundle);
> +	RCU_INIT_POINTER(nk->active, NULL);
> +	rcu_assign_pointer(nk->peer, dev);
> +	return 0;
> +err_configure_peer:
> +	unregister_netdevice(peer);
> +	return err;
> +err_register_peer:
> +	free_netdev(peer);
> +	return err;
> +}


  Andrew