Re: [PATCH net-next 03/37] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 10.10.26 16:02, 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 calls the change a conversion of 'unreliable'
>    ARPHRD_CAN checks to 'robust' can_get_ml_priv() checks.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but isotp_bind()
>    in net/can/isotp.c ignores the return values of both can_rx_register()…
> 
> --
> 
> can: convert unreliable ARPHRD_CAN type checks to robust 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 reason given is
> that TUNSETLINK can switch a down TUN/TAP device to ARPHRD_CAN while its
> ml_priv stays NULL.
> 
> Link: https://patch.msgid.link/20261009121115.61012-1-socketcan@hartkopp.net
> 
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 34fe3b28d576f..dc27ace43719d 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
> 
> [ ... ]
> 
>> @@ -519,7 +519,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>   	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
>>   	struct can_dev_rcv_lists *dev_rcv_lists;
>>   
>> -	if (dev && dev->type != ARPHRD_CAN)
>> +	if (dev && !can_get_ml_priv(dev))
>>   		return;
> 
> [Severity: Low]
> The commit message describes this change as replacing "unreliable" checks
> with "robust" ones. Should it also say that the change closes a NULL
> pointer dereference that a user can trigger?
> 
> Before this patch, the sequence looks like this:
> 
>    ioctl(TUNSETLINK, ARPHRD_CAN) on a down tun device
>      tun->dev->type = (int) arg;      /* ml_priv stays NULL */
>    isotp_bind()
>      old dev->type != ARPHRD_CAN check passes
>      can_rx_register() returns -ENODEV, return value ignored
>      so->bound = 1; so->dev = dev;
>    isotp_release() or isotp_notify()
>      can_rx_unregister(net, so->dev, ...)
>        old dev->type != ARPHRD_CAN guard passes
>        can_dev_rcv_lists_find()
>          can_ml = can_get_ml_priv(dev);   /* NULL */
>          return &can_ml->dev_rcv_lists;
>        hlist_for_each_entry_rcu() walks the near-NULL pointer
> 
> dev_rcv_lists sits at offset 0 of struct can_ml_priv, so the returned
> pointer is near-NULL and the list walk oopses.
> 
> Triggering this only requires owning a tun device, or CAP_NET_ADMIN inside
> an unprivileged user namespace.
> 
> The patch carries a Cc: stable tag. Could the message name the oops in
> can_rx_unregister() (reached through isotp_release() or isotp_notify()) and
> the privilege needed to trigger it?

Hi Marc,

do you want me to rephrase the commit message?

The referenced "Fixes" commit from Oleksij
4e096a18867a5a989b ("net: introduce CAN specific pointer in the struct 
net_device") was introduced in Linux 5.12 but is available in stable 5.10.

Best regards,
Oliver

> 
> [ ... ]
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index f5dc9d04bd686..6c28802c0605f 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 is a pre-existing issue, not one introduced by this patch. Further
> down, isotp_bind() still ignores the return values of both
> can_rx_register() calls:
> 
> 	if (isotp_register_rxid(so))
> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> 				isotp_rcv, sk, ...);
> 	...
> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> 			isotp_rcv_echo, sk, ...);
> 
> 	/* switch to new settings */
> 	...
> 	so->bound = 1;
> 
> The can_get_ml_priv() check above now rules out -ENODEV. Using
> dev_get_by_index(net, ...) also guarantees the netns matches. That leaves
> one possible failure: -ENOMEM from can_rx_register():
> 
> 	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
> 	if (!rcv)
> 		return -ENOMEM;
> 
> If that allocation fails, bind() still returns 0. The socket then never
> receives flow control frames, or never receives its own TX echoes.
> 
> Transfers would stall until the hrtimers fire. Later, isotp_release() or
> isotp_notify() would hit the "receive list entry not found" warning in
> can_rx_unregister().
> 
> Should isotp_bind() check these return values, and unwind the first
> registration if the second one fails?
>