CAN-XL frame to not CAN-XL enabled interface

CAN-XL frame to not CAN-XL enabled interface

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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, sashiko-bot@kernel.org 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/20260731-master-v5-0-5b27029dee20@qq.com?part=1

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Vincent Mailhol]

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, sashiko-bot@kernel.org 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/20260731-master-v5-0-5b27029dee20@qq.com?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<---

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

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

[Vincent Mailhol]

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, sashiko-bot@kernel.org 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/20260731-master-v5-0-5b27029dee20@qq.com?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<---

> 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

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

[Oliver Hartkopp]

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, sashiko-bot@kernel.org 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/20260731-master-v5-0-5b27029dee20@qq.com?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
> 
>