Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less restrictive

[Oliver Hartkopp <socketcan@hartkopp.net>]

Luotao Fu wrote:
> Hi Oliver,
> 

>>> -	BUG_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8);
>>> +	WARN_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8);
>>>  
>>>  	/* update statistics */
>>>  	can_stats.rx_frames++;
>> NAK.
>>
>> The CAN applications can rely on getting proper CAN frames with this check. It
>> was introduced some time ago together with several other sanity checks - even
>> on the TX path.
>>
>> The CAN core *only* consumes skbuffs originated from a CAN netdevice
>> (ARPHRD_CAN).
> 
> I don't quite get it. The problem here is a broken can message sent to
> the device can bring down the kernel.

I assume you mean from the wire via the controller to the Kernel here, right?

> 
>> When this BUG() triggers, someone provided a definitely broken *CAN* network
>> driver, and this needsfp to be fixed on that level. 
> 
> In our case a sender (a FPGA) generates correct can frames carrying
> wrong dlc length.

Which is therefore *NOT* a correct CAN frame.

> This way the can driver on our side runs into the bug
> though the driver itself is allright.

Whatever there is on the bus or whatever your CAN controller provides in it's
dlc value: You need to ensure that the dlc is 0..8 before you push it into the
skbuff and call netif_rx(). Everything else *is* broken and not CAN conform.

> The opposite needed to be fixed,
> not our side.

Sure but it's your turn to be robust against obviously wrongs stuff, that's
provided by your (obviously sloppy) CAN controller.

>  Though we do suffer a system crash only because the
> sender sends trash into the can network. This is imo quite bad.

No. You suffer because you allow the trash to climb up into the system.

Anyway i really wonder that there is a CAN controller that provides you
information in its registers that describe a non conform CAN frame.

This discussion shows that using BUG() was the correct approach :-)

Fix your driver and do not allow to pass broken stuff into the system.

Cheers,
Oliver