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

On 19.08.26 18:26, [email protected] wrote:
> 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?
> 

This is a common remark from Sashiko-bot when looking at this code.

No problem here.

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