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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 20.10.26 19:11, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message is inaccurate about where the crash happens.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but isotp_bind()
>    in net/can/isotp.c still ignores the return value of both…
> 
> --
> 
> 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 TUNSETLINK
> ioctl can switch a TUN/TAP device to ARPHRD_CAN while dev->ml_priv stays
> NULL, and such a device passes the old type checks.
> 
> Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net
> Link: https://patch.msgid.link/20261019152741.54854-5-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?
> 
> Commit 8aa59e3559494 ("can: af_can: fix NULL pointer dereference in
> can_rx_register()") already made can_rx_register() reject devices
> without ml_priv:
> 
>      if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
>          return -ENODEV;

The commit message refers to the CAN filter register/unregister
procedure as a whole, not to a single code path in a specific tree.

Commit 8aa59e3559494 only added the ml_priv check to can_rx_register().
can_rx_unregister() and the CAN protocols kept checking dev->type only.
So that commit fixed the problem only partially, and the NULL pointer
dereference is still reachable on the unregister side.

This patch completes that fix. It uses can_get_ml_priv() as the only
criterion for a real CAN device in all register/unregister paths and in
the protocols' bind/notifier code. It no longer relies on each caller
handling the can_rx_register() return value correctly.

> 
> raw, bcm, gw (through cgw_register_filter()) and isotp all create filters
> through can_rx_register(). So creating a filter can't reach the NULL
> ml_priv.
> 
> The only dereference left seems to be on removal. can_rx_unregister()
> checked only dev->type, and then can_dev_rcv_lists_find() does:
> 
>      struct can_ml_priv *can_ml = can_get_ml_priv(dev);
>      return &can_ml->dev_rcv_lists;
> 
> The spin_lock_bh() on dev_rcv_lists->lock then goes through a near-NULL
> pointer.
> 
> The path that actually gets there looks like isotp. isotp_bind() ignores
> the -ENODEV from both can_rx_register() calls and sets so->bound = 1
> anyway. Later, unregistering the filters crashes:
> 
> isotp_bind()
>      can_rx_register() -> -ENODEV, ignored
>      so->bound = 1
> 
> isotp_release() or isotp_notify(NETDEV_UNREGISTER)
>      can_rx_unregister()
>          can_dev_rcv_lists_find()
>          spin_lock_bh(&dev_rcv_lists->lock)
> 
> The code change itself looks correct. Could the commit message describe
> the crash on removal and the isotp path instead? That would show stable
> backporters which code is affected.
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 64ff203a37f4..5e1c3971ebfc 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
> 
> [ ... ]
> 
>> @@ -1618,7 +1619,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 is a pre-existing issue and was not introduced by this patch.
> Further down in isotp_bind(), the return values of both can_rx_register()
> calls are still ignored:

Right. That's a separate issue, and it doesn't belong to this patch. The
missing error handling in isotp_bind() will be addressed in a separate
patch.

Best regards,
Oliver

> 
> 	if (isotp_register_rxid(so))
> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> 				isotp_rcv, sk, sock_i_ino(sk), sk);
> 	...
> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> 			isotp_rcv_echo, sk, sock_i_ino(sk), sk);
> 
> 	/* switch to new settings */
> 	...
> 	so->bound = 1;
> 
> The new can_get_ml_priv() check rules out the -ENODEV cases. However,
> can_rx_register() can still return -ENOMEM:
> 
> 	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
> 	if (!rcv)
> 		return -ENOMEM;
> 
> In that case, does bind() report success even though the socket has no RX
> receiver, no echo receiver, or neither? RX frames would be dropped without
> any error, and isotp TX would wait for echo frames that never arrive.
> 
> A later isotp_release() or isotp_notify() for NETDEV_UNREGISTER would
> only print "receive list entry not found" from can_rx_unregister(). It
> would not crash.
> 
> Other CAN protocols handle this: raw_enable_filters() rolls back on error,
> and bcm_rx_setup() checks err. Should isotp_bind() check these results and
> unwind too? The last patch in this series ("can: protect the receive lists
> with a per-device lock") still leaves both calls unchecked.
>