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

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

Posted by Kaixuan Li 5 days, 9 hours ago

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>
---
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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -754,8 +754,19 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
 	 */
 	if (skb->len != so->ll.mtu)
 		return;
 
+	/* skb->len does not separate the frame types on its own: a CAN XL
+	 * frame with cxl->len == 4 is CAN_MTU bytes, and canxl_frame.flags
+	 * aliases canfd_frame.len.
+	 */
+	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] can: isotp: check the frame type, not just the length

Posted by Oliver Hartkopp 5 days ago

Hello Kaixuan,

many thanks for your patch and your finding!

On 19.09.26 14:28, 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>
> ---
> 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.

In fact the skb->len length check is not enough since CAN XL has been 
introduced. A good catch!

> ---
>   net/can/isotp.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -754,8 +754,19 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
>   	 */
>   	if (skb->len != so->ll.mtu)
>   		return;
>   
> +	/* skb->len does not separate the frame types on its own: a CAN XL
> +	 * frame with cxl->len == 4 is CAN_MTU bytes, and canxl_frame.flags
> +	 * aliases canfd_frame.len.
> +	 */

No need to duplicate the documentation provided in the patch 
description. Just describe what is done here.

/* check for correct CAN CC/FD frame content */

should be enough for the below code.

> +	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)

You can add my

Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

in your v2 patch.

Many thanks and best regards,
Oliver