Re: [PATCH] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

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

Pre-existing issues:
- [High] Data modification of shared/cloned SKBs in `can_send()` causes payload corruption for concurrent users.
--

--- Patch [1]: [PATCH] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv() ---
commit 57320b2cc995d1852816aaacf79460f42c05858d
Author: Oliver Hartkopp <[email protected]>

can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

This commit updates the CAN subsystem to use can_get_ml_priv() for
identifying valid CAN devices instead of relying on ARPHRD_CAN. This
prevents issues with TUN/TAP devices that might overwrite their hardware
type without properly initializing the midlayer private data.

Cc: [email protected]
Cc: Oleksij Rempel <[email protected]>

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 7bc86b176b4de..ef435f22ac933 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -226,7 +226,7 @@ int can_send(struct sk_buff *skb, int loop)
>  		goto inval_skb;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but can modifying the SKB payload here cause
data corruption for concurrent receivers?

Earlier in can_send(), the SKB data is directly modified without checking if
it is shared:

	} else if (can_is_canfd_skb(skb)) {
		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

		skb->protocol = htons(ETH_P_CANFD);

		/* set CAN FD flag for CAN FD frames by default */
		cfd->flags |= CANFD_FDF;

When routing frames without data modifications, can_can_gw_rcv() creates
a cloned SKB using skb_clone() and passes it to can_send():

net/can/gw.c:can_can_gw_rcv() {
	...
	nskb = skb_clone(skb, GFP_ATOMIC);
	...
	can_send(nskb, gwj->limit_hops);
}

Since skb_clone() shares the underlying data buffer, wouldn't modifying
cfd->flags directly in can_send() corrupt the payload for other concurrent
users of the original SKB?

> -	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
> +	if (unlikely(!can_get_ml_priv(skb->dev))) {
>  		err = -EPERM;
>  		goto inval_skb;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1