On 7/24/19 8:07 PM, Oliver Hartkopp wrote:
> On 24.07.19 15:36, Marc Kleine-Budde wrote:
>
>>> @@ -425,23 +502,22 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>> int max_len = nskb->len - offsetof(struct canfd_frame, data);
>>
>> I know, this is original code...but max_len can either be 8 (for CAN
>> frames) or 64 (for CAN-FD frames)? Because we always have full can_frame
>> or canfd_frame in the skb, right? I assume a lot more will break if the
>> len neither 8 nor 64.
>
> Yes. The code has been added recently for commit 0aaa81377c5a0 ("can:
> gw: ensure DLC boundaries after CAN frame modification") to check the
> data length after the CAN frame modification.
>
> And yes, we only have proper CAN (FD) frames in our skbs which have a
> special ethertype :-)
>
>>> /* dlc may have changed, make sure it fits to the CAN frame */
>>> - if (cf->len > max_len)
>>> - goto out_delete;
>>> + if (cf->len > max_len) {
>>> + /* delete frame due to misconfiguration */
>>> + gwj->deleted_frames++;
>>> + kfree_skb(nskb);
>>> + return;
>>> + }
>>>
>>> - /* check for checksum updates in classic CAN length only */
>>> - if (gwj->mod.csumfunc.crc8) {
>>> - if (cf->len > 8)
>>> - goto out_delete;
>>> + /* ensure a valid CAN (FD) frame data length */
>>> + cf->len = validate_len[cf->len];
>>
>> This looks strange to me, but I cannot say if I don't userstand this or
>> if there really is a potential problem:
>> - first you calculate max_len
>> - the cf->len > max_len is discarded
>
> Right. In this case the frame modification leads to a length value
> beyond 8/64 byte, which is then discarded.
>
>> - but then cf->len is "rounded up" via validate_len[].
>>
>> What's the purpose of the last step?
>
> Ha, good question :-)
>
> The reason for this results from the non-linear data length code for CAN
> FD data length.
>
> For CAN you can have 0, 1, 2, 3, 4, 5, 6, 7, 8
>
> For CAN FD it is 0, 1, 2, 3, 4, 5, 6, 7, 8, 12, 16, 20, 24, 32, 48, 64
>
> If the CAN FD length was 12 before the modification and the modification
> was to "set bit 0 in the length field" then you get 13.
ok
> But the length value of 13 is an illegal value for CAN FD length and can
> not been sent by a CAN FD controller.
ok, but that doesn't matter, because all CAN-FD drivers must convert
from struct canfd_frame::len to the dlc (== their register value)
anyways using the can_len2dlc() function.
> Therefore we need a round-up to get the next valid CAN FD length value
> (in our example it get's from 13 to 16).
I don't think this is needed, the user space might send such packets and
the drivers have to deal with then anyways.
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
