netdev - 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: <4A7C117E.5010005@hartkopp.net>
Date:	Fri, 07 Aug 2009 13:35:26 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Rémi Denis-Courmont 
	<remi.denis-courmont@...ia.com>, Luotao Fu <l.fu@...gutronix.de>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"socketcan-users@...ts.berlios.de" <socketcan-users@...ts.berlios.de>,
	Michael Olbrich <m.olbrich@...gutronix.de>
Subject: Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less
 restrictive

Rémi Denis-Courmont wrote:
> Moving to netdev....
> 
> On Thursday 06 August 2009 19:48:23 ext Oliver Hartkopp wrote:
>> 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.
> 
> AFAIK, the TUN driver can inject layer-2 frames of any type, any size and any 
> content from userspace into the packet type handler. Sure enough, you need 
> CAP_NET_ADMIN and r/w access to /dev/net/tun but is it sufficient to bring the 
> system down?
> 

The complete code section currently looks like this:


static int can_rcv(struct sk_buff *skb, struct net_device *dev,
                   struct packet_type *pt, struct net_device *orig_dev)
{
        struct dev_rcv_lists *d;
        struct can_frame *cf = (struct can_frame *)skb->data;
        int matches;

        if (dev->type != ARPHRD_CAN || !net_eq(dev_net(dev), &init_net)) {
                kfree_skb(skb);
                return 0;
        }

        BUG_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8);

(..)

So you would need to have an originating interface with ARPHRD_CAN ...

Do you think, it's still possible with the TUN driver?


@Luotao: I talked to Urs and we discussed to prepare a patch that only creates
a warning and drops the skb afterwards, as the problem is not critical for a
proper ongoing kernel operation. I think, that was you original intention:

       if (!net_eq(dev_net(dev), &init_net) ||
           WARN_ON(dev->type != ARPHRD_CAN) ||
           WARN_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8)) {
               kfree_skb(skb);
               return NET_RX_BAD;
       }

Would this be ok for you?

Regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html