Re: [PATCH] can: af_can: reject can rx unregister if dev is not can

[Oliver Hartkopp]

On 25.05.26 15:56, Edward Adam Davis wrote:
> When a user binds a non-CAN device to a socket, the vulnerability reported
> in [1] is triggered during the socket's closure and release phase, due to
> the inability to find the expected receive list.
>
> Added checks for Mid-layer private and type during the rx unregistration
> process.
>
> [1]
> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
> pc : can_rx_unregister+0x124/0x560 net/can/af_can.c:537
> Call trace:
>   can_rx_unregister+0x124/0x560 net/can/af_can.c:531 (P)
>   isotp_release+0x500/0x9d8 net/can/isotp.c:1232
>   __sock_release+0xa0/0x1d4 net/socket.c:722
>   sock_close+0x24/0x38 net/socket.c:1514
>
> Fixes: bdfb5765e45b ("can: af_can: remove NULL-ptr checks from users of can_dev_rcv_lists_find()")
> Reported-by: syzbot+8ed98cbd0161632bce95@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=8ed98cbd0161632bce95
> Tested-by: syzbot+8ed98cbd0161632bce95@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>

Hello Edward,

many thanks for your investigation an effort to address the syzcaller issue!

Btw. the root cause of the problem, that the receive lists can not be 
accessed is the bonding process that the bonding driver mutates
and modifies the network device states to fit an Ethernet-like
aggregation model. Which destroys the can_ml_priv.

When CAN netdevices are left alone the can_ml_priv data is always valid 
and therefore does not need to be checked. Additionally this bonding 
process and your fix will lead to memleaks of CAN filter data.

Syzcaller can continue its work to test the CAN API also without bonding.

So it seems to be the better solution to reject CAN interfaces to be 
bonded. See my patch here:

https://lore.kernel.org/linux-can/20260525175639.112492-1-socketcan@xxxxxxxxxxxx/T/#u

I intentionally missed to add the bonding maintainers - and I'm not yet 
clear what Fixes: tag would be appropriate. Does it fix the

commit ccb29637991f [CAN]: Add virtual CAN netdevice driver

??

What do you think?

Best regards,
Oliver

> ---
>   net/can/af_can.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 7bc86b176b4d..72831b4e0776 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 && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
>   		return;
>   
>   	if (dev && !net_eq(net, dev_net(dev)))