linux-kernel - Re: [Socketcan-users] [PATCH] CAN: make checking in can

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4A7B43EE.8050601@hartkopp.net>
Date:	Thu, 06 Aug 2009 22:58:22 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Luotao Fu <l.fu@...gutronix.de>
CC:	socketcan-users@...ts.berlios.de,
	Michael Olbrich <m.olbrich@...gutronix.de>,
	linux-kernel@...r.kernel.org
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/