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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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)