On 8/9/19 12:27 PM, Oliver Hartkopp wrote: >>> In fact the only point where can_len2dlc() is active is when the CAN >>> frame is sent with a real CAN (FD) controller. >>> >>> Maybe it would make sense to create a special flag for the can-gw >>> modifications like CGW_FLAGS_CAN_FD_SANITIZE_LEN. >> >> My point is, it makes no sense to do any CAN-FD related len sanitisation >> in the can-gw, as the user will not see any strange length. >> >>> So we could point out that the CAN FD length information is manipulated >>> and that we offer to round the values or not. >> >> What I suggest instead is to do len sanitisation in the vcan driver, so >> that the problem I illustrated above will be fixed. >> >> Another path we have to think about is the loop back of CAN-FD frames, I >> think they are not sanitised as well. > > The problem in loopback and vcan is similar. > > But the issues that rises up when sanitizing the FD length value in > loopback/vcan/af_can -> we start to fiddle inside the skb data. > > When we do so, we need to skb_copy() the skb instead of working on > clones to prevent race conditions inside the data, see: > > https://elixir.bootlin.com/linux/v5.2.6/source/net/can/gw.c#L387 > > skb_copy would have a performance impact and it would trigger a big > rewriting of the current code. Don't know if it's worth that. Which race condition do you have in mind? 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. 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()? > 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. 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. > Or do you think I should just omit it and let the data flow as-is? ACK regards, 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
