Re: [PATCH net 02/14] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
From: sashiko-bot@kernel.org
Date: 2026-08-27 12:10:55
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 [off-list ref]
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
quoted hunk ↗ jump to hunk
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