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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 27.08.26 14:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Modifying cloned SKB data without unsharing causes data races and potential silent data corruption in shared CAN FD buffers.
> --
> 
> commit cd014a29d1194dd4eff54897a5d2062f2ec32a0c
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
>      
> This patch converts unreliable ARPHRD_CAN type checks to use the robust
> can_get_ml_priv() function. This prevents TUN/TAP devices from spoofing
> their hardware type to bypass CAN device validation.
> 
> Link: https://patch.msgid.link/20260819160822.8256-1-socketcan@hartkopp.net
> 
>> 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;
>>   	}
>>   
>> -	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
>> +	if (unlikely(!can_get_ml_priv(skb->dev))) {
> 
> [Severity: High]
> This is a pre-existing issue, but while reviewing the changes in can_send(),
> I noticed a potential data race when handling cloned SKBs.
> 
> When the CAN gateway forwards a frame without modification functions,
> can_can_gw_rcv() forwards a cloned SKB:
> 
> net/can/gw.c:can_can_gw_rcv() {
>      ...
>      nskb = skb_clone(skb, GFP_ATOMIC);
>      ...
>      can_send(nskb, gwj->limit_hops);
> }
> 
> Then in can_send() just above this diff hunk, the SKB data area is modified
> unconditionally for CAN FD frames:
> 
> net/can/af_can.c:can_send() {
>      ...
>      } else if (can_is_canfd_skb(skb)) {
>          struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>          ...
>          /* set CAN FD flag for CAN FD frames by default */
>          cfd->flags |= CANFD_FDF;
>      }
>      ...
> }
> 
> Since skb_clone() shares the data area with the original SKB, does modifying
> cfd->flags without unsharing the SKB first cause a data race?
> 
> On architectures without atomic byte-store instructions, this performs a
> word-sized read-modify-write operation. If the original SKB is concurrently
> processed by other receivers, could this silently corrupt adjacent fields?
> Given that an unprivileged user with CAP_NET_ADMIN in a user namespace can
> set up a virtual CAN interface and trigger this path, is this safe?

Yes. Sashiko groundhog day ...

https://lore.kernel.org/linux-can/ac22b59e-129b-4883-b9c6-da5e69e4078d@hartkopp.net/


> 
>>   		err = -EPERM;
>>   		goto inval_skb;
>>   	}
> 
> [ ... ]
>