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

[sashiko-bot@kernel.org]

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?

>  		err = -EPERM;
>  		goto inval_skb;
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=2