On 10/20/20 1:30 PM, Vincent Mailhol wrote: >> On 19.10.20 21:05, Marc Kleine-Budde wrote: >>> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >>> >>> In classical CAN, the length of the data (i.e. CAN payload) is not >>> always equal to the DLC! If the frame is a Remote Transmission Request >>> (RTR), data length is always zero regardless of DLC value and else, if >>> the DLC is greater than 8, the length is 8. Contrary to common belief, >>> ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8 >>> for Classical Frames and specifies that those DLCs shall indicate that >>> the data field is 8 bytes long. >>> >>> Above facts are widely unknown and so many developpers uses the "len" >>> field of "struct canfd_frame" to get the length of classical CAN >>> frames: this is incorrect! >>> >>> This patch introduces function get_can_len() which can be used in >>> remediation. The function takes the SKB as an input in order to be >>> able to determine if the frame is classical or FD. >>> >>> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >>> Link: https://lore.kernel.org/r/20201002154219.4887-4-mailhol.vincent@xxxxxxxxxx >>> [mkl: renamed get_can_len() -> can_get_len()] >>> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> >>> --- >>> include/linux/can/dev.h | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h >>> index 41ff31795320..2bb132fc6d88 100644 >>> --- a/include/linux/can/dev.h >>> +++ b/include/linux/can/dev.h >>> @@ -192,6 +192,29 @@ u8 can_dlc2len(u8 can_dlc); >>> /* map the sanitized data length to an appropriate data length code */ >>> u8 can_len2dlc(u8 len); >>> >>> +/* >>> + * can_get_len(skb) - get the length of the CAN payload. >>> + * >>> + * In classical CAN, the length of the data (i.e. CAN payload) is not >>> + * always equal to the DLC! If the frame is a Remote Transmission >>> + * Request (RTR), data length is always zero regardless of DLC value >>> + * and else, if the DLC is greater than 8, the length is 8. Contrary >>> + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow >>> + * DLCs greater than 8 for Classical Frames and specifies that those >>> + * DLCs shall indicate that the data field is 8 bytes long. >>> + */ >>> +static inline u8 can_get_len(const struct sk_buff *skb) >>> +{ >>> + const struct canfd_frame *cf = (const struct canfd_frame *)skb->data; >>> + >>> + if (can_is_canfd_skb(skb)) >>> + return min_t(u8, cf->len, CANFD_MAX_DLEN); >>> + else if (cf->can_id & CAN_RTR_FLAG) >>> + return 0; >>> + else >>> + return min_t(u8, cf->len, CAN_MAX_DLEN); >>> +} >> >> The main idea behind this patch and patch 05/16 is to provide a correct >> statistic in the tx bytes, right? >> >> A simple test for the CAN_RTR_FLAG will do the job as all the length >> sanitizing is already done in the tx path by can_dropped_invalid_skb() >> in all known drivers right *before* the skb is stored in the echo skb array. >> >> IMO there's no need for a separate helper function. Maybe a macro which >> should have something with 'payload' in its name - to determine the tx >> byte statistics based on CAN_RTR_FLAG ... > > Actually, the main idea is not only to provide a valid length for the > tx statistics. > > First fact is that many of the drivers (if not all?) will have the > same issue for the rx statistics as well, so this helper function can > be beneficial in other locations as well but that is not yet the main > point. > > What bugs me the most, is that there is a global misunderstanding of > the definition of the Classical CAN frame's DLC in the kernel. > > ISO 11898-1:2015 tells us in section 8.4.2.4 "DLC field" that, I > quote: "[...] *This DLC shall consist of 4 bit*. The admissible > number of data bytes for Classical Frames shall range from 0 to > 8. DLCs in the range of 0 to 7 shall indicate data fields of length of > 0 byte to 7 byte. In Classical Frames, *all other DLCs shall indicate > that the data field is 8 bytes long*. [...]" > > So the DLC is a 4 bits value (meaning from 0 to 15) and all values > from 8 to 15 designate a data length of 8. > > The real idea is to have an ISO 11898-1 compliant function to retrieve > the length. > > That said, I hear your comment that the DLC is sanitized in > can_dropped_invalid_skb(). However, what bothers me is that this > sanitazation is done on false assumptions: Classical frames with DLC > greater than 8 and lower or equal to 15, which are valid by the > standard, are being discarded. > > To give you the full picture, I plan to send a RFC to fix this DLC > issue to allow Classical CAN frame with DLCs between 9 and 15 to be > sent and received in Socket CAN. This can_get_len() function is one > piece of it, however, because I also found the bad RTR length issue, I > thought to introduce this one in advance. Sorry for the lack of > context. > > The RFC I am thinking of will not be trivial. The main reason is that > the macro CAN_MAX_DLC which is incorrectly set to 8 instead of 15 is > exposed to the user land in include/linux/can.h. Modifying it would be > a no-go because we do not want to break user space. The direction > would be to have a socket option to set the maximum DLC to 15 on > demand and to keep it to 8 by default (this way, niche users who needs > this can do so but other people are not impacted). > > One question you might ask is: "why should be allow DLC greater than > 8?". One concrete use case is security testing. In order to check for > vulnerabilities, we want to send such frames for test coverage > (especially during fuzz testing). Aside of that I do not know other > use cases. I am not aware of any OEM that would use this in production > but I still want to push to have this option in the kernel just for > the security testing reason. > > I hope that you now understand the full idea behind this patch. If you > agree with my comments, please reconsider adding that patch. If I > failed to convince you and if you prefer to first see the full > picture, then I am OK to go with the simple test for the CAN_RTR_FLAG > as you suggested in your other patch and will come back to you later > on the MAX DLC topic when ready. > > Thanks for reading me until the end! Ok, I see your point. I'll keep Oliver's patch on linux-can/testing. If you want to improve the stack toward more 11898-1:2015 complience please make that a dedicated series. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: OpenPGP digital signature
