Currently, the CAN FD skb validation logic is based on the MTU: the
interface is deemed FD capable if and only if its MTU is greater or
equal to CANFD_MTU.
This logic is showing its limit with the introduction of CAN XL. For
example, consider the two scenarios below:
1. An interface configured with CAN FD on and CAN XL on
2. An interface configured with CAN FD off and CAN XL on
In those two scenarios, the interfaces would have the same MTU:
CANXL_MTU
making it impossible to differentiate which one has CAN FD turned on
and which one has it off.
Because of the limitation, the only non-UAPI-breaking workaround is to
do the check at the device level using the can_priv->ctrlmode flags.
Unfortunately, the virtual interfaces (vcan, vxcan), which do not have
a can_priv, are left behind.
Add a check on the CAN_CTRLMODE_FD flag in can_dev_dropped_skb() and
drop FD frames whenever the feature is turned off.
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:
RFC -> v1:
- add an netdev_info_once() message.
- this was sent as a standalone patch for discussion, it is now
integrated in the CAN XL series.
Link: https://lore.kernel.org/linux-can/20250907080504.598419-2-mailhol@kernel.org/
---
include/linux/can/dev.h | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 0fe8f80f223e..d59b283c981a 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -103,12 +103,20 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
netdev_info_once(dev,
"interface in listen only mode, dropping skb\n");
- kfree_skb(skb);
- dev->stats.tx_dropped++;
- return true;
+ goto invalid_skb;
+ }
+
+ if (!(priv->ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
+ netdev_info_once(dev, "CAN FD is disabled, dropping skb\n");
+ goto invalid_skb;
}
return can_dropped_invalid_skb(dev, skb);
+
+invalid_skb:
+ kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return true;
}
void can_setup(struct net_device *dev);
--
2.49.1On 13.10.2025 20:01:23, Vincent Mailhol wrote: > Currently, the CAN FD skb validation logic is based on the MTU: the > interface is deemed FD capable if and only if its MTU is greater or > equal to CANFD_MTU. > > This logic is showing its limit with the introduction of CAN XL. For > example, consider the two scenarios below: > > 1. An interface configured with CAN FD on and CAN XL on > > 2. An interface configured with CAN FD off and CAN XL on > > In those two scenarios, the interfaces would have the same MTU: > > CANXL_MTU > > making it impossible to differentiate which one has CAN FD turned on > and which one has it off. > > Because of the limitation, the only non-UAPI-breaking workaround is to > do the check at the device level using the can_priv->ctrlmode flags. > Unfortunately, the virtual interfaces (vcan, vxcan), which do not have > a can_priv, are left behind. > > Add a check on the CAN_CTRLMODE_FD flag in can_dev_dropped_skb() and > drop FD frames whenever the feature is turned off. > > Signed-off-by: Vincent Mailhol <mailhol@kernel.org> What about merging both can_dev_dropped_skb() an can_dropped_invalid_skb() in the skb.c, so that there is no stub in the header file anymore. Someone (i.e. me) used can_dropped_invalid_skb() in a driver, that means the check for CAN_CTRLMODE_LISTENONLY is missing :/ (I'll send a fix). 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 |
On 17.10.2025 10:28:38, Marc Kleine-Budde wrote:
> On 13.10.2025 20:01:23, Vincent Mailhol wrote:
> > Currently, the CAN FD skb validation logic is based on the MTU: the
> > interface is deemed FD capable if and only if its MTU is greater or
> > equal to CANFD_MTU.
> >
> > This logic is showing its limit with the introduction of CAN XL. For
> > example, consider the two scenarios below:
> >
> > 1. An interface configured with CAN FD on and CAN XL on
> >
> > 2. An interface configured with CAN FD off and CAN XL on
> >
> > In those two scenarios, the interfaces would have the same MTU:
> >
> > CANXL_MTU
> >
> > making it impossible to differentiate which one has CAN FD turned on
> > and which one has it off.
> >
> > Because of the limitation, the only non-UAPI-breaking workaround is to
> > do the check at the device level using the can_priv->ctrlmode flags.
> > Unfortunately, the virtual interfaces (vcan, vxcan), which do not have
> > a can_priv, are left behind.
> >
> > Add a check on the CAN_CTRLMODE_FD flag in can_dev_dropped_skb() and
> > drop FD frames whenever the feature is turned off.
> >
> > Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
>
> What about merging both can_dev_dropped_skb() an
> can_dropped_invalid_skb() in the skb.c, so that there is no stub in the
> header file anymore.
Ouch! Don't do this. We still need can_dropped_invalid_skb() for virtual
interfaces. See commit ae64438be192 ("can: dev: fix skb drop check").
But then I'm asking: Why is there the difference between virtual and
non-virtual interface in the first place? Can we get rid of it?
> Someone (i.e. me) used can_dropped_invalid_skb() in a driver, that means
> the check for CAN_CTRLMODE_LISTENONLY is missing :/ (I'll send a fix).
These drivers need this fix:
| drivers/net/can/bxcan.c:845: if (can_dropped_invalid_skb(ndev, skb))
| drivers/net/can/esd/esdacc.c:257: if (can_dropped_invalid_skb(netdev, skb))
| drivers/net/can/rockchip/rockchip_canfd-tx.c:75: if (can_dropped_invalid_skb(ndev, skb))
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 |
On 17/10/2025 at 22:27, Marc Kleine-Budde wrote:
> On 17.10.2025 10:28:38, Marc Kleine-Budde wrote:
>> On 13.10.2025 20:01:23, Vincent Mailhol wrote:
>>> Currently, the CAN FD skb validation logic is based on the MTU: the
>>> interface is deemed FD capable if and only if its MTU is greater or
>>> equal to CANFD_MTU.
>>>
>>> This logic is showing its limit with the introduction of CAN XL. For
>>> example, consider the two scenarios below:
>>>
>>> 1. An interface configured with CAN FD on and CAN XL on
>>>
>>> 2. An interface configured with CAN FD off and CAN XL on
>>>
>>> In those two scenarios, the interfaces would have the same MTU:
>>>
>>> CANXL_MTU
>>>
>>> making it impossible to differentiate which one has CAN FD turned on
>>> and which one has it off.
>>>
>>> Because of the limitation, the only non-UAPI-breaking workaround is to
>>> do the check at the device level using the can_priv->ctrlmode flags.
>>> Unfortunately, the virtual interfaces (vcan, vxcan), which do not have
>>> a can_priv, are left behind.
>>>
>>> Add a check on the CAN_CTRLMODE_FD flag in can_dev_dropped_skb() and
>>> drop FD frames whenever the feature is turned off.
>>>
>>> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
>>
>> What about merging both can_dev_dropped_skb() an
>> can_dropped_invalid_skb() in the skb.c, so that there is no stub in the
>> header file anymore.
>
> Ouch! Don't do this. We still need can_dropped_invalid_skb() for virtual
> interfaces. See commit ae64438be192 ("can: dev: fix skb drop check").
Exactly!
> But then I'm asking: Why is there the difference between virtual and
> non-virtual interface in the first place? Can we get rid of it?
The fact is that:
- We need a function for the physical interfaces to check the
CAN_CTRLMODE_LISTENONLY, i.e. with an access to struct can_priv.
- We need a similar function but which work for the virtual
interfaces which do not have a can_priv member.
So unless we do a major code refactor so that the virtual interfaces,
I do not see how we could get rid of it.
>> Someone (i.e. me) used can_dropped_invalid_skb() in a driver, that means
>> the check for CAN_CTRLMODE_LISTENONLY is missing :/ (I'll send a fix).
At least, this does not seem like a security breach like it was the
case for the missing net_device_ops->ndo_change_mtu().
> These drivers need this fix:
>
> | drivers/net/can/bxcan.c:845: if (can_dropped_invalid_skb(ndev, skb))
> | drivers/net/can/esd/esdacc.c:257: if (can_dropped_invalid_skb(netdev, skb))
> | drivers/net/can/rockchip/rockchip_canfd-tx.c:75: if (can_dropped_invalid_skb(ndev, skb))
Yeah, I think that this is a pitfall, but at the same time, I do not
see how to get rid of this can_dev_dropped_skb() and
can_dropped_invalid_skb() pair. The best I can think of it to be
careful on this problem doing the code review now that we are aware
about it.
Yours sincerely,
Vincent Mailhol
On 18.10.2025 00:30:45, Vincent Mailhol wrote:
> >> What about merging both can_dev_dropped_skb() an
> >> can_dropped_invalid_skb() in the skb.c, so that there is no stub in the
> >> header file anymore.
> >
> > Ouch! Don't do this. We still need can_dropped_invalid_skb() for virtual
> > interfaces. See commit ae64438be192 ("can: dev: fix skb drop check").
>
> Exactly!
>
> > But then I'm asking: Why is there the difference between virtual and
> > non-virtual interface in the first place? Can we get rid of it?
>
> The fact is that:
>
> - We need a function for the physical interfaces to check the
> CAN_CTRLMODE_LISTENONLY, i.e. with an access to struct can_priv.
>
> - We need a similar function but which work for the virtual
> interfaces which do not have a can_priv member.
>
> So unless we do a major code refactor so that the virtual interfaces,
> I do not see how we could get rid of it.
Ack...There are more interesting/important tasks.
> >> Someone (i.e. me) used can_dropped_invalid_skb() in a driver, that means
> >> the check for CAN_CTRLMODE_LISTENONLY is missing :/ (I'll send a fix).
>
> At least, this does not seem like a security breach like it was the
> case for the missing net_device_ops->ndo_change_mtu().
>
> > These drivers need this fix:
> >
> > | drivers/net/can/bxcan.c:845: if (can_dropped_invalid_skb(ndev, skb))
> > | drivers/net/can/esd/esdacc.c:257: if (can_dropped_invalid_skb(netdev, skb))
> > | drivers/net/can/rockchip/rockchip_canfd-tx.c:75: if (can_dropped_invalid_skb(ndev, skb))
>
> Yeah, I think that this is a pitfall, but at the same time, I do not
> see how to get rid of this can_dev_dropped_skb() and
> can_dropped_invalid_skb() pair. The best I can think of it to be
> careful on this problem doing the code review now that we are aware
> about it.
Ack.
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 |
© 2016 - 2026 Red Hat, Inc.