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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 14.10.26 10:37, netdev-bot+sashiko@kernel.org wrote:
> 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.

I'll send a v4 covering ARPHRD_6LOWPAN too.
> 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?

No. We had that discussion already on the ML.

team (like bonding) intentionally supports non-Ethernet ports via 
team_setup_by_port() (e.g. IPoIB), so an allowlist would regress 
existing users. The denylist targets the L2 types whose stacks rely on 
type-specific private data.


> [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?

Yes. When this team patch is settled and shows up in the tree, I'll 
create a follow-up for bonding for those 4 exceptions.

Best regards,
Oliver

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