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

[Vincent Mailhol <mailhol@kernel.org>]

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