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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 16.10.26 10:42, Marc Kleine-Budde wrote:
> On 2026-10-16 02:37 +0000, netdev-bot+sashiko@kernel.org wrote:
>> 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
>>
>>> 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/

> 
> pw-bot: cr
> 
>> [ ... 26 lines skipped ... ]
> 
>> The code change itself looks complete. No ARPHRD_CAN checks remain in
>> net/can/ after this patch.
>>
>>> 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
>>
>> [ ... ]
>>
>>> @@ -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