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

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

[Rémi Denis-Courmont]

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?

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

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

[Oliver Hartkopp]

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

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

[Luotao Fu]

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

Hi Oliver,

On Fri, Aug 07, 2009 at 01:35:26PM +0200, Oliver Hartkopp wrote:
> R?mi Denis-Courmont wrote:
....
> 
> @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?

I'm absolutely fine with this.

thx
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

[Rémi Denis-Courmont]

On Friday 07 August 2009 14:35:26 ext Oliver Hartkopp wrote:
> So you would need to have an originating interface with ARPHRD_CAN ...
>
> Do you think, it's still possible with the TUN driver?

Hmm, actually no... TUN can create packets with any type (ETH_P_*), but 
devices always have ARPHRD_NONE type.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki