Re: [PATCH v3] team: reject CAN and IEEE 802.15.4 devices in team_port_add

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new denylist in team_port_add()
  (drivers/net/team/team_core.c:1228-1236) only rejects ARPHRD_CAN,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the bonding
  driver, which the commit message cites as the precedent being…

--

team: reject CAN and IEEE 802.15.4 devices in team_port_add

This patch makes team_port_add() reject ports of type ARPHRD_CAN,
ARPHRD_IEEE802154 and ARPHRD_IEEE802154_MONITOR. Team never sets up the
private mid-layer data these L2 types expect, so enslaving one leads to a
NULL pointer dereference. The check mirrors the CAN check bonding has had
since commit 8ba68464e478.

Link: https://lore.kernel.org/all/b58d98e0-8fe3-4e4e-b8a5-c1c7e647ccca@hartkopp.net/

> diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
> index ca75f08e3c593..5f1708c2a3445 100644
> --- a/drivers/net/team/team_core.c
> +++ b/drivers/net/team/team_core.c
> @@ -1225,6 +1225,16 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
>  		return -EINVAL;
>  	}
>  
> +	if (port_dev->type == ARPHRD_CAN ||
> +	    port_dev->type == ARPHRD_IEEE802154 ||
> +	    port_dev->type == ARPHRD_IEEE802154_MONITOR) {

[Severity: High]
Does this list cover the whole class of devices the commit message
describes? ARPHRD_6LOWPAN devices still seem to get through
team_port_add(). Examples are lowpan0 on top of wpan0, or a Bluetooth bt0.

For the first port of a different type, the call chain
team_port_add()->team_dev_type_check_change()->team_setup_by_port() copies
the port's type onto the team device:

drivers/net/team/team_core.c:team_setup_by_port() {
    ...
	dev->type = port_dev->type;
    ...
}

At that point netdev_priv() of the team device is still a struct team. None
of the NETDEV_PRE_TYPE_CHANGE notifiers seem to refuse the change here. The
team device's inet6_dev also survives it.

The global lowpan notifier checks only the type and the inet6_dev before it
writes through lowpan_dev(dev):

net/6lowpan/core.c:lowpan_event() {
	if (dev->type != ARPHRD_6LOWPAN)
		return NOTIFY_DONE;

	idev = __in6_dev_get(dev);
	if (!idev)
		return NOTIFY_DONE;
    ...
	case NETDEV_DOWN:
		for (i = 0; i < LOWPAN_IPHC_CTX_TABLE_SIZE; i++)
			clear_bit(LOWPAN_IPHC_CTX_FLAG_ACTIVE,
				  &lowpan_dev(dev)->ctx.table[i].flags);
    ...
}

When the team device is set down, would these 16 clear_bit() calls write
into struct team? The exact offsets depend on the config. On a typical
x86_64 config without spinlock debugging, they appear to hit:

- notify_peers.dw.work.data (the pending bit)
- notify_peers.dw.timer.flags
- mcast_rejoin.count_pending
- mode_priv[0]

The last write lands at about sizeof(struct team), which is just past the
private area.

On NETDEV_UP and NETDEV_CHANGE, lowpan_is_ll() also reads
lowpan_dev(dev)->lltype. That field overlaps team->pcpu_stats.

Would an allowlist of supported port types be more robust here than a
longer denylist?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The commit
message says this change mirrors a bonding check, but that check only
rejects CAN:

drivers/net/bonding/bond_main.c:bond_enslave() {
    ...
	if (slave_dev->type == ARPHRD_CAN) {
		BOND_NL_ERR(bond_dev, extack,
			    "CAN devices cannot be enslaved");
		return -EPERM;
	}
    ...
}

Does bonding have the same IEEE 802.15.4 hole? For a non-Ethernet first
slave, bond_enslave()->bond_setup_by_slave() copies slave_dev->type onto
the bond. A wpan device appears to pass all the remaining checks. The bond
then ends up as ARPHRD_IEEE802154 with a NULL ieee802154_ptr.

ieee802154_get_dev() filters only on the type before it dereferences that
pointer:

net/ieee802154/socket.c:ieee802154_get_dev() {
    ...
		for_each_netdev(net, tmp) {
			if (tmp->type != ARPHRD_IEEE802154)
				continue;

			pan_id = tmp->ieee802154_ptr->pan_id;
    ...
}

This path runs from bind() or connect() with a short address on an
AF_IEEE802154 SOCK_DGRAM socket. ieee802154_create() needs no capability
for SOCK_DGRAM.

Once a bond has a wpan slave, could an unprivileged user oops the kernel
this way? Should bond_enslave() get the same IEEE 802.15.4 checks in a
separate patch?

> +		NL_SET_ERR_MSG(extack,
> +			       "CAN and IEEE 802.15.4 devices can't be added as a team port");

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013083523.4728-1-socketcan%40hartkopp.net