[PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[Vincent Mailhol]

Sending a PF_PACKET bypasses the CAN framework logic and can directly
reach a CAN driver's xmit() function. The PF_PACKET framework only
checks that skb->len does not exceed the net_device MTU.

For a CAN device that is not CAN XL capable, anything above CANFD_MTU
(72 bytes) is therefore dropped before it reaches the driver. However,
CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in
the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to
2060 bytes.

As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes
can pass both the MTU and the can_dropped_invalid_skb() checks.

A driver that does not support CAN XL will interpret canxl_frame->flags
as a length because of the overlap with can_frame->len. And because
CANXL_XLF is set, the resulting length is between 128 and 255. For
drivers that do not check can_frame->len before copying can_frame->data,
as most drivers do, this results in a buffer overflow of up to 247
bytes.

Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL
capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs.

Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1
Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Cc: stable@vger.kernel.org
---
 drivers/net/can/dev/skb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..4f7a189de265 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
  */
 
+#include <linux/can/can-ml.h>
 #include <linux/can/dev.h>
 #include <linux/module.h>
 #include <net/can.h>
@@ -384,7 +385,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;
 

---
base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4
change-id: 20260731-drop_canxl_frames-189872010abc

Best regards,
--  
Vincent Mailhol <mailhol@kernel.org>

Re: [PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `can_skb_init_valid()` directly modifies `skb->data` without checking if the SKB is cloned or shared, violating SKB shared buffer rules and causing data corruption.
--

--- Patch [1]: [PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices ---
commit 4602c4bbb604b601116708346040725e0603f25d
Author: Vincent Mailhol <mailhol@kernel.org>

can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

Sending a PF_PACKET bypasses the CAN framework logic and can directly
reach a CAN driver's xmit() function. The PF_PACKET framework only
checks that skb->len does not exceed the net_device MTU.

For a CAN device that is not CAN XL capable, anything above CANFD_MTU
(72 bytes) is therefore dropped before it reaches the driver. However,
CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in
the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to
2060 bytes.

As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes
can pass both the MTU and the can_dropped_invalid_skb() checks.

A driver that does not support CAN XL will interpret canxl_frame->flags
as a length because of the overlap with can_frame->len. And because
CANXL_XLF is set, the resulting length is between 128 and 255. For
drivers that do not check can_frame->len before copying can_frame->data,
as most drivers do, this results in a buffer overflow of up to 247
bytes.

Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL
capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs.

Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1
Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f80..4f7a189de265d 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c

[ ... ]

> @@ -384,7 +385,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;

[Severity: High]
This isn't a bug introduced by this patch, but looking at can_skb_init_valid()
which is called right after this switch statement:

drivers/net/can/dev/skb.c:can_skb_init_valid() {
    ...
		/* set CANFD_FDF flag for CAN FD frames */
		if (can_is_canfd_skb(skb)) {
			struct canfd_frame *cfd;

			cfd = (struct canfd_frame *)skb->data;
			cfd->flags |= CANFD_FDF;
		}
    ...
}

Can this modification of skb->data happen without ensuring exclusive ownership
via skb_unshare() or skb_cow_head()?

If a local user constructs a linear CANFD packet via AF_PACKET, which is then
cloned (e.g., via TC mirred or BPF redirect) and queued to a CAN interface, it
seems can_skb_init_valid() will directly modify cfd->flags in the shared data
buffer.

Since the network core's validate_xmit_skb() does not unclone an already-linear
SKB, could this corrupt other users of the same buffer data and lead to silent
data corruption in unrelated code paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-drop_canxl_frames-v1-1-7387b70353b3@kernel.org?part=1

Re: [PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[Oliver Hartkopp]

On 31.07.26 18:45, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] `can_skb_init_valid()` directly modifies `skb->data` without checking if the SKB is cloned or shared, violating SKB shared buffer rules and causing data corruption.
> --
> 
> --- Patch [1]: [PATCH] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices ---
> commit 4602c4bbb604b601116708346040725e0603f25d
> Author: Vincent Mailhol <mailhol@kernel.org>
> 
> can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices
> 
> Sending a PF_PACKET bypasses the CAN framework logic and can directly
> reach a CAN driver's xmit() function. The PF_PACKET framework only
> checks that skb->len does not exceed the net_device MTU.
> 
> For a CAN device that is not CAN XL capable, anything above CANFD_MTU
> (72 bytes) is therefore dropped before it reaches the driver. However,
> CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in
> the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to
> 2060 bytes.
> 
> As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes
> can pass both the MTU and the can_dropped_invalid_skb() checks.
> 
> A driver that does not support CAN XL will interpret canxl_frame->flags
> as a length because of the overlap with can_frame->len. And because
> CANXL_XLF is set, the resulting length is between 128 and 255. For
> drivers that do not check can_frame->len before copying can_frame->data,
> as most drivers do, this results in a buffer overflow of up to 247
> bytes.
> 
> Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL
> capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs.
> 
> Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1
> Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
> 
>> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
>> index 95fcdc1026f80..4f7a189de265d 100644
>> --- a/drivers/net/can/dev/skb.c
>> +++ b/drivers/net/can/dev/skb.c
> 
> [ ... ]
> 
>> @@ -384,7 +385,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;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but looking at can_skb_init_valid()
> which is called right after this switch statement:
> 
> drivers/net/can/dev/skb.c:can_skb_init_valid() {
>      ...
> 		/* set CANFD_FDF flag for CAN FD frames */
> 		if (can_is_canfd_skb(skb)) {
> 			struct canfd_frame *cfd;
> 
> 			cfd = (struct canfd_frame *)skb->data;
> 			cfd->flags |= CANFD_FDF;
> 		}
>      ...
> }
> 
> Can this modification of skb->data happen without ensuring exclusive ownership
> via skb_unshare() or skb_cow_head()?
> 
> If a local user constructs a linear CANFD packet via AF_PACKET, which is then
> cloned (e.g., via TC mirred or BPF redirect) and queued to a CAN interface, it
> seems can_skb_init_valid() will directly modify cfd->flags in the shared data
> buffer.
> 
> Since the network core's validate_xmit_skb() does not unclone an already-linear
> SKB, could this corrupt other users of the same buffer data and lead to silent
> data corruption in unrelated code paths?
> 

The finding about can_skb_init_valid() modifying skb->data on a 
potentially shared buffer is not relevant for this (correct) patch.

The FDF-flag write in can_skb_init_valid() only matters for PF_PACKET 
use: PF_CAN allocators already set CANFD_FDF. The write exists to 
normalize frames for PF_PACKET observers (tcpdump/Wireshark) so they can 
distinguish classic CAN from CAN FD at the netdev level, and to 
compensate for PF_PACKET senders that omit the bit. That normalization 
is functionally required.

The shared-buffer concern only becomes realistic through a specific 
chain: a PF_PACKET sender injects a CANFD-sized frame, the CAN driver's 
loopback puts an echo skb back into can_rcv(), and cgw (without 
modfuncs) clones it for forwarding to another interface. Only on that 
second can_skb_init_valid() call is the skb cloned. By then the bit was 
already set on the exclusive skb during the initial xmit path, so the 
flags |= CANFD_FDF write is strictly idempotent for any parallel 
consumer of the shared buffer — no memory-safety or 
information-disclosure consequence. It's a formal violation of the skb 
sharing rules without an observable effect, so the current code can stay 
as-is.

Best regards,
Oliver