[PATCH v2] can: restore skb header initialisations in init_can_skb()

[PATCH v2] can: restore skb header initialisations in init_can_skb()

[zjamg]

Commit 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
removed the skb_reset_mac_header()/skb_reset_network_header()/
skb_reset_transport_header() calls from init_can_skb(). As a result, RX
skbs from alloc_can_skb() and friends again carry mac_header = 0xFFFF.
When such an skb reaches packet_rcv_spkt() (SOCK_PACKET), the push length
calculation overflows and triggers skb_under_panic -> kernel BUG -> full
machine panic.

The same issue was originally reported in 2014 on linux-can and fixed by
commit 969439016d2c ("can: add missing initialisations in CAN related
skbuffs"). packet_rcv_spkt() itself has never been hardened: only
packet_rcv() and tpacket_rcv() gained dev_has_header() checks in
commit d549699048b4 ("net/packet: fix packet receive on L3 devices
without visible hard header").

Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
Cc: stable@vger.kernel.org
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: zjamg <ndaugoing@gmail.com>
---
v2:
 - Remove in-code comment per Oliver's feedback.
 - Pick up Reviewed-by and Acked-by tags from Oliver Hartkopp.

 drivers/net/can/dev/skb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..14edec5afb57 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -210,6 +210,10 @@ static void init_can_skb(struct sk_buff *skb)
 {
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
 }
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
-- 
2.53.0

Re: [PATCH v2] can: restore skb header initialisations in init_can_skb()

[Quchaosheng]

Hello zjamg,

I reproduced the panic independently and verified that your patch alone
fixes it, so:

Tested-by: Quchaosheng <quchaosheng000406@163.com>

How I tested, in case it is useful for the maintainers.  The driver RX
path is required: vcan does not reproduce it, because can_send() resets
the headers itself on the way out.  I used slcan over a pty, opened a
SOCK_PACKET socket on the resulting can0, and pushed one frame in through
the line discipline.  Two kernels, same config, same initramfs, only your
patch differing.

Without the patch, v7.3.0-rc5 under QEMU:

  skbuff: skb_under_panic: text:ffffffffa0b28261 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
  kernel BUG at net/core/skbuff.c:214!
  RIP: 0010:skb_panic+0x50/0x60
  Call Trace:
   <TASK>
   skb_push+0x4d/0x60
   packet_rcv_spkt+0xe1/0x170
  Kernel panic - not syncing: Fatal exception in interrupt

  (the len/put values match the ones in your report)

With your v2 applied verbatim, the identical run prints

  RESULT: survived, no skb_under_panic

and the guest powers off normally.

One note that may be worth adding to the commit message if you respin: the
2015 fix you reference, 969439016d2c, is the second time this class of bug
was closed on the producer side.  packet_rcv_spkt() has still never been
hardened, which is why the same failure mode came back a third time when
9f10374bb024 dropped the calls again.  I sent a separate patch for that
receiver-side guard so the next regression of this kind degrades to a
dropped frame instead of a panic - no overlap with this one, and yours is
the one that fixes the actual regression.

Thanks for picking this up,
Quchaosheng

[PATCH v2] can: isotp: check the frame type, not just the length

[Kaixuan Li]

isotp_rcv() separates Classic CAN from CAN FD by skb->len alone:

	if (skb->len != so->ll.mtu)
		return;

	cf = (struct canfd_frame *)skb->data;

A CAN XL frame with cxl->len 4 is CAN_MTU bytes, so it passes, and is then
read as a canfd_frame whose len comes out of canxl_frame.flags: at least
0x80.

Of the paths that follow, only the flow control one uses that length
without bounding it first, so check_pad() walks to 255 over a 16-byte
frame and the caller reports EBADMSG on an unrelated socket.

bcm_rx_handler(), j1939_can_recv(), can_can_gw_rcv() and raw_rcv() check
the frame type here, and can_dropped_invalid_skb() switches on
skb->protocol on the transmit side. isotp_rcv() is the gap.

Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
v2: shorten the comment above the new check to say what it does; the
    reasoning stays in the description (Oliver Hartkopp). Add Oliver's
    Reviewed-by and Acked-by. No code change.

v1: https://lore.kernel.org/linux-can/20260919122852.1868961-1-kaixuanli0131@gmail.com/

Reproduced on v7.2.4 over vcan, one isotp socket per case bound rx 0x123
with RX_PADDING|CHK_PAD_DATA and rxpad_content 0xAA, a first frame in
flight, and one frame injected from a CAN_RAW socket.

	case                            stock     patched
	A CAN XL, cxl->len 4, flags ff  EBADMSG   none
	B Classic FC, padded 0xAA       none      none
	C Classic FC, padded 0x00       EBADMSG   EBADMSG
	D as A, with CHK_PAD_LEN on     EBADMSG   none

C bounds the impact: a malformed Classic FC frame from any sender on the
bus gives the same EBADMSG, so nothing becomes reachable that was not
already. D differs only in which branch of check_pad() returns.

No memory safety issue. KASAN was on for all eight runs and reported
nothing.
---
 net/can/isotp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -754,8 +754,16 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
 	 */
 	if (skb->len != so->ll.mtu)
 		return;
 
+	/* check for correct CAN CC/FD frame content */
+	if (so->ll.mtu == CAN_MTU) {
+		if (!can_is_can_skb(skb))
+			return;
+	} else if (!can_is_canfd_skb(skb)) {
+		return;
+	}
+
 	cf = (struct canfd_frame *)skb->data;
 
 	/* if enabled: check reception of my configured extended address */
 	if (ae && cf->data[0] != so->opt.rx_ext_address)

Re: [PATCH v2] can: isotp: check the frame type, not just the length

[Oliver Hartkopp]

On 20.09.26 05:56, Kaixuan Li wrote:
> isotp_rcv() separates Classic CAN from CAN FD by skb->len alone:
> 
> 	if (skb->len != so->ll.mtu)
> 		return;
> 
> 	cf = (struct canfd_frame *)skb->data;
> 
> A CAN XL frame with cxl->len 4 is CAN_MTU bytes, so it passes, and is then
> read as a canfd_frame whose len comes out of canxl_frame.flags: at least
> 0x80.
> 
> Of the paths that follow, only the flow control one uses that length
> without bounding it first, so check_pad() walks to 255 over a 16-byte
> frame and the caller reports EBADMSG on an unrelated socket.
> 
> bcm_rx_handler(), j1939_can_recv(), can_can_gw_rcv() and raw_rcv() check
> the frame type here, and can_dropped_invalid_skb() switches on
> skb->protocol on the transmit side. isotp_rcv() is the gap.
> 
> Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
> Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
> Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> v2: shorten the comment above the new check to say what it does; the
>      reasoning stays in the description (Oliver Hartkopp). Add Oliver's
>      Reviewed-by and Acked-by. No code change.
> 

Thanks for the fast update!

Awaiting upstream.

Best regards,
Oliver

> v1: https://lore.kernel.org/linux-can/20260919122852.1868961-1-kaixuanli0131@gmail.com/
> 
> Reproduced on v7.2.4 over vcan, one isotp socket per case bound rx 0x123
> with RX_PADDING|CHK_PAD_DATA and rxpad_content 0xAA, a first frame in
> flight, and one frame injected from a CAN_RAW socket.
> 
> 	case                            stock     patched
> 	A CAN XL, cxl->len 4, flags ff  EBADMSG   none
> 	B Classic FC, padded 0xAA       none      none
> 	C Classic FC, padded 0x00       EBADMSG   EBADMSG
> 	D as A, with CHK_PAD_LEN on     EBADMSG   none
> 
> C bounds the impact: a malformed Classic FC frame from any sender on the
> bus gives the same EBADMSG, so nothing becomes reachable that was not
> already. D differs only in which branch of check_pad() returns.
> 
> No memory safety issue. KASAN was on for all eight runs and reported
> nothing.
> ---
>   net/can/isotp.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -754,8 +754,16 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
>   	 */
>   	if (skb->len != so->ll.mtu)
>   		return;
>   
> +	/* check for correct CAN CC/FD frame content */
> +	if (so->ll.mtu == CAN_MTU) {
> +		if (!can_is_can_skb(skb))
> +			return;
> +	} else if (!can_is_canfd_skb(skb)) {
> +		return;
> +	}
> +
>   	cf = (struct canfd_frame *)skb->data;
>   
>   	/* if enabled: check reception of my configured extended address */
>   	if (ae && cf->data[0] != so->opt.rx_ext_address)

Re: [PATCH v2] can: isotp: check the frame type, not just the length

[Quchaosheng]

Hello Kaixuan,

I hit the same collision from a different direction and independently
arrived at the same fix, so:

Reviewed-by: Quchaosheng <quchaosheng000406@163.com>

Two things I checked that may be worth having on the record, since they
are the questions this patch is likely to attract.

First, that isotp really is the only gap, so it does not need to grow into
a series.  I went through the other places that consume a CAN skb without
looking at its type.  bcm_rx_handler() and can_can_gw_rcv() do gate on
can_is_can_skb() / can_is_canfd_skb(), and those two helpers carry a
second condition on the frame length field
(include/linux/can/skb.h:93 and :101).  On a CAN XL frame that field is
cxl->flags, and CANXL_XLF alone is 0x80, which is already past
CAN_MAX_DLEN and CANFD_MAX_DLEN, so an XL frame is rejected there before
its length is ever used.  So the collision is specific to isotp.

Second, nothing on the transmit side can produce it.  isotp always builds
its frames at so->ll.mtu and validates ll.mtu against CAN_MTU / CANFD_MTU
in the setsockopt path, so the frame that passes the old length-only test
can only come off the wire.

Your A/B table is what convinced me, by the way - case C in particular is
the right control, since it shows the EBADMSG already existed for a
malformed Classic FC from any sender.  Nothing becomes reachable that was
not reachable before.

Thanks for fixing this,
Quchaosheng