Re: [PATCH net-next 03/37] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

On 16.10.26 10:42, Marc Kleine-Budde wrote:

On 2026-10-16 02:37 +0000, netdev-bot+sashiko@kernel.org wrote:

quoted

can: replace ARPHRD_CAN type checks with can_get_ml_priv()

This patch replaces the remaining dev->type == ARPHRD_CAN checks in
af_can, bcm, gw, isotp and raw with can_get_ml_priv(). The goal is to
keep TUN/TAP devices that were retyped to ARPHRD_CAN via TUNSETLINK away
from CAN code that dereferences a NULL dev->ml_priv.

Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net (local)

quoted

The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware
type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed
netdevice passes the ARPHRD_CAN checks and triggers a NULL pointer
dereference when CAN filters are created and when they are removed,
e.g. by the CAN protocols' netdevice notifier handlers.

[Severity: Low]
Is the "when CAN filters are created" part accurate for this tree?

Before this patch, can_rx_register() already rejected such a device:

net/can/af_can.c:can_rx_register() {
     ...
	if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
		return -ENODEV;
     ...
}

can_rcv(), canfd_rcv() and canxl_rcv() also already tested
!can_get_ml_priv(dev). That makes those four af_can.c hunks look like
simplifications that don't change behavior. This holds for kernels
that already carry commit 8aa59e3559494.

The anslysis looks correct to me. Oliver, can you re-spin this patch
with an updated description?

Done

https://lore.kernel.org/linux-can/20261016093525.327367-1-socketcan@hartkopp.net/ (local)

pw-bot: cr

quoted

[ ... 26 lines skipped ... ]

quoted

The code change itself looks complete. No ARPHRD_CAN checks remain in
net/can/ after this patch.

quoted

diff --git a/net/can/isotp.c b/net/can/isotp.c
index f5dc9d04bd68..6c28802c0605 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c

[ ... ]

quoted

@@ -1610,7 +1611,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
  		err = -ENODEV;
  		goto out;
  	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
  		err = -ENODEV;
  		goto out_put_dev;
  	}

[Severity: Medium]
This issue was not introduced by this patch. Further down in
isotp_bind(), the return values of both can_rx_register() calls are
thrown away, and the socket is marked bound anyway:

	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, "isotp", sk);
	...
	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
			isotp_rcv_echo, sk, "isotpe", sk);

	/* switch to new settings */
	...
	so->bound = 1;

Oliver, can you add error handling for can_rx_register() to isotp?

No. I'm really getting pissed about being urged by sahiko-bot changing commit messages because "those four af_can.c hunks look like simplifications that don't change behavior". Really? Every normal reviewer would see that and created this fact in his mind automatically.

When this patch is applied isotp_bind() can only fail with -ENOMEM and nothing breaks. The socket just becomes unusable. And if we have -ENOMEM we have a big problem anyway.

If I would provide such a patch the proc pointer removal patch would break next.

I have ~20 patches for isotp and its CAN XL support. I will add the isotp_bind() error handling there when these current (can-next) fixes are in mainline.

Best regards,
Oliver