From: Vincent Mailhol Sending a PF_PACKET bypasses the CAN framework logic and can directly reach a CAN driver's xmit() function. The PF_PACKET framework only checks that skb->len does not exceed the net_device MTU. For a CAN device that is not CAN XL capable, anything above CANFD_MTU (72 bytes) is therefore dropped before it reaches the driver. However, CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to 2060 bytes. As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes can pass both the MTU and the can_dropped_invalid_skb() checks. A driver that does not support CAN XL will interpret canxl_frame->flags as a length because of the overlap with can_frame->len. And because CANXL_XLF is set, the resulting length is between 128 and 255. For drivers that do not check can_frame->len before copying can_frame->data, as most drivers do, this results in a buffer overflow of up to 247 bytes. Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs. Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1 Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames") Signed-off-by: Vincent Mailhol Link: https://patch.msgid.link/20260731-drop_canxl_frames-v1-1-7387b70353b3@kernel.org Cc: stable@kernel.org Signed-off-by: Marc Kleine-Budde --- drivers/net/can/dev/skb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 14edec5afb57..bc0787535b52 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -4,6 +4,7 @@ * Copyright (C) 2008-2009 Wolfgang Grandegger */ +#include #include #include #include @@ -388,7 +389,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb) break; case ETH_P_CANXL: - if (!can_is_canxl_skb(skb)) + if (!can_cap_enabled(dev, CAN_CAP_XL) || !can_is_canxl_skb(skb)) goto inval_skb; break; base-commit: 98c2ad4f5d805dbc1ea436a334ee59311806a48c -- 2.53.0 From: Sang-Heon Jeon netdev_hold() does nothing if dev is NULL, so the check before the call is redundant. So remove it. No functional change. This is the result of running the Coccinelle script from scripts/coccinelle/free/ifnulldev_put.cocci after commit f83b8a58695c ("coccinelle: ifnulldev_put: update outdated helper names"). Signed-off-by: Sang-Heon Jeon Acked-by: Oliver Hartkopp Link: https://patch.msgid.link/20260831052031.902699-1-ekffu200098@gmail.com Signed-off-by: Marc Kleine-Budde --- net/can/raw.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/can/raw.c b/net/can/raw.c index 0acd4f6c6dd6..6944ebab3feb 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -529,8 +529,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len ro->bound = 1; /* bind() ok -> hold a reference for new ro->dev */ ro->dev = dev; - if (ro->dev) - netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); + netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); } out_put_dev: -- 2.53.0 From: Cunhao Lu <1579567540@qq.com> can_put_echo_skb() can be called with hardware interrupts disabled. Its direct drop paths use kfree_skb(), while can_create_echo_skb() uses kfree_skb() when cloning fails and consume_skb() after a successful clone. None of these helpers is safe in every IRQ context. Use dev_kfree_skb_any() for all drop paths and dev_consume_skb_any() when consuming a successfully cloned skb. This preserves the respective skb drop and consumed semantics regardless of the caller IRQ context. Signed-off-by: Cunhao Lu <1579567540@qq.com> Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com Cc: stable@kernel.org Fixes: ae64438be192 ("can: dev: fix skb drop check") Fixes: 0ae89beb283a ("can: add destructor for self generated skbs") Fixes: 3ccd4c6167d3 ("can: Unify droping of invalid tx skbs and netdev stats") Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface") [mkl: also convert can_dropped_invalid_skb(), can_dev_dropped_skb()] [mkl: add stable on Cc + add Fixes tags] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/dev/skb.c | 6 +++--- include/linux/can/dev.h | 2 +- include/linux/can/skb.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index bc0787535b52..46c151eece8f 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, (skb->protocol != htons(ETH_P_CAN) && skb->protocol != htons(ETH_P_CANFD) && skb->protocol != htons(ETH_P_CANXL))) { - kfree_skb(skb); + dev_kfree_skb_any(skb); return 0; } @@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, } else { /* locking problem with netif_stop_queue() ?? */ netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx); - kfree_skb(skb); + dev_kfree_skb_any(skb); return -EBUSY; } @@ -403,7 +403,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb) return false; inval_skb: - kfree_skb(skb); + dev_kfree_skb_any(skb); dev->stats.tx_dropped++; return true; } diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 6d0710d6f571..346faddee02e 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -176,7 +176,7 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s return can_dropped_invalid_skb(dev, skb); invalid_skb: - kfree_skb(skb); + dev_kfree_skb_any(skb); dev->stats.tx_dropped++; return true; } diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h index 5d27843862fc..87ace09c0d8b 100644 --- a/include/linux/can/skb.h +++ b/include/linux/can/skb.h @@ -78,12 +78,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb) nskb = skb_clone(skb, GFP_ATOMIC); if (unlikely(!nskb)) { - kfree_skb(skb); + dev_kfree_skb_any(skb); return NULL; } can_skb_set_owner(nskb, skb->sk); - consume_skb(skb); + dev_consume_skb_any(skb); return nskb; } -- 2.53.0 From: Cunhao Lu <1579567540@qq.com> The CAN skb allocation helpers are used from hardware interrupt receive handlers. If can_skb_ext_add() fails, they release the newly allocated skb with kfree_skb(), which is not safe in hardware interrupt context. Use dev_kfree_skb_any() for the allocation failure paths in alloc_can_skb(), alloc_canfd_skb(), and alloc_canxl_skb(). Fixes: 96ea3a1e2d31 ("can: add CAN skb extension infrastructure") Signed-off-by: Cunhao Lu <1579567540@qq.com> Link: https://patch.msgid.link/tencent_C825C17D442F801351CE2FBC4984064B4605@qq.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/dev/skb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 46c151eece8f..3e36c0368bae 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -228,7 +228,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) csx = can_skb_ext_add(skb); if (!csx) { - kfree_skb(skb); + dev_kfree_skb_any(skb); goto out_error_cc; } @@ -259,7 +259,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, csx = can_skb_ext_add(skb); if (!csx) { - kfree_skb(skb); + dev_kfree_skb_any(skb); goto out_error_fd; } @@ -297,7 +297,7 @@ struct sk_buff *alloc_canxl_skb(struct net_device *dev, csx = can_skb_ext_add(skb); if (!csx) { - kfree_skb(skb); + dev_kfree_skb_any(skb); goto out_error_xl; } -- 2.53.0 From: Cunhao Lu <1579567540@qq.com> can_put_echo_skb() consumes the skb on all paths except when the echo index is out of bounds. This leaves ownership with the caller on -EINVAL, unlike the other error paths, and can leak the skb if the caller expects consistent semantics. Free the skb before returning -EINVAL so that all return paths consume it. Fixes: 6411959c10fe ("can: dev: can_put_echo_skb(): don't crash kernel if can_priv::echo_skb is accessed out of bounds") Cc: stable@vger.kernel.org Reviewed-by: Vincent Mailhol Signed-off-by: Cunhao Lu <1579567540@qq.com> Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com Signed-off-by: Marc Kleine-Budde --- FAQ: Q: `can_skb_init_valid()` directly modifies `skb->data` without checking if the SKB is cloned or shared. Is this violating SKB shared buffer rules and causing data corruption? A: The finding about can_skb_init_valid() modifying skb->data on a potentially shared buffer is not relevant for this (correct) patch. The FDF-flag write in can_skb_init_valid() only matters for PF_PACKET use: PF_CAN allocators already set CANFD_FDF. The write exists to normalize frames for PF_PACKET observers (tcpdump/Wireshark) so they can distinguish classic CAN from CAN FD at the netdev level, and to compensate for PF_PACKET senders that omit the bit. That normalization is functionally required. The shared-buffer concern only becomes realistic through a specific chain: a PF_PACKET sender injects a CANFD-sized frame, the CAN driver's loopback puts an echo skb back into can_rcv(), and cgw (without modfuncs) clones it for forwarding to another interface. Only on that second can_skb_init_valid() call is the skb cloned. By then the bit was already set on the exclusive skb during the initial xmit path, so the flags |= CANFD_FDF write is strictly idempotent for any parallel consumer of the shared buffer - no memory-safety or information-disclosure consequence. It's a formal violation of the skb sharing rules without an observable effect, so the current code can stay as-is. Link: https://lore.kernel.org/all/54a3cc01-abcf-4a33-b932-39bb1f68cdd5@hartkopp.net/ [mkl: convert Oliver's mail to FAQ section] --- drivers/net/can/dev/skb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c index 3e36c0368bae..738d5968dfa6 100644 --- a/drivers/net/can/dev/skb.c +++ b/drivers/net/can/dev/skb.c @@ -55,6 +55,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, if (idx >= priv->echo_skb_max) { netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n", __func__, idx, priv->echo_skb_max); + dev_kfree_skb_any(skb); return -EINVAL; } -- 2.53.0 From: Kaixuan Li isotp_rcv() separates Classic CAN from CAN FD by skb->len alone: if (skb->len != so->ll.mtu) return; cf = (struct canfd_frame *)skb->data; A CAN XL frame with cxl->len 4 is CAN_MTU bytes, so it passes, and is then read as a canfd_frame whose len comes out of canxl_frame.flags: at least 0x80. Of the paths that follow, only the flow control one uses that length without bounding it first, so check_pad() walks to 255 over a 16-byte frame and the caller reports EBADMSG on an unrelated socket. bcm_rx_handler(), j1939_can_recv(), can_can_gw_rcv() and raw_rcv() check the frame type here, and can_dropped_invalid_skb() switches on skb->protocol on the transmit side. isotp_rcv() is the gap. Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames") Signed-off-by: Kaixuan Li Reviewed-by: Oliver Hartkopp Acked-by: Oliver Hartkopp Reviewed-by: Quchaosheng Link: https://patch.msgid.link/20260920035626.2581040-1-kaixuanli0131@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde --- net/can/isotp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/net/can/isotp.c b/net/can/isotp.c index f5dc9d04bd68..d3e43ab48425 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -755,6 +755,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data) if (skb->len != so->ll.mtu) return; + /* check for correct CAN CC/FD frame content */ + if (so->ll.mtu == CAN_MTU) { + if (!can_is_can_skb(skb)) + return; + } else if (!can_is_canfd_skb(skb)) { + return; + } + cf = (struct canfd_frame *)skb->data; /* if enabled: check reception of my configured extended address */ -- 2.53.0 From: Runyu Xiao Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path") converted several members of struct can_pkg_stats to atomic_long_t and updated the hot TX/RX and procfs read paths to use atomic_long_*() helpers. However, can_init_stats() still clears the whole struct with memset(). can_init_stats() is reached from can_stat_update() in timer context and also from the procfs reset path. Those paths can run while the TX/RX hot paths are concurrently updating rx_frames, tx_frames, matches and their *_delta counters. Hitting the whole-struct memset() in that window performs plain writes to fields that otherwise follow an atomic_long_t access contract, which can lose or mix live statistics updates. This issue was found by source-level API-misuse analysis looking for whole-object resets left behind after atomic_long_t conversions, then manually audited on Linux v6.18.21. Replace the whole-struct memset() with a helper that resets the atomic_long_t counters via atomic_long_set() and clears the derived scalar statistics explicitly. This preserves the existing reset semantics for scalar fields while restoring atomic access discipline for the live counters. Build-tested by compiling net/can/proc.o on x86_64 netdev/main. Runtime-tested with a QEMU + vcan setup on Linux v6.18.21 by driving concurrent traffic and reset_stats reads, which reproduced inconsistent exported statistics before the fix. [Hartkopp] can_stat_update() also read the *_delta counters at the beginning and cleared them with atomic_long_set() at the end. Increments from the hot path in between were lost for both the current and the next interval. Use atomic_long_xchg() to fetch and clear the *_delta counters in one atomic step instead. Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path") Cc: stable@vger.kernel.org Signed-off-by: Runyu Xiao Acked-by: Oliver Hartkopp Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20261019152741.54854-2-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde --- net/can/proc.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/net/can/proc.c b/net/can/proc.c index de4d05ae3459..70104f13077e 100644 --- a/net/can/proc.c +++ b/net/can/proc.c @@ -76,16 +76,39 @@ static const char rx_list_name[][8] = { * af_can statistics stuff */ +static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats) +{ + atomic_long_set(&pkg_stats->rx_frames, 0); + atomic_long_set(&pkg_stats->tx_frames, 0); + atomic_long_set(&pkg_stats->matches, 0); + + pkg_stats->total_rx_rate = 0; + pkg_stats->total_tx_rate = 0; + pkg_stats->total_rx_match_ratio = 0; + + pkg_stats->current_rx_rate = 0; + pkg_stats->current_tx_rate = 0; + pkg_stats->current_rx_match_ratio = 0; + + pkg_stats->max_rx_rate = 0; + pkg_stats->max_tx_rate = 0; + pkg_stats->max_rx_match_ratio = 0; + + atomic_long_set(&pkg_stats->rx_frames_delta, 0); + atomic_long_set(&pkg_stats->tx_frames_delta, 0); + atomic_long_set(&pkg_stats->matches_delta, 0); +} + static void can_init_stats(struct net *net) { struct can_pkg_stats *pkg_stats = net->can.pkg_stats; struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats; /* - * This memset function is called from a timer context (when + * This stats reset is called from a timer context (when * can_stattimer is active which is the default) OR in a process * context (reading the proc_fs when can_stattimer is disabled). */ - memset(pkg_stats, 0, sizeof(struct can_pkg_stats)); + can_reset_pkg_stats(pkg_stats); pkg_stats->jiffies_init = jiffies; rcv_lists_stats->stats_reset++; @@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t) long rx_frames = atomic_long_read(&pkg_stats->rx_frames); long tx_frames = atomic_long_read(&pkg_stats->tx_frames); long matches = atomic_long_read(&pkg_stats->matches); - long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta); - long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta); - long matches_delta = atomic_long_read(&pkg_stats->matches_delta); + /* fetch and clear values for 'current rate' calculation */ + long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0); + long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0); + long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0); /* restart counting in timer context on user request */ if (user_reset) @@ -172,11 +196,6 @@ void can_stat_update(struct timer_list *t) if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio) pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio; - /* clear values for 'current rate' calculation */ - atomic_long_set(&pkg_stats->tx_frames_delta, 0); - atomic_long_set(&pkg_stats->rx_frames_delta, 0); - atomic_long_set(&pkg_stats->matches_delta, 0); - /* restart timer (one second) */ mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ)); } -- 2.53.0 From: Oliver Hartkopp Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path") introduced local snapshots of the statistics counters at the beginning of can_stat_update(). When can_init_stats() resets the statistics, these stale pre-reset snapshots are still used for the following total/current /max calculations and overwrite the freshly reset values. Additionally a jiffies tick between the snapshot and the reset could trigger a second can_init_stats() call. Merge the reset conditions into a single check and skip the calculation for the current interval when the statistics have been reset. Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path") Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20261019152741.54854-3-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde --- net/can/proc.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/net/can/proc.c b/net/can/proc.c index 70104f13077e..fb144d2adc35 100644 --- a/net/can/proc.c +++ b/net/can/proc.c @@ -149,25 +149,18 @@ void can_stat_update(struct timer_list *t) long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0); long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0); - /* restart counting in timer context on user request */ - if (user_reset) - can_init_stats(net); - - /* restart counting on jiffies overflow */ - if (j < pkg_stats->jiffies_init) - can_init_stats(net); - - /* prevent overflow in calc_rate() */ - if (rx_frames > (LONG_MAX / HZ)) - can_init_stats(net); - - /* prevent overflow in calc_rate() */ - if (tx_frames > (LONG_MAX / HZ)) - can_init_stats(net); - - /* matches overflow - very improbable */ - if (matches > (LONG_MAX / 100)) + /* + * Restart counting in timer context on user request, on jiffies + * overflow or to prevent overflows in calc_rate() and in the + * match ratio calculation. The local snapshots are stale after + * the reset, so skip the calculation for this interval. + */ + if (user_reset || j < pkg_stats->jiffies_init || + rx_frames > (LONG_MAX / HZ) || tx_frames > (LONG_MAX / HZ) || + matches > (LONG_MAX / 100)) { can_init_stats(net); + goto out; + } /* calc total values */ if (rx_frames) @@ -196,6 +189,7 @@ void can_stat_update(struct timer_list *t) if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio) pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio; +out: /* restart timer (one second) */ mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ)); } -- 2.53.0 From: Oliver Hartkopp The proc content in /proc/net/can/ and /proc/net/can-bcm/ is intended to check the internal filter lists (af_can, can_raw) and the efficiency and functionality of can_bcm jobs. While exposing kernel addresses was acceptable when this code was written the times have changed and multiple attempts have been made to hash or remove such now sensitive data. This patch removes the disclosure of pointers and instead provides the function names and sock inode numbers when available. When the function name can not be resolved (e.g. KALLSYMS=n or stripped module symbols) %ps would fall back to the raw text address. Therefore (unknown) is printed instead in this case. As there's no known tooling around the CAN specific proc output breaking the ABI with this rework creates no issue. Suggested-by: Sebastian Andrzej Siewior Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20261019152741.54854-4-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde --- Documentation/networking/can.rst | 16 ++++++++++------ include/linux/can/core.h | 2 +- net/can/af_can.c | 6 +++--- net/can/af_can.h | 2 +- net/can/bcm.c | 14 +++++++------- net/can/gw.c | 2 +- net/can/isotp.c | 4 ++-- net/can/j1939/main.c | 2 +- net/can/proc.c | 26 ++++++++++++++++---------- net/can/raw.c | 4 ++-- 10 files changed, 44 insertions(+), 34 deletions(-) diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst index 536ff411da1d..51bc70748f75 100644 --- a/Documentation/networking/can.rst +++ b/Documentation/networking/can.rst @@ -1040,17 +1040,21 @@ As described in :ref:`socketcan-receive-lists` the SocketCAN core uses several f lists to deliver received CAN frames to CAN protocol modules. These receive lists, their filters and the count of filter matches can be checked in the appropriate receive list. All entries contain the -device and a protocol module identifier:: +CAN network device, the filter (can_id/can_mask), the number of filter +matches, the inode number of the subscribing socket and the name of the +receive function. A sock_inode of 0 denotes a kernel internal subscriber +without a socket (e.g. CAN gateway or J1939). When the function name can +not be resolved (e.g. with CONFIG_KALLSYMS=n) "(unknown)" is printed:: foo@bar:~$ cat /proc/net/can/rcvlist_all receive list 'rx_all': - (vcan3: no entry) - (vcan2: no entry) - (vcan1: no entry) - device can_id can_mask function userdata matches ident - vcan0 000 00000000 f88e6370 f6c6f400 0 raw (any: no entry) + device can_id can_mask matches sock_inode function + vcan0 000 00000000 0 63586 raw_rcv [can_raw] + (vcan1: no entry) + (vcan2: no entry) + (vcan3: no entry) In this example an application requests any CAN traffic from vcan0:: diff --git a/include/linux/can/core.h b/include/linux/can/core.h index 2de74c2b78b6..effb5c31ef40 100644 --- a/include/linux/can/core.h +++ b/include/linux/can/core.h @@ -51,7 +51,7 @@ extern void can_proto_unregister(const struct can_proto *cp); int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), - void *data, char *ident, struct sock *sk); + void *data, u64 ino, struct sock *sk); extern void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, diff --git a/net/can/af_can.c b/net/can/af_can.c index 34fe3b28d576..6ebe9d3689ac 100644 --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -418,7 +418,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask, * @mask: CAN mask (see description) * @func: callback function on filter match * @data: returned parameter for callback function - * @ident: string for calling module identification + * @ino: inode number of sock (0 = unknown) * @sk: socket pointer (might be NULL) * * Description: @@ -443,7 +443,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask, */ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), - void *data, char *ident, struct sock *sk) + void *data, u64 ino, struct sock *sk) { struct receiver *rcv; struct hlist_head *rcv_list; @@ -472,7 +472,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, atomic_long_set(&rcv->matches, 0); rcv->func = func; rcv->data = data; - rcv->ident = ident; + rcv->ino = ino; rcv->sk = sk; hlist_add_head_rcu(&rcv->list, rcv_list); diff --git a/net/can/af_can.h b/net/can/af_can.h index 87887014f562..6e593ed5db5f 100644 --- a/net/can/af_can.h +++ b/net/can/af_can.h @@ -55,7 +55,7 @@ struct receiver { atomic_long_t matches; void (*func)(struct sk_buff *skb, void *data); void *data; - char *ident; + u64 ino; struct sock *sk; struct rcu_head rcu; }; diff --git a/net/can/bcm.c b/net/can/bcm.c index 3d637a1e0ac1..6c85a76477c3 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -220,9 +220,7 @@ static int bcm_proc_show(struct seq_file *m, void *v) struct bcm_sock *bo = bcm_sk(sk); struct bcm_op *op; - seq_printf(m, ">>> socket %pK", sk->sk_socket); - seq_printf(m, " / sk %pK", sk); - seq_printf(m, " / bo %pK", bo); + seq_printf(m, ">>> sock inode %s", bo->procname); seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs); seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex)); seq_printf(m, " <<<\n"); @@ -1536,7 +1534,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, op->can_id, REGMASK(op->can_id), bcm_rx_handler, op, - "bcm", sk); + sock_i_ino(sk), sk); /* keep a tracked reference so that a later * unregister can safely reach the device even @@ -1560,7 +1558,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, } else { err = can_rx_register(sock_net(sk), NULL, op->can_id, REGMASK(op->can_id), - bcm_rx_handler, op, "bcm", sk); + bcm_rx_handler, op, + sock_i_ino(sk), sk); } if (err) { @@ -2040,8 +2039,9 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int #if IS_ENABLED(CONFIG_PROC_FS) if (net->can.bcmproc_dir) { - /* unique socket address as filename */ - sprintf(bo->procname, "%llu", sock_i_ino(sk)); + /* use unique socket inode number as filename */ + snprintf(bo->procname, sizeof(bo->procname), + "%llu", sock_i_ino(sk)); bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644, net->can.bcmproc_dir, bcm_proc_show, sk); diff --git a/net/can/gw.c b/net/can/gw.c index 5793cb2420ed..36f9aa0f7026 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -579,7 +579,7 @@ static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj) { return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id, gwj->ccgw.filter.can_mask, can_can_gw_rcv, - gwj, "gw", NULL); + gwj, 0, NULL); } static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj) diff --git a/net/can/isotp.c b/net/can/isotp.c index d3e43ab48425..64ff203a37f4 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1633,14 +1633,14 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l if (isotp_register_rxid(so)) can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), - isotp_rcv, sk, "isotp", sk); + isotp_rcv, sk, sock_i_ino(sk), sk); /* no consecutive frame echo skb in flight */ WRITE_ONCE(so->cfecho, 0); /* register for echo skb's */ can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id), - isotp_rcv_echo, sk, "isotpe", sk); + isotp_rcv_echo, sk, sock_i_ino(sk), sk); /* switch to new settings */ so->ifindex = ifindex; diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c index 5e5e6c228f22..d9384cf0e356 100644 --- a/net/can/j1939/main.c +++ b/net/can/j1939/main.c @@ -184,7 +184,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv) j1939_priv_get(priv); ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK, - j1939_can_recv, priv, "j1939", NULL); + j1939_can_recv, priv, 0, NULL); if (ret < 0) { j1939_priv_put(priv); return ret; diff --git a/net/can/proc.c b/net/can/proc.c index fb144d2adc35..b7496d2a37cd 100644 --- a/net/can/proc.c +++ b/net/can/proc.c @@ -204,26 +204,32 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list, struct receiver *r; hlist_for_each_entry_rcu(r, rx_list, list) { + char symcheck[4]; char *fmt = (r->can_id & CAN_EFF_FLAG)? - " %-5s %08x %08x %pK %pK %8ld %s\n" : - " %-5s %03x %08x %pK %pK %8ld %s\n"; + " %6s %08x %08x %8ld %10llu" : + " %6s %03x %08x %8ld %10llu"; seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask, - r->func, r->data, atomic_long_read(&r->matches), - r->ident); + atomic_long_read(&r->matches), r->ino); + + /* %ps falls back to the raw text address when the symbol + * can not be resolved (KALLSYMS=n, stripped module symbols) + */ + snprintf(symcheck, sizeof(symcheck), "%ps", r->func); + if (strncmp(symcheck, "0x", 2)) + seq_printf(m, " %ps\n", r->func); + else + seq_puts(m, " (unknown)\n"); } } static void can_print_recv_banner(struct seq_file *m) { /* - * can1. 00000000 00000000 00000000 - * ....... 0 tp20 + * device can_id can_mask matches sock_inode function + * vcan0 000 00000000 0 43798 raw_rcv [can_raw] */ - if (IS_ENABLED(CONFIG_64BIT)) - seq_puts(m, " device can_id can_mask function userdata matches ident\n"); - else - seq_puts(m, " device can_id can_mask function userdata matches ident\n"); + seq_puts(m, " device can_id can_mask matches sock_inode function\n"); } static int can_stats_proc_show(struct seq_file *m, void *v) diff --git a/net/can/raw.c b/net/can/raw.c index 6944ebab3feb..3e57f72a3afe 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -226,7 +226,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev, for (i = 0; i < count; i++) { err = can_rx_register(net, dev, filter[i].can_id, filter[i].can_mask, - raw_rcv, sk, "raw", sk); + raw_rcv, sk, sock_i_ino(sk), sk); if (err) { /* clean up successfully registered filters */ while (--i >= 0) @@ -247,7 +247,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev, if (err_mask) err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG, - raw_rcv, sk, "raw", sk); + raw_rcv, sk, sock_i_ino(sk), sk); return err; } -- 2.53.0 From: Oliver Hartkopp Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device") introduced an explicit way to assign the midlayer private pointer (dev->ml_priv) to named users like ML_PRIV_CAN. A valid pointer returned by can_get_ml_priv() therefore identifies a real CAN device. The referenced commit only converted the j1939 and proc code to this check, while the other parts of the CAN subsystem still rely on dev->type == ARPHRD_CAN. The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed netdevice passes the ARPHRD_CAN checks and triggers a NULL pointer dereference when CAN filters are created and when they are removed, e.g. by the CAN protocols' netdevice notifier handlers. This only requires owning a TUN/TAP device or CAP_NET_ADMIN in an unprivileged user namespace. Convert the remaining ARPHRD_CAN checks in af_can, bcm, gw, isotp and raw to can_get_ml_priv(). Fixes: 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device") Cc: stable@kernel.org Cc: Oleksij Rempel Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net Link: https://patch.msgid.link/20261019152741.54854-5-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde --- net/can/af_can.c | 15 ++++++--------- net/can/bcm.c | 7 ++++--- net/can/gw.c | 7 ++++--- net/can/isotp.c | 5 +++-- net/can/raw.c | 4 ++-- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/net/can/af_can.c b/net/can/af_can.c index 6ebe9d3689ac..d97f85fc3232 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))) { err = -EPERM; goto inval_skb; } @@ -452,7 +452,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, /* insert new receiver (dev,canid,mask) -> (func,data) */ - if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev))) + if (dev && !can_get_ml_priv(dev)) return -ENODEV; if (dev && !net_eq(net, dev_net(dev))) @@ -519,7 +519,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats; struct can_dev_rcv_lists *dev_rcv_lists; - if (dev && dev->type != ARPHRD_CAN) + if (dev && !can_get_ml_priv(dev)) return; if (dev && !net_eq(net, dev_net(dev))) @@ -684,8 +684,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev, { struct can_skb_ext *csx = can_skb_ext_find(skb); - if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) || - !csx || !can_is_can_skb(skb))) { + if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_can_skb(skb))) { pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n", dev->type, skb->len); @@ -710,8 +709,7 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev, { struct can_skb_ext *csx = can_skb_ext_find(skb); - if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) || - !csx || !can_is_canfd_skb(skb))) { + if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canfd_skb(skb))) { pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n", dev->type, skb->len); @@ -736,8 +734,7 @@ static int canxl_rcv(struct sk_buff *skb, struct net_device *dev, { struct can_skb_ext *csx = can_skb_ext_find(skb); - if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) || - !csx || !can_is_canxl_skb(skb))) { + if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canxl_skb(skb))) { pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n", dev->type, skb->len); diff --git a/net/can/bcm.c b/net/can/bcm.c index 6c85a76477c3..6759662e024c 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -54,6 +54,7 @@ #include #include #include +#include #include #include #include @@ -1718,7 +1719,7 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto out_release; } - if (dev->type != ARPHRD_CAN) { + if (!can_get_ml_priv(dev)) { dev_put(dev); ret = -ENODEV; goto out_release; @@ -1866,7 +1867,7 @@ static int bcm_notifier(struct notifier_block *nb, unsigned long msg, { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - if (dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(dev)) return NOTIFY_DONE; if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) return NOTIFY_DONE; @@ -2023,7 +2024,7 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int ret = -ENODEV; goto fail; } - if (dev->type != ARPHRD_CAN) { + if (!can_get_ml_priv(dev)) { dev_put(dev); ret = -ENODEV; goto fail; diff --git a/net/can/gw.c b/net/can/gw.c index 36f9aa0f7026..8a5327d8a00e 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -52,6 +52,7 @@ #include #include #include +#include #include #include #include @@ -612,7 +613,7 @@ static int cgw_notifier(struct notifier_block *nb, struct net_device *dev = netdev_notifier_info_to_dev(ptr); struct net *net = dev_net(dev); - if (dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(dev)) return NOTIFY_DONE; if (msg == NETDEV_UNREGISTER) { @@ -1163,7 +1164,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, if (!gwj->src.dev) goto out; - if (gwj->src.dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(gwj->src.dev)) goto out; gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx); @@ -1171,7 +1172,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, if (!gwj->dst.dev) goto out; - if (gwj->dst.dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(gwj->dst.dev)) goto out; /* is sending the skb back to the incoming interface intended? */ diff --git a/net/can/isotp.c b/net/can/isotp.c index 64ff203a37f4..5e1c3971ebfc 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -65,6 +65,7 @@ #include #include #include +#include #include #include #include @@ -1618,7 +1619,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l err = -ENODEV; goto out; } - if (dev->type != ARPHRD_CAN) { + if (!can_get_ml_priv(dev)) { err = -ENODEV; goto out_put_dev; } @@ -1905,7 +1906,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg, { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - if (dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(dev)) return NOTIFY_DONE; if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) return NOTIFY_DONE; diff --git a/net/can/raw.c b/net/can/raw.c index 3e57f72a3afe..dfea48768b23 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -348,7 +348,7 @@ static int raw_notifier(struct notifier_block *nb, unsigned long msg, { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - if (dev->type != ARPHRD_CAN) + if (!can_get_ml_priv(dev)) return NOTIFY_DONE; if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN) return NOTIFY_DONE; @@ -491,7 +491,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len err = -ENODEV; goto out; } - if (dev->type != ARPHRD_CAN) { + if (!can_get_ml_priv(dev)) { err = -ENODEV; goto out_put_dev; } -- 2.53.0 From: Oliver Hartkopp The receive lists of a CAN device are located in the device's can_ml_priv and travel with the device when it is moved to another network namespace. But they are protected by the per-netns rcvlists_lock. So can_rx_unregister() needs dev_net(dev) to pick the right lock and silently returns when the passed namespace does not match dev_net(dev) anymore. isotp_release() and bcm_release() leave their notifier list before they take any lock that serializes against the netdev notifier, which runs under RTNL. When the bound device is moved to another namespace in this window, the notifier skips the socket and the release path has to remove the filters itself. isotp_release() passes the socket's namespace, which does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev), which can_rx_unregister() reads again for its check, so a namespace change between both reads makes the check fail as well. In both cases can_rx_unregister() silently returns. As can_rx_register() stores rcv->sk without a reference, the remaining receivers point to the freed socket (isotp) or the freed op (bcm): BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0 Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578 can_rcv_filter+0x4af/0x8c0 can_receive+0x28d/0x3c0 can_rcv+0x2a9/0x310 __netif_receive_skb_one_core+0x21a/0x260 process_backlog+0x210/0x760 Besides this, the lists of a device are protected by the rcvlists_lock of the namespace that is passed in. Callers which pass dev_net(dev) read it at different times, so the same lists may be modified under the locks of different namespaces while the device is moved. Taking RTNL in these release paths is no option: raw_bind(), raw_setsockopt() and raw_release() already hold RTNL while adding and removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged users can trigger repeatedly and which makes RTNL a global bottleneck. Instead fix the root cause and add a spinlock to struct can_dev_rcv_lists, which protects the receive lists of a CAN device or the 'any' CAN device lists of a namespace. It is initialized in can_set_ml_priv(), which is used by all CAN network devices. With this, can_rx_unregister() finds the subscription independently of the current namespace of the device. The protocols keep the device valid with their tracked references. The per-netns rcvlists_lock only protects the receive list statistics anymore, and the callers pass the namespace that has been used for can_rx_register(). So isotp_release() and bcm_release() now remove their filters correctly without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and isotp_bind() do not need RTNL anymore and add and remove their filters under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind() publishes the device inside the RCU section of its lookup, and raw_notify() and isotp_notify() recheck the bound device under lock_sock(). Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER") Fixes: 8e8cda6d737d ("can: initial support for network namespaces") Reported-by: Norbert Szetei Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/ Cc: stable@vger.kernel.org Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20261019152741.54854-6-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde --- include/linux/can/can-ml.h | 3 + include/net/netns/can.h | 2 +- net/can/af_can.c | 40 +++++++---- net/can/bcm.c | 43 ++++------- net/can/isotp.c | 40 +++++++---- net/can/raw.c | 142 ++++++++++++++++++++++++------------- 6 files changed, 161 insertions(+), 109 deletions(-) diff --git a/include/linux/can/can-ml.h b/include/linux/can/can-ml.h index 1e99fda2b380..cfefa4a385be 100644 --- a/include/linux/can/can-ml.h +++ b/include/linux/can/can-ml.h @@ -45,6 +45,7 @@ #include #include #include +#include /* exposed CAN device capabilities for network layer */ #define CAN_CAP_CC BIT(0) /* CAN CC aka Classical CAN */ @@ -59,6 +60,7 @@ enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX }; struct can_dev_rcv_lists { + spinlock_t lock; /* protects the receive lists and entries */ struct hlist_head rx[RX_MAX]; struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ]; struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ]; @@ -81,6 +83,7 @@ static inline struct can_ml_priv *can_get_ml_priv(struct net_device *dev) static inline void can_set_ml_priv(struct net_device *dev, struct can_ml_priv *ml_priv) { + spin_lock_init(&ml_priv->dev_rcv_lists.lock); netdev_set_ml_priv(dev, ml_priv, ML_PRIV_CAN); } diff --git a/include/net/netns/can.h b/include/net/netns/can.h index 48b79f7e6236..ef7aefdf7b23 100644 --- a/include/net/netns/can.h +++ b/include/net/netns/can.h @@ -29,7 +29,7 @@ struct netns_can { /* receive filters subscribed for 'all' CAN devices */ struct can_dev_rcv_lists *rx_alldev_list; - spinlock_t rcvlists_lock; + spinlock_t rcvlists_lock; /* protects rcv_lists_stats */ struct timer_list stattimer; /* timer for statistics update */ struct can_pkg_stats *pkg_stats; struct can_rcv_lists_stats *rcv_lists_stats; diff --git a/net/can/af_can.c b/net/can/af_can.c index d97f85fc3232..bf612c4b983d 100644 --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -462,9 +462,10 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, if (!rcv) return -ENOMEM; - spin_lock_bh(&net->can.rcvlists_lock); - dev_rcv_lists = can_dev_rcv_lists_find(net, dev); + + spin_lock_bh(&dev_rcv_lists->lock); + rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists); rcv->can_id = can_id; @@ -478,6 +479,9 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id, hlist_add_head_rcu(&rcv->list, rcv_list); dev_rcv_lists->entries++; + spin_unlock_bh(&dev_rcv_lists->lock); + + spin_lock_bh(&net->can.rcvlists_lock); rcv_lists_stats->rcv_entries++; rcv_lists_stats->rcv_entries_max = max(rcv_lists_stats->rcv_entries_max, rcv_lists_stats->rcv_entries); @@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct rcu_head *rp) * * Description: * Removes subscription entry depending on given (subscription) values. + * The receive lists of a CAN device are protected by a per-device lock, + * so the subscription is found even when the device has been moved to + * another network namespace after can_rx_register(). The caller has to + * ensure that @dev stays valid, e.g. by holding a reference to @dev or + * RTNL. @net is the namespace that has been used for can_rx_register(). + * It selects the 'any' CAN device lists for @dev == NULL and is used for + * the receive list statistics. */ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, canid_t mask, void (*func)(struct sk_buff *, void *), @@ -522,12 +533,10 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, if (dev && !can_get_ml_priv(dev)) return; - if (dev && !net_eq(net, dev_net(dev))) - return; - - spin_lock_bh(&net->can.rcvlists_lock); - dev_rcv_lists = can_dev_rcv_lists_find(net, dev); + + spin_lock_bh(&dev_rcv_lists->lock); + rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists); /* Search the receiver list for the item to delete. This should @@ -547,26 +556,26 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id, * a warning here. */ if (!rcv) { + spin_unlock_bh(&dev_rcv_lists->lock); pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n", DNAME(dev), can_id, mask); - goto out; + return; } hlist_del_rcu(&rcv->list); dev_rcv_lists->entries--; + spin_unlock_bh(&dev_rcv_lists->lock); + + spin_lock_bh(&net->can.rcvlists_lock); if (rcv_lists_stats->rcv_entries > 0) rcv_lists_stats->rcv_entries--; - - out: spin_unlock_bh(&net->can.rcvlists_lock); /* schedule the receiver item for deletion */ - if (rcv) { - if (rcv->sk) - sock_hold(rcv->sk); - call_rcu(&rcv->rcu, can_rx_delete_receiver); - } + if (rcv->sk) + sock_hold(rcv->sk); + call_rcu(&rcv->rcu, can_rx_delete_receiver); } EXPORT_SYMBOL(can_rx_unregister); @@ -823,6 +832,7 @@ static int can_pernet_init(struct net *net) net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list); if (!net->can.rx_alldev_list) goto out; + spin_lock_init(&net->can.rx_alldev_list->lock); net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats); if (!net->can.pkg_stats) goto out_free_rx_alldev_list; diff --git a/net/can/bcm.c b/net/can/bcm.c index 6759662e024c..7bd2aa39777a 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -962,7 +962,7 @@ static void bcm_remove_op(struct bcm_op *op) static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op) { if (op->rx_reg_dev == dev) { - can_rx_unregister(dev_net(dev), dev, op->can_id, + can_rx_unregister(sock_net(op->sk), dev, op->can_id, REGMASK(op->can_id), bcm_rx_handler, op); /* mark as removed subscription */ @@ -988,21 +988,10 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh, /* disable automatic timer on frame reception */ op->flags |= RX_NO_AUTOTIMER; - /* - * Don't care if we're bound or not (due to netdev - * problems) can_rx_unregister() is always a save - * thing to do here. - */ if (op->ifindex) { - /* - * Only remove subscriptions that had not - * been removed due to NETDEV_UNREGISTER - * in bcm_notifier() - * - * op->rx_reg_dev is a tracked reference taken - * when the subscription was registered, so it - * stays valid here even if a concurrent - * NETDEV_UNREGISTER already unlisted the dev. + /* Only remove subscriptions that had not been + * removed in bcm_notify(). op->rx_reg_dev keeps + * the device's receive lists accessible. */ if (op->rx_reg_dev) bcm_rx_unreg(op->rx_reg_dev, op); @@ -1935,6 +1924,7 @@ static int bcm_release(struct socket *sock) list_del(&bo->notifier); spin_unlock(&bcm_notifier_lock); + /* bcm_notify() can not access this socket anymore */ lock_sock(sk); #if IS_ENABLED(CONFIG_PROC_FS) @@ -1948,29 +1938,20 @@ static int bcm_release(struct socket *sock) bcm_remove_op(op); } - list_for_each_entry_safe(op, next, &bo->rx_ops, list) { - /* - * Don't care if we're bound or not (due to netdev problems) - * can_rx_unregister() is always a save thing to do here. - */ + list_for_each_entry(op, &bo->rx_ops, list) { if (op->ifindex) { - /* - * Only remove subscriptions that had not - * been removed due to NETDEV_UNREGISTER - * in bcm_notifier() - * - * op->rx_reg_dev is a tracked reference taken - * when the subscription was registered, so it - * stays valid here even if a concurrent - * NETDEV_UNREGISTER already unlisted the device. + /* Only remove subscriptions that had not been removed + * in bcm_notify(). op->rx_reg_dev keeps the device's + * receive lists accessible. */ if (op->rx_reg_dev) bcm_rx_unreg(op->rx_reg_dev, op); - } else + } else { can_rx_unregister(net, NULL, op->can_id, REGMASK(op->can_id), bcm_rx_handler, op); - + } + cond_resched(); } synchronize_rcu(); diff --git a/net/can/isotp.c b/net/can/isotp.c index 5e1c3971ebfc..5ae55ed28d57 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1497,11 +1497,11 @@ static int isotp_release(struct socket *sock) list_del(&so->notifier); spin_unlock(&isotp_notifier_lock); - rtnl_lock(); lock_sock(sk); /* remove current filters & unregister - * tracked reference so->dev is taken at bind() time with rtnl_lock + * the tracked reference so->dev keeps the device's receive lists + * accessible, even after a namespace change */ if (so->bound && so->dev) { if (isotp_register_rxid(so)) @@ -1519,8 +1519,6 @@ static int isotp_release(struct socket *sock) so->bound = 0; so->dev = NULL; - rtnl_unlock(); - /* Always wait for a grace period before touching the timers below. * A concurrent NETDEV_UNREGISTER may have already unregistered our * filters and cleared so->bound in isotp_notify() without waiting @@ -1588,7 +1586,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l if (!addr->can_ifindex) return -ENODEV; - rtnl_lock(); + /* lock_sock() serializes the binding with isotp_notify() */ lock_sock(sk); if (so->bound) { @@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l goto out; } - dev = dev_get_by_index(net, addr->can_ifindex); + /* Publish the device inside the RCU section of the lookup: When it + * is unlisted concurrently, either isotp_notify() sees so->dev after + * synchronize_net() or the lookup fails. + */ + rcu_read_lock(); + dev = dev_get_by_index_rcu(net, addr->can_ifindex); + if (dev) { + dev_hold(dev); + WRITE_ONCE(so->dev, dev); + } + rcu_read_unlock(); if (!dev) { err = -ENODEV; goto out; @@ -1627,7 +1635,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l err = -EINVAL; goto out_put_dev; } - if (!(dev->flags & IFF_UP)) + if (!(READ_ONCE(dev->flags) & IFF_UP)) notify_enetdown = 1; ifindex = dev->ifindex; @@ -1653,15 +1661,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l * can safely reach the device later, even if a concurrent * NETDEV_UNREGISTER has already unlisted it by ifindex. */ - so->dev = dev; netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL); out_put_dev: - /* remove potential reference from dev_get_by_index() */ + /* the published so->dev is only valid for a successful bind() */ + if (err) + WRITE_ONCE(so->dev, NULL); + + /* remove reference from the device lookup */ dev_put(dev); out: release_sock(sk); - rtnl_unlock(); if (notify_enetdown) { sk->sk_err = ENETDOWN; @@ -1864,12 +1874,18 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg, if (!net_eq(dev_net(dev), sock_net(sk))) return; - if (so->dev != dev) + /* lockless fast path, see raw_notify() */ + if (READ_ONCE(so->dev) != dev) return; switch (msg) { case NETDEV_UNREGISTER: lock_sock(sk); + if (so->dev != dev) { + release_sock(sk); + break; + } + /* remove current filters & unregister */ if (so->bound) { if (isotp_register_rxid(so)) @@ -1885,7 +1901,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg, so->ifindex = 0; so->bound = 0; - so->dev = NULL; + WRITE_ONCE(so->dev, NULL); release_sock(sk); sk->sk_err = ENODEV; diff --git a/net/can/raw.c b/net/can/raw.c index dfea48768b23..0d8edfa772fb 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -85,6 +85,7 @@ struct raw_sock { struct sock sk; struct net_device *dev; netdevice_tracker dev_tracker; + struct net_device *bind_dev; /* hint for raw_notify(), no reference */ struct list_head notifier; int ifindex; unsigned int bound:1; @@ -224,6 +225,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev, int i; for (i = 0; i < count; i++) { + cond_resched(); err = can_rx_register(net, dev, filter[i].can_id, filter[i].can_mask, raw_rcv, sk, sock_i_ino(sk), sk); @@ -258,9 +260,11 @@ static void raw_disable_filters(struct net *net, struct net_device *dev, { int i; - for (i = 0; i < count; i++) + for (i = 0; i < count; i++) { can_rx_unregister(net, dev, filter[i].can_id, filter[i].can_mask, raw_rcv, sk); + cond_resched(); + } } static inline void raw_disable_errfilter(struct net *net, @@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, struct net_device *dev) { struct sock *sk = &ro->sk; + struct net_device *bind_dev; if (!net_eq(dev_net(dev), sock_net(sk))) return; - if (ro->dev != dev) + /* Lockless fast path, ro->bind_dev covers a concurrent bind(). + * Read it before ro->dev, pairs with smp_store_release() in + * raw_bind(). A stale ro->bind_dev only costs a lock_sock(). + */ + bind_dev = smp_load_acquire(&ro->bind_dev); + if (READ_ONCE(ro->dev) != dev && + (msg != NETDEV_UNREGISTER || bind_dev != dev)) return; switch (msg) { case NETDEV_UNREGISTER: lock_sock(sk); + if (ro->dev != dev) { + release_sock(sk); + break; + } + /* remove current filters & unregister */ if (ro->bound) { raw_disable_allfilters(dev_net(dev), dev, sk); @@ -326,7 +342,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, ro->ifindex = 0; ro->bound = 0; - ro->dev = NULL; + WRITE_ONCE(ro->dev, NULL); ro->count = 0; release_sock(sk); @@ -381,6 +397,7 @@ static int raw_init(struct sock *sk) ro->bound = 0; ro->ifindex = 0; ro->dev = NULL; + ro->bind_dev = NULL; /* set default filter to single entry dfilter */ ro->dfilter.can_id = 0; @@ -431,13 +448,15 @@ static int raw_release(struct socket *sock) list_del(&ro->notifier); spin_unlock(&raw_notifier_lock); - rtnl_lock(); lock_sock(sk); - /* remove current filters & unregister */ + /* remove current filters & unregister + * the tracked reference ro->dev keeps the device's receive lists + * accessible, even after a namespace change + */ if (ro->bound) { if (ro->dev) { - raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); + raw_disable_allfilters(net, ro->dev, sk); netdev_put(ro->dev, &ro->dev_tracker); } else { raw_disable_allfilters(net, NULL, sk); @@ -456,7 +475,6 @@ static int raw_release(struct socket *sock) sock->sk = NULL; release_sock(sk); - rtnl_unlock(); sock_prot_inuse_add(net, sk->sk_prot, -1); sock_put(sk); @@ -479,14 +497,25 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len if (addr->can_family != AF_CAN) return -EINVAL; - rtnl_lock(); + /* lock_sock() serializes the binding with raw_notify() */ lock_sock(sk); if (ro->bound && addr->can_ifindex == ro->ifindex) goto out; if (addr->can_ifindex) { - dev = dev_get_by_index(sock_net(sk), addr->can_ifindex); + /* Publish the device inside the RCU section of the lookup: + * When it is unlisted concurrently, either raw_notify() sees + * ro->bind_dev after synchronize_net() or the lookup fails. + */ + rcu_read_lock(); + dev = dev_get_by_index_rcu(sock_net(sk), addr->can_ifindex); + if (dev) { + dev_hold(dev); + /* pairs with smp_load_acquire() in raw_notify() */ + smp_store_release(&ro->bind_dev, dev); + } + rcu_read_unlock(); if (!dev) { err = -ENODEV; goto out; @@ -496,7 +525,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len goto out_put_dev; } - if (!(dev->flags & IFF_UP)) + if (!(READ_ONCE(dev->flags) & IFF_UP)) notify_enetdown = 1; ifindex = dev->ifindex; @@ -517,7 +546,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len if (ro->bound) { /* unregister old filters */ if (ro->dev) { - raw_disable_allfilters(dev_net(ro->dev), + raw_disable_allfilters(sock_net(sk), ro->dev, sk); /* drop reference to old ro->dev */ netdev_put(ro->dev, &ro->dev_tracker); @@ -528,16 +557,15 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len ro->ifindex = ifindex; ro->bound = 1; /* bind() ok -> hold a reference for new ro->dev */ - ro->dev = dev; + WRITE_ONCE(ro->dev, dev); netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); } out_put_dev: - /* remove potential reference from dev_get_by_index() */ + /* remove reference from the device lookup */ dev_put(dev); out: release_sock(sk); - rtnl_unlock(); if (notify_enetdown) { sk->sk_err = ENETDOWN; @@ -566,58 +594,41 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr, } static int raw_setsockopt_locked(struct socket *sock, int optname, - sockptr_t optval, unsigned int optlen) + sockptr_t optval, unsigned int optlen, + struct can_filter *filter, + struct can_filter *sfilter, int count) { struct sock *sk = sock->sk; struct raw_sock *ro = raw_sk(sk); - struct can_filter *filter = NULL; /* dyn. alloc'ed filters */ - struct can_filter sfilter; /* single filter */ struct net_device *dev = NULL; can_err_mask_t err_mask = 0; - int count = 0; int flag; int err = 0; switch (optname) { case CAN_RAW_FILTER: - if (optlen % sizeof(struct can_filter) != 0) - return -EINVAL; - - if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter)) - return -EINVAL; - - count = optlen / sizeof(struct can_filter); - - if (count > 1) { - /* filter does not fit into dfilter => alloc space */ - filter = memdup_sockptr(optval, optlen); - if (IS_ERR(filter)) - return PTR_ERR(filter); - } else if (count == 1) { - if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter))) - return -EFAULT; - } - + /* @filter has been copied by raw_setsockopt(). It is linked + * to the socket on success and freed by the caller on error. + * + * The reg_state check is only a shortcut: On a concurrent + * unregistration or namespace change raw_notify() waits for + * lock_sock() and removes the filters set here. + */ dev = ro->dev; - if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED) { - if (count > 1) - kfree(filter); + if (ro->bound && dev && + READ_ONCE(dev->reg_state) != NETREG_REGISTERED) return -ENODEV; - } if (ro->bound) { /* (try to) register the new filters */ if (count == 1) err = raw_enable_filters(sock_net(sk), dev, sk, - &sfilter, 1); + sfilter, 1); else err = raw_enable_filters(sock_net(sk), dev, sk, filter, count); - if (err) { - if (count > 1) - kfree(filter); + if (err) return err; - } /* remove old filter registrations */ raw_disable_filters(sock_net(sk), dev, sk, ro->filter, @@ -631,7 +642,7 @@ static int raw_setsockopt_locked(struct socket *sock, int optname, /* link new filters to the socket */ if (count == 1) { /* copy filter data for single filter */ - ro->dfilter = sfilter; + ro->dfilter = *sfilter; filter = &ro->dfilter; } ro->filter = filter; @@ -647,8 +658,10 @@ static int raw_setsockopt_locked(struct socket *sock, int optname, err_mask &= CAN_ERR_MASK; + /* lockless shortcut, see CAN_RAW_FILTER */ dev = ro->dev; - if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED) + if (ro->bound && dev && + READ_ONCE(dev->reg_state) != NETREG_REGISTERED) return -ENODEV; /* remove current error mask */ @@ -750,18 +763,47 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, sockptr_t optval, unsigned int optlen) { struct sock *sk = sock->sk; + struct can_filter *filter = NULL; /* dyn. alloc'ed filters */ + struct can_filter sfilter; /* single filter */ + int count = 0; int err; if (level != SOL_CAN_RAW) return -EINVAL; - rtnl_lock(); + if (optname == CAN_RAW_FILTER) { + /* Copy the filters before lock_sock(), so a stalled copy can + * not block raw_notify() waiting for lock_sock() under RTNL. + */ + if (optlen % sizeof(struct can_filter) != 0) + return -EINVAL; + + if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter)) + return -EINVAL; + + count = optlen / sizeof(struct can_filter); + + if (count > 1) { + /* filter does not fit into dfilter => alloc space */ + filter = memdup_sockptr(optval, optlen); + if (IS_ERR(filter)) + return PTR_ERR(filter); + } else if (count == 1) { + if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter))) + return -EFAULT; + } + } + lock_sock(sk); - err = raw_setsockopt_locked(sock, optname, optval, optlen); + err = raw_setsockopt_locked(sock, optname, optval, optlen, + filter, &sfilter, count); release_sock(sk); - rtnl_unlock(); + + /* on error free the filters allocated for CAN_RAW_FILTER above */ + if (err && count > 1) + kfree(filter); return err; } -- 2.53.0