On 8/9/19 7:29 PM, Oliver Hartkopp wrote:
>> Which race condition do you have in mind?
>
> E.g. you create a can-gw job that forwards the incoming CAN frame to 10
> different vcan's.
>
> These 10 skb's have been cloned which means their skb->data points to
> the exact same location. When now each vcan instance on a multicore
> system reads and writes the length info you have a classical race on
> that one memory location.
Thanks. I've totally missed the point that can-gw can forward CAN frames
to more than one interface.
>> For loopback ("can_put_echo_skb()"), I see a race condition in some CAN
>> drivers: They first do a can_put_echo_skb(), then keep using the skb to
>> write ->data into the hardware.
>>
>> If we modify the skb in can_put_echo_skb(), then the CAN driver will see
>> the sanitized struct canfd_frame::len. This means for dlc == 13, 14, 15
>> the driver will write more data into the hardware than needed. It
>> probably depends on the HW what the controller sends out if you have a
>> dlc == 14 (which means len up to 48 bytes) but only write 33 bytes into
>> the registers.
>
> Then you have uninitialized content from the CAN registers and not from
> the skb on the wire :-)
ACK.
>> While other drivers don't touch the skb after can_put_echo_skb().
>>
>> Can we modify the ->len in can_get_echo_skb() (or the __ variant) right
>> before pushing the skv into the networking stack? We have to look at the
>> data path for driver that cannot/don't use can_get_echo_skb() due to no
>> TX-complete interrupt.
>>
>> Looking at vcan, where's the race condition when modifying the skb in
>> vcan_tx()?
>
> As written above. Additionally the echo is mostly done in can_send() in
> af_can.c. And vcan_tx() is just a /dev/null from the packet flow
> perspective.
With can-gw, the skb hitting vcan may be cloned and thus changing the
data is not allowed. But what about the loopback path? Are there
usecases where a cloned skb used?
>>> I tend to sanitize the CAN FD length values when they are introduced
>>> into the system (CAN_RAW, CAN_BCM, CAN FD drivers*) and when they are
>>> modified (CAN_GW).
>>>
>>> * = already done
>>
>> You miss the datapath that directly injects packets into the networking
>> stack with the mechanism that tcpdump uses but the sending into the
>> kernel instead of receiving (I don't remember the exact name). This path
>> skips all checks in CAN_RAW. This is why we have the
>> "can_dropped_invalid_skb()" in all drivers.
>
> I don't know whether these tools can create ethertype CAN(FD) skbs which
> is mandatory for the CAN stack to process skbs. But if so, you are right.
You can inject arbitrary packets into the kernel.
>> So from my point of view it makes no sense to sanitize the len value in
>> gw, as the drivers convert from len to dlc anyways.
>>
>> What do I see when I attach a candump to the src and another one to the
>> dst interface? I suspenct:
>>
>> src) the unmodified canfd_frame with sanitized len, as the controller
>> knowns only the dlc.
>>
>> dst) the modified-by-gw canfd_frame, _after_ the loopback by the driver?
>> If there's neither sanitation in the gw not in the loopback, it's the
>> unsanitized len.
>>
>> When I have another application sending on the dst interface, the
>> candump receives the un-sanitized, as there's no sanitation in the loopback.
>>
>> For me the loopback is and vcan is the part to modify, as gw is just a
>> another source of unsanitized len information.
>>
>>> My idea was to fix things up when someone manipulates the CAN or CAN FD
>>> length field with can-gw.
>>
>> For CAN you don't have to do any sanitization, only check if the len is
>> <= 8.
>
> Mapping [0..8] to [0..8] is pointless but correct :-)
:p
>>> Or do you think I should just omit it and let the data flow as-is?
>>
>> ACK
>
> Ok, will send a v2 with a removed sanitized CAN FD length. The
> boundaries of the max length information (max 8 or 64) is tested anyway.
Tnx. It's in the linux-can-next-for-5.4-20190814 pull reqeust.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
