Messages in this thread | | | Date | Thu, 06 Aug 2009 22:58:22 +0200 | From | Oliver Hartkopp <> | Subject | Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less restrictive |
| |
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
| |