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: <20090806201740.GA7067@pengutronix.de>
Date:	Thu, 6 Aug 2009 22:17:40 +0200
From:	Luotao Fu <l.fu@...gutronix.de>
To:	Oliver Hartkopp <oliver@...tkopp.net>
Cc:	Luotao Fu <l.fu@...gutronix.de>, 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

Hi Oliver,

On Thu, Aug 06, 2009 at 06:48:23PM +0200, Oliver Hartkopp wrote:
> Luotao Fu wrote:
> > From: Michael Olbrich <m.olbrich@...gutronix.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@...gutronix.de>
> > Signed-off-by: Luotao Fu <l.fu@...gutronix.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 |

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)