Re: CAN-XL frame to not CAN-XL enabled interface

On 31.07.26 15:28, Vincent Mailhol wrote:
> On 31/07/2026 at 14:55, Vincent Mailhol wrote:
>> On 31/07/2026 at 13:02, Marc Kleine-Budde wrote:
>>> Vincent,
>>>
>>> does the current code check if you send a CAN XL frame to a CAN XL
>>> enabled interface?
>>>
>>> On 31.07.2026 10:54:02, [email protected] wrote:
>>>> This is a pre-existing issue, but does this code properly handle CAN XL frames?
>>>>
>>>> If an ETH_P_CANXL frame is sent via AF_PACKET, can_dev_dropped_skb() currently
>>>> lacks a check to drop CAN XL frames for devices that don't support CAN XL.
>>>> When such a frame enters rkcanfd_start_xmit(), can_is_canfd_skb() returns
>>>> false, causing the driver to treat it as a Classic CAN frame.
>>>
>>>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
>>
>> The logic is for checking an ETH_P_CANXL frame is:
>>
>>    - When sending a packet (even with PF_PACKET) the only check which
>>      is performed is to check that the skb len is not above
>>      net_device->mtu. So, if the device is not CAN XL capable, anything
>>      about CANFD_MTU (72 bytes) is dropped.
>>
>>    - In the driver, can_dev_dropped_skb() calls
>>      can_dropped_invalid_skb() which then call can_is_canxl_skb() which
>>      validate that the skb length is within the range
>>
>>        [CANXL_HDR_SIZE + CANXL_MIN_DLEN, skb->len > CANXL_MTU]
>>
>>      that is to say 13 and 2060. But we already establish that the
>>      maximum length is 72 at this time.
>>
>> So, yes, it seems that an ETH_P_CANXL frame with a length between 13
>> and 72 will pass the can_dev_dropped_skb() check. And because
>> CANXL_XLF must be set at this point, the driver sees a can_frame->len
>> of at least 128 bytes (and up to 255).
>>
>> Until now, I was convinced in my mind that can_is_canxl_skb() rejected
>> frames with a length below CANXL_MIN_MTU which would have prevented
>> the issue. And I wrote the implementation under this false assumption
>> without double checking.
>>
>>
>> Here is a potential fix:
>>
>> ---8<---
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 6d0710d6f571..88dec0f91d71 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -152,6 +152,7 @@ static inline bool can_dev_in_xl_only_mode(struct can_priv *priv)
>>   /* drop skb if it does not contain a valid CAN frame for sending */
>>   static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *skb)
>>   {
>> +       const struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
>>          struct can_priv *priv = netdev_priv(dev);
>>          u32 silent_mode = priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY |
>>                                              CAN_CTRLMODE_RESTRICTED);
>> @@ -167,6 +168,11 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
>>                  goto invalid_skb;
>>          }
>>   
>> +       if (!(priv->ctrlmode & CAN_CTRLMODE_XL) && cxl->flags & CANXL_XLF) {
>> +               netdev_info_once(dev, "CAN XL is disabled, dropping skb\n");
>> +               goto invalid_skb;
>> +       }
>> +
>>          if (can_dev_in_xl_only_mode(priv) && !can_is_canxl_skb(skb)) {
>>                  netdev_info_once(dev,
>>                                   "Error signaling is disabled, dropping skb\n");
>> ---8<---
> 
> On second thought, maybe this is an even better fix:
> 
> ---8<---
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f8..9846989d4960 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -384,7 +384,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>                  break;
>   
>          case ETH_P_CANXL:
> -               if (!can_is_canxl_skb(skb))
> +               if (!can_cap_enabled(dev, CAN_CAP_XL) || !can_is_canxl_skb(skb))
>                          goto inval_skb;
>                  break;
> ---8<---

Yes! I really like the can_cap stuff ;-)

> 
>> The cxl->flags & CANXL_XLF doesn't catch everything by itself, but
>> once we know that this flag is off, can_is_canxl_skb() will reject the
>> frame later on. So this should be the minimum fix
>> (can_dev_dropped_skb() is an inline function in the hot path, so any
>> trick is welcome here, I think).
>>
>> Does this make sense?
> 
> 
> Yours sincerely,
> Vincent Mailhol
> 
>