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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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