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

Edward Adam Davis posted 1 patch 2 weeks ago
net/can/af_can.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] can: af_can: reject can rx unregister if dev is not can
Posted by Edward Adam Davis 2 weeks ago
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8ed98cbd0161632bce95
Tested-by: syzbot+8ed98cbd0161632bce95@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 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)))
-- 
2.43.0
Re: [PATCH] can: af_can: reject can rx unregister if dev is not can
Posted by Oliver Hartkopp 2 weeks ago

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=8ed98cbd0161632bce95
> Tested-by: syzbot+8ed98cbd0161632bce95@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

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@hartkopp.net/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)))
Re: [PATCH] can: af_can: reject can rx unregister if dev is not can
Posted by Edward Adam Davis 1 week, 6 days ago
On Mon, 25 May 2026 20:15:21 +0200, Oliver Hartkopp 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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=8ed98cbd0161632bce95
> > Tested-by: syzbot+8ed98cbd0161632bce95@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> 
> 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.
I noticed the "bonding" aspect, but I haven't yet delved deeply into
understanding why a vxcan interface cannot be enslaved to a bonding
net dev. After testing your patch, I observed that sockets previously
bound to the bonding net dev are no longer bound to that bonding net dev.

BR,
Edward
Re: [PATCH] can: af_can: reject can rx unregister if dev is not can
Posted by Oliver Hartkopp 1 week, 6 days ago

On 26.05.26 01:46, Edward Adam Davis wrote:
> On Mon, 25 May 2026 20:15:21 +0200, Oliver Hartkopp 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@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=8ed98cbd0161632bce95
>>> Tested-by: syzbot+8ed98cbd0161632bce95@syzkaller.appspotmail.com
>>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>>
>> 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.
> I noticed the "bonding" aspect, but I haven't yet delved deeply into
> understanding why a vxcan interface cannot be enslaved to a bonding
> net dev. After testing your patch, I observed that sockets previously
> bound to the bonding net dev are no longer bound to that bonding net dev.

I don't understand this last sentence.

With my patch the CAN interfaces can not be enslaved anymore.
As Syzbot is simply doing whatever is possible, it get's an error and 
this error path is closed. Of course Syzbot can still test AF_CAN sockets.

Regarding your observation the I assume that the bonding driver makes an 
interface down/up cycle or something similar which might have such an 
effect.

But in the end it doesn't crash anymore when the bonding driver is 
trying to fiddle with CAN interfaces.

Best regards,
Oliver