* CAN-XL frame to not CAN-XL enabled interface @ 2026-07-31 11:02 Marc Kleine-Budde 2026-07-31 12:55 ` Vincent Mailhol 0 siblings, 1 reply; 4+ messages in thread From: Marc Kleine-Budde @ 2026-07-31 11:02 UTC (permalink / raw) To: Vincent Mailhol; +Cc: Oleksij Rempel, linux-can, Oliver Hartkopp, sashiko-bot [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: CAN-XL frame to not CAN-XL enabled interface 2026-07-31 11:02 CAN-XL frame to not CAN-XL enabled interface Marc Kleine-Budde @ 2026-07-31 12:55 ` Vincent Mailhol 2026-07-31 13:28 ` Vincent Mailhol 0 siblings, 1 reply; 4+ messages in thread From: Vincent Mailhol @ 2026-07-31 12:55 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: Oleksij Rempel, linux-can, Oliver Hartkopp, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: CAN-XL frame to not CAN-XL enabled interface 2026-07-31 12:55 ` Vincent Mailhol @ 2026-07-31 13:28 ` Vincent Mailhol 2026-07-31 13:42 ` Oliver Hartkopp 0 siblings, 1 reply; 4+ messages in thread From: Vincent Mailhol @ 2026-07-31 13:28 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: Oleksij Rempel, linux-can, Oliver Hartkopp, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: CAN-XL frame to not CAN-XL enabled interface 2026-07-31 13:28 ` Vincent Mailhol @ 2026-07-31 13:42 ` Oliver Hartkopp 0 siblings, 0 replies; 4+ messages in thread From: Oliver Hartkopp @ 2026-07-31 13:42 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: Oleksij Rempel, linux-can, sashiko-bot 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-31 13:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-31 11:02 CAN-XL frame to not CAN-XL enabled interface Marc Kleine-Budde 2026-07-31 12:55 ` Vincent Mailhol 2026-07-31 13:28 ` Vincent Mailhol 2026-07-31 13:42 ` Oliver Hartkopp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox