Linux-Can Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
	Oliver Hartkopp <socketcan@hartkopp.net>,
	linux-can@vger.kernel.org
Cc: kernel@pengutronix.de
Subject: Re: [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames
Date: Tue, 20 Oct 2020 13:48:23 +0200
Message-ID: <116918a9-e09a-d1d6-f3b6-d6af4a9d0ac2@pengutronix.de> (raw)
In-Reply-To: <20201020113023.102360-1-mailhol.vincent@wanadoo.fr>

[-- Attachment #1.1: Type: text/plain, Size: 6949 bytes --]

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@wanadoo.fr>
>>>
>>> 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@wanadoo.fr>
>>> Link: https://lore.kernel.org/r/20201002154219.4887-4-mailhol.vincent@wanadoo.fr
>>> [mkl: renamed get_can_len() -> can_get_len()]
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>> ---
>>>   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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19 19:05 [RFC]: can 2020-10-19 Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 01/16] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 02/16] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 03/16] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames Marc Kleine-Budde
2020-10-19 20:35   ` Oliver Hartkopp
2020-10-20  6:35     ` Marc Kleine-Budde
2020-10-20 11:30       ` Vincent Mailhol
2020-10-20 11:48         ` Marc Kleine-Budde [this message]
2020-10-20 12:38         ` Oliver Hartkopp
2020-10-20 15:02           ` Marc Kleine-Budde
2020-10-20 16:07           ` Vincent Mailhol
2020-10-20 17:04             ` Oliver Hartkopp
2020-10-20 18:50               ` Marc Kleine-Budde
2020-10-21  0:52               ` Vincent Mailhol
2020-10-21  6:23                 ` Vincent MAILHOL
2020-10-21  7:11                   ` Joakim Zhang
2020-10-21  7:21                     ` Marc Kleine-Budde
2020-10-21  7:48                       ` Joakim Zhang
2020-10-21  9:21                   ` Oliver Hartkopp
2020-10-21  9:48                     ` Oliver Hartkopp
2020-10-21 11:55                     ` Vincent MAILHOL
2020-10-21 17:52                       ` Oliver Hartkopp
2020-10-22  3:30                         ` Vincent MAILHOL
2020-10-22  7:15                           ` Oliver Hartkopp
2020-10-22 12:23                             ` Vincent MAILHOL
2020-10-22 13:28                               ` Oliver Hartkopp
2020-10-22 15:46                                 ` Vincent MAILHOL
2020-10-22 17:06                                   ` Oliver Hartkopp
2020-10-23 10:36                                     ` Vincent MAILHOL
2020-10-23 16:47                                       ` Oliver Hartkopp
2020-10-24  5:25                                         ` Vincent MAILHOL
2020-10-24 11:31                                           ` Oliver Hartkopp
2020-10-19 19:05 ` [net-rfc 05/16] can: dev: __can_get_echo_skb(): fix the returned length of CAN frame Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 06/16] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 07/16] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 08/16] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 09/16] can: isotp: enable RX timeout handling in listen-only mode Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 10/16] can: ti_hecc: add missed clk_disable_unprepare() in error path Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 11/16] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 12/16] can: peak_usb: fix timestamp wrapping Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 13/16] can: peak_canfd: fix echo management when loopback is on Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 14/16] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 15/16] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 16/16] can: mcp251xfd: remove unneeded break Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=116918a9-e09a-d1d6-f3b6-d6af4a9d0ac2@pengutronix.de \
    --to=mkl@pengutronix.de \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    --cc=socketcan@hartkopp.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Can Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-can/0 linux-can/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-can linux-can/ https://lore.kernel.org/linux-can \
		linux-can@vger.kernel.org
	public-inbox-index linux-can

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-can


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git