[PATCH] CAN: make checking in can_rcv less restrictive

[PATCH] CAN: make checking in can_rcv less restrictive

[Luotao Fu]

From: Michael Olbrich <m.olbrich@pengutronix.de>

Checking for can frame format in can_rcv() is too restrictive. BUG_ON is way too
heavy for the case that the can interface probably received a can frame with
malicious format. Further it can be used for DDOS attack since BUG_ON can lead
to kernel panic. Hence we turn this to WARN_ON instead.

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
Signed-off-by: Luotao Fu <l.fu@pengutronix.de>
---
 net/can/af_can.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index e733725..e6dcf4b 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -656,7 +656,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		return 0;
 	}
 
-	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++;
-- 
1.6.3.3

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

[Oliver Hartkopp]

Luotao Fu wrote:
> From: Michael Olbrich <m.olbrich@pengutronix.de>
> 
> Checking for can frame format in can_rcv() is too restrictive. BUG_ON is way too
> heavy for the case that the can interface probably received a can frame with
> malicious format. Further it can be used for DDOS attack since BUG_ON can lead
> to kernel panic. Hence we turn this to WARN_ON instead.
> 
> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> Signed-off-by: Luotao Fu <l.fu@pengutronix.de>
> ---
>  net/can/af_can.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index e733725..e6dcf4b 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -656,7 +656,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
>  		return 0;
>  	}
>  
> -	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).

When this BUG() triggers, someone provided a definitely broken *CAN* network
driver, and this needs to be fixed on that level. It is really not that
problematic to ensure proper CAN frames on driver level ... this sanity check
should not be needed to be performed by every single application.

Btw. the SocketCAN core ML and probably the netdev ML are the better places to
post CAN specific stuff the first time.

Regards,
Oliver

ps. If you have a use-case to make a DDOS via CAN bus - please let me know.
I'm always interested doing strange things on CAN ;-)

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

[Luotao Fu]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

Hi Oliver,

On Thu, Aug 06, 2009 at 06:48:23PM +0200, Oliver Hartkopp wrote:
> Luotao Fu wrote:
> > From: Michael Olbrich <m.olbrich@pengutronix.de>
> > 
> > Checking for can frame format in can_rcv() is too restrictive. BUG_ON is way too
> > heavy for the case that the can interface probably received a can frame with
> > malicious format. Further it can be used for DDOS attack since BUG_ON can lead
> > to kernel panic. Hence we turn this to WARN_ON instead.
> > 
> > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > Signed-off-by: Luotao Fu <l.fu@pengutronix.de>
> > ---
> >  net/can/af_can.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/can/af_can.c b/net/can/af_can.c
> > index e733725..e6dcf4b 100644
> > --- a/net/can/af_can.c
> > +++ b/net/can/af_can.c
> > @@ -656,7 +656,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
> >  		return 0;
> >  	}
> >  
> > -	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. Which means that we can this way
easily shoot down a system with a single can message. This might be a
serious security hole. Sanity check at this level should imho better
made in the can application. We shall not bring the systemstabity in
danger this way.

> 
> 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. This way the can driver on our side runs into the bug
though the driver itself is allright. The opposite needed to be fixed,
not our side.  Though we do suffer a system crash only because the
sender sends trash into the can network. This is imo quite bad.

cheers
Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Oliver Hartkopp]

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

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

[Luotao Fu]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

Hi Oliver (again ;-)),

On Thu, Aug 06, 2009 at 10:17:40PM +0200, Luotao Fu wrote:
> Hi Oliver,
> 
> On Thu, Aug 06, 2009 at 06:48:23PM +0200, Oliver Hartkopp wrote:
> > 
....
> > 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. This way the can driver on our side runs into the bug
> though the driver itself is allright. The opposite needed to be fixed,
> not our side.  Though we do suffer a system crash only because the
> sender sends trash into the can network. This is imo quite bad.
> 

/me answering myself
had a closer look again. Seemed you are right. The can driver should
have get the can_dlc right prior to passing the message a level higher.

cheers
Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Oliver Hartkopp]

Luotao Fu wrote:
> Hi Oliver (again ;-)),
> 
> On Thu, Aug 06, 2009 at 10:17:40PM +0200, Luotao Fu wrote:
>> Hi Oliver,
>>
>> On Thu, Aug 06, 2009 at 06:48:23PM +0200, Oliver Hartkopp wrote:
> ....
>>> 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. This way the can driver on our side runs into the bug
>> though the driver itself is allright. The opposite needed to be fixed,
>> not our side.  Though we do suffer a system crash only because the
>> sender sends trash into the can network. This is imo quite bad.
>>
> 
> /me answering myself
> had a closer look again. Seemed you are right. The can driver should
> have get the can_dlc right prior to passing the message a level higher.

Hi Luotao,

just one additional point i discovered after sending my last reply:

When can_dlc is not in the CAN conform value range from 0..8, you can really
get into trouble when accessing the CAN frames payload by using the can_dlc as
an index (a usual use-case):

        for (i = 0; i < frame.can_dlc; i++) {
                my_userdata[i] = frame.data[i];
                printf("%02X ", frame.data[i]);
        }

In this case you might access values outside the data[8] array.

And this is definitely a bad idea when you're writing to my_userdata[].

Best regards,
Oliver