From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mo4-p03-ob.smtp.rzone.de (mo4-p03-ob.smtp.rzone.de [81.169.146.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C8B973CD8BF for ; Fri, 24 Jul 2026 08:43:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=81.169.146.172 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784882633; cv=pass; b=EcVbnYZ1tAzrZGqbh9Ztb+M4Y4c/RUqaB2sxDNvNST2v2YQQmyxIvIAY5+LhhBOkcsvOiYe8cJI7yk336/u/fkxg3lgf5SZ+vf1hx6yeq4QICWxrwpE1YAZv0S0H4xwwpucuY0kI3oRmhPsvtVRBo1kGmtHC4D+8hAH93h0U/FM= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784882633; c=relaxed/simple; bh=6UC52UkrqzHUqwRLEj5pwxf0vf2GQIN5NJmVHrkEDAE=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=EyFELJ8SzcUbK6L4VLr2Jqio7pNNNeugKsMyhqPyL2yx9UjIZCeyyjDhGFztk7CT/FCmBpRfLpz85ywu5aGfZhs6aRoMn3046zCkO2jRT1Vcre+4Swk+Zl8Yy7Y1kW/iarba+8e7K6au6zPo1mKldTo4fut1NY8C5xbeecgAx0g= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=hartkopp.net; spf=fail smtp.mailfrom=hartkopp.net; dkim=pass (2048-bit key) header.d=hartkopp.net header.i=@hartkopp.net header.b=oGP+oDV1; dkim=permerror (0-bit key) header.d=hartkopp.net header.i=@hartkopp.net header.b=fAZko7MA; arc=pass smtp.client-ip=81.169.146.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=hartkopp.net Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=hartkopp.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=hartkopp.net header.i=@hartkopp.net header.b="oGP+oDV1"; dkim=permerror (0-bit key) header.d=hartkopp.net header.i=@hartkopp.net header.b="fAZko7MA" ARC-Seal: i=1; a=rsa-sha256; t=1784882449; cv=none; d=strato.com; s=strato-dkim-0002; b=WcbPus5xPQx/X+ivTxO7Z1m/g7hEeUZnijlCxf85naWRSNfQd4vvqZ7HDjSL0tekp6 4e8J/ZakOJVC9AqJbiefkBzAkkV6A+xMSjDnDPM6RpRdJt0O5/fxKm/YuQjp7aBzFzU5 rdpxgLYzRrNmnkLyyFVyzfikV5bwR4rYzX2CnMwNUYn+ElAr7QmGtLJ0TSx8QN5v1R5Q qWxBLBEX1NODi7hj5H/bRUZZvLhg4UXQJn8orj76Bm/6vCt1zJYmKzaY0h3PYsPbx3GH G9P0lhj6HkZ/AJX29qXsXDpXaf0Yu1TN7Uy8Q1VeNvuZCNA3vcoFZCvPBLmJ/EGPy3uX Ar4Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; t=1784882449; s=strato-dkim-0002; d=strato.com; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Cc:Date: From:Subject:Sender; bh=HnYGgEXMSS1Lid4uQT3T8NdJ63CYqvH6WypssSF6Nks=; b=BHRlCyv2dlwndc4tSvHeU7M8k2WJdCIfZfezFqIyx4hjb+vhKJRLJoWvdivzDuDyNa clF/39JjLhQCxHQmTsla71jRliqXwbxmbf1jlZ48w1GmAGmrSQxXzExj0/lq7i1wGH1g +r1rNENmeEeHjoLXhczJBPbRP/jbV0npW9NEAEWjdQxY75lNHP3CgYWWPFkkG8Ppu+rm KfLgR5iJIVD13LH9Wl7/j3vgIN8C8UPzloztGZwOomt/dOcT46qJ5G1TBxij2TrSQE54 GpQG07xDxi8lWIum9IBAVurkwtqppgjnL8UE2+Z06mZ3mQkM+Si8bYDcxliKCUbVnToR hIpQ== ARC-Authentication-Results: i=1; strato.com; arc=none; dkim=none X-RZG-CLASS-ID: mo03 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1784882449; s=strato-dkim-0002; d=hartkopp.net; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Cc:Date: From:Subject:Sender; bh=HnYGgEXMSS1Lid4uQT3T8NdJ63CYqvH6WypssSF6Nks=; b=oGP+oDV1odMuL7W1a5KtP3HRySnrwh4TyLfZal+X3ytQj91UM0YwuYG20OF0VeIee2 XZhSOZM2CWCR1whECQ5rzA5EoLnKjTRJrUwkGcfuzht/KcFWGk2vYethgkaEwV/Audgu bq43qBAT53Q2QpE2qYCAZl0myYvFtdVbiBmfTRoxKhNkZnv5kSVV+TgujVQO7oHUJ9pd N6CGYJxQ4gofcexWS4yHgHfvWilhpw5oE/K4Ru7bfF8lP11+4+Vm0MsGCYXKiu/UvRtV Nkvk6HVJ0meejZFbeIgpNp/a0dCFLaVhuCAi1nRn+tCQd44h3kGmj+fVP8a7SD9tnvfW 1sCA== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; t=1784882449; s=strato-dkim-0003; d=hartkopp.net; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Cc:Date: From:Subject:Sender; bh=HnYGgEXMSS1Lid4uQT3T8NdJ63CYqvH6WypssSF6Nks=; b=fAZko7MA5ClnswDBrHT7CXYh8rV0PBjJ8n2iKg0sKaGBUQ8K+4eStS3l+C7Ie/tiII dAJEqaYFJ+tkJyKOTCAQ== X-RZG-AUTH: ":P2MHfkW8eP4Mre39l357AZT/I7AY/7nT2yrDxb8mjH4JKvMdQv2tRkI16oOSW1Ti/f4PoH8=" Received: from vivo.lan by smtp.strato.de (RZmta 55.5.6 SBL|AUTH) with ESMTPSA id K5281926O8emrWr (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Fri, 24 Jul 2026 10:40:48 +0200 (CEST) From: Oliver Hartkopp To: stable@vger.kernel.org Cc: Oliver Hartkopp , Marc Kleine-Budde Subject: [PATCH 1/8] can: bcm: add locking when updating filter and timer values Date: Fri, 24 Jul 2026 10:40:35 +0200 Message-ID: <20260724084042.12984-2-socketcan@hartkopp.net> X-Mailer: git-send-email 2.53.0 In-Reply-To: <2026072011-atypical-tapping-9d9e@gregkh> References: <2026072011-atypical-tapping-9d9e@gregkh> Precedence: bulk X-Mailing-List: stable@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="us-ascii" commit 749179c2e25b95d22499ed29096b3e02d6dfd2b4 upstream. KCSAN detected a simultaneous access to timer values that can be overwritten in bcm_rx_setup() when updating timer and filter content while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler() run concurrently on incoming CAN traffic. Protect the timer (ival1/ival2/kt_ival1/kt_ival2/kt_lastmsg) and filter (nframes/flags/frames/last_frames) updates in bcm_rx_setup() with a new per-op bcm_rx_update_lock, taken with the matching scope in the RX handlers. memcpy_from_msg() is staged into a temporary buffer before the lock is taken, since it can sleep and must not run under a spinlock. hrtimer_cancel() is always called without bcm_rx_update_lock held, since bcm_rx_timeout_handler()/bcm_rx_thr_handler() take the same lock and a running callback would otherwise deadlock against the canceller. Also close a related race: bcm_rx_setup() cleared the RTR flag in the stored reply frame's can_id as a separate, unprotected step after the frame content was already installed, so a concurrent bcm_rx_handler() could transmit a stale reply with CAN_RTR_FLAG still set. Fold that normalization into the initial frame preparation instead (on the staged buffer for updates, directly on op->frames pre-registration for new ops), so the installed frame is always atomically self-consistent. bcm_rx_handler()'s RX_RTR_FRAME check now takes a lock-protected snapshot of op->flags before deciding whether to call bcm_can_tx(), but does not hold the lock across that call. Also take a lock-protected snapshot of the currframe in bcm_can_tx() to avoid partly overwrites by content updates in bcm_tx_setup(). Finally check if a TX_RESET_MULTI_IDX/SETTIMER might have reset op->currframe between the two locked sections in bcm_can_tx(). Omit calling hrtimer_forward() with zero interval in bcm_rx_thr_handler(). kt_ival2 may have been concurrently cleared by bcm_rx_setup() before it cancels this timer, so check kt_ival2 inside the bcm_rx_update_lock. Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates") Reported-by: syzbot+75e5e4ae00c3b4bb544e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/ Signed-off-by: Oliver Hartkopp Link: https://patch.msgid.link/20260714-bcm_fixes-v15-3-562f7e3e42da@hartkopp.net Cc: stable@kernel.org Signed-off-by: Marc Kleine-Budde Signed-off-by: Oliver Hartkopp --- net/can/bcm.c | 176 ++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 133 insertions(+), 43 deletions(-) diff --git a/net/can/bcm.c b/net/can/bcm.c index 149d4df..50fc81b 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -128,6 +128,7 @@ struct bcm_op { struct sock *sk; struct net_device *rx_reg_dev; spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */ + spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */ }; struct bcm_sock { @@ -292,21 +293,27 @@ static int bcm_proc_show(struct seq_file *m, void *v) * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface * of the given bcm tx op */ -static void bcm_can_tx(struct bcm_op *op) +static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf) { struct sk_buff *skb; struct net_device *dev; - struct canfd_frame *cf; + struct canfd_frame cframe; + bool cyclic = !cf; + unsigned int idx = 0; int err; /* no target device? => exit */ if (!op->ifindex) return; - /* read currframe under lock protection */ - spin_lock_bh(&op->bcm_tx_lock); - cf = op->frames + op->cfsiz * op->currframe; - spin_unlock_bh(&op->bcm_tx_lock); + if (cyclic) { + /* read currframe under lock protection */ + spin_lock_bh(&op->bcm_tx_lock); + idx = op->currframe; + memcpy(&cframe, op->frames + op->cfsiz * idx, op->cfsiz); + cf = &cframe; + spin_unlock_bh(&op->bcm_tx_lock); + } dev = dev_get_by_index(sock_net(op->sk), op->ifindex); if (!dev) { @@ -335,14 +342,20 @@ static void bcm_can_tx(struct bcm_op *op) if (!err) op->frames_abs++; - op->currframe++; + /* only advance the cyclic sequence if nothing reset currframe while + * we were sending - a concurrent TX_RESET_MULTI_IDX means this + * frame's bookkeeping belongs to a sequence that no longer exists + */ + if (!cyclic || op->currframe == idx) { + op->currframe++; - /* reached last frame? */ - if (op->currframe >= op->nframes) - op->currframe = 0; + /* reached last frame? */ + if (op->currframe >= op->nframes) + op->currframe = 0; - if (op->count > 0) - op->count--; + if (op->count > 0) + op->count--; + } spin_unlock_bh(&op->bcm_tx_lock); out: @@ -455,7 +468,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer) struct bcm_msg_head msg_head; if (op->kt_ival1 && (op->count > 0)) { - bcm_can_tx(op); + bcm_can_tx(op, NULL); if (!op->count && (op->flags & TX_COUNTEVT)) { /* create notification to user */ @@ -472,7 +485,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer) } } else if (op->kt_ival2) { - bcm_can_tx(op); + bcm_can_tx(op, NULL); } return bcm_tx_set_expiry(op, &op->timer) ? @@ -616,6 +629,8 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer) struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer); struct bcm_msg_head msg_head; + spin_lock_bh(&op->bcm_rx_update_lock); + /* if user wants to be informed, when cyclic CAN-Messages come back */ if ((op->flags & RX_ANNOUNCE_RESUME) && op->last_frames) { /* clear received CAN frames to indicate 'nothing received' */ @@ -632,6 +647,8 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer) msg_head.can_id = op->can_id; msg_head.nframes = 0; + spin_unlock_bh(&op->bcm_rx_update_lock); + bcm_send_to_user(op, &msg_head, NULL, 0); return HRTIMER_NORESTART; @@ -680,15 +697,26 @@ static int bcm_rx_thr_flush(struct bcm_op *op) static enum hrtimer_restart bcm_rx_thr_handler(struct hrtimer *hrtimer) { struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer); + enum hrtimer_restart ret; - if (bcm_rx_thr_flush(op)) { + spin_lock_bh(&op->bcm_rx_update_lock); + + /* kt_ival2 may have been concurrently cleared by bcm_rx_setup() + * before it cancels this timer - never forward with a zero + * interval in that case. + */ + if (bcm_rx_thr_flush(op) && op->kt_ival2) { hrtimer_forward_now(hrtimer, op->kt_ival2); - return HRTIMER_RESTART; + ret = HRTIMER_RESTART; } else { /* rearm throttle handling */ op->kt_lastmsg = 0; - return HRTIMER_NORESTART; + ret = HRTIMER_NORESTART; } + + spin_unlock_bh(&op->bcm_rx_update_lock); + + return ret; } /* @@ -698,8 +726,10 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data) { struct bcm_op *op = (struct bcm_op *)data; const struct canfd_frame *rxframe = (struct canfd_frame *)skb->data; + struct canfd_frame rtrframe; unsigned int i; unsigned char traffic_flags; + bool rtr_frame; if (op->can_id != rxframe->can_id) return; @@ -723,9 +753,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data) /* update statistics */ op->frames_abs++; - if (op->flags & RX_RTR_FRAME) { + /* snapshot the flag under lock: op->flags/op->frames may be updated + * concurrently by bcm_rx_setup(). + */ + spin_lock_bh(&op->bcm_rx_update_lock); + rtr_frame = op->flags & RX_RTR_FRAME; + if (rtr_frame) + memcpy(&rtrframe, op->frames, op->cfsiz); + spin_unlock_bh(&op->bcm_rx_update_lock); + + if (rtr_frame) { /* send reply for RTR-request (placed in op->frames[0]) */ - bcm_can_tx(op); + bcm_can_tx(op, &rtrframe); return; } @@ -737,6 +776,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data) traffic_flags |= RX_OWN; } + spin_lock_bh(&op->bcm_rx_update_lock); + if (op->flags & RX_FILTER_ID) { /* the easiest case */ bcm_rx_update_and_send(op, op->last_frames, rxframe, @@ -772,6 +813,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data) rx_starttimer: bcm_rx_starttimer(op); + + spin_unlock_bh(&op->bcm_rx_update_lock); } /* @@ -1115,7 +1158,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, list_add_rcu(&op->list, &bo->tx_ops); if (op->flags & TX_ANNOUNCE) - bcm_can_tx(op); + bcm_can_tx(op, NULL); if (op->flags & STARTTIMER) bcm_tx_start_timer(op); @@ -1129,6 +1172,24 @@ free_op: return err; } +static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head, + struct bcm_op *op, void *new_frames) +{ + /* funny feature in RX(!)_SETUP only for RTR-mode: + * copy can_id into frame BUT without RTR-flag to + * prevent a full-load-loopback-test ... ;-] + * normalize this on the staged buffer, before it is + * ever installed into op->frames. + */ + if (msg_head->flags & RX_RTR_FRAME) { + struct canfd_frame *frame0 = new_frames; + + if ((msg_head->flags & TX_CP_CAN_ID) || + frame0->can_id == op->can_id) + frame0->can_id = op->can_id & ~CAN_RTR_FLAG; + } +} + /* * bcm_rx_setup - create or update a bcm rx op (for bcm_sendmsg) */ @@ -1163,6 +1224,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, /* check the given can_id */ op = bcm_find_op(&bo->rx_ops, msg_head, ifindex); if (op) { + void *new_frames = NULL; + /* update existing BCM operation */ /* @@ -1174,19 +1237,48 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, return -E2BIG; if (msg_head->nframes) { - /* update CAN frames content */ - err = memcpy_from_msg(op->frames, msg, + /* get new CAN frames content before locking */ + new_frames = kmalloc(msg_head->nframes * op->cfsiz, + GFP_KERNEL); + if (!new_frames) + return -ENOMEM; + + err = memcpy_from_msg(new_frames, msg, msg_head->nframes * op->cfsiz); - if (err < 0) + if (err < 0) { + kfree(new_frames); return err; + } - /* clear last_frames to indicate 'nothing received' */ - memset(op->last_frames, 0, msg_head->nframes * op->cfsiz); + bcm_rx_setup_rtr_check(msg_head, op, new_frames); } + spin_lock_bh(&op->bcm_rx_update_lock); op->nframes = msg_head->nframes; op->flags = msg_head->flags; + if (msg_head->nframes) { + /* update CAN frames content */ + memcpy(op->frames, new_frames, + msg_head->nframes * op->cfsiz); + + /* clear last_frames to indicate 'nothing received' */ + memset(op->last_frames, 0, + msg_head->nframes * op->cfsiz); + } + + if (msg_head->flags & SETTIMER) { + op->ival1 = msg_head->ival1; + op->ival2 = msg_head->ival2; + op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1); + op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2); + op->kt_lastmsg = 0; + } + spin_unlock_bh(&op->bcm_rx_update_lock); + + /* free temporary frames / kfree(NULL) is safe */ + kfree(new_frames); + /* Only an update -> do not call can_rx_register() */ do_rx_register = 0; @@ -1197,6 +1289,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, return -ENOMEM; spin_lock_init(&op->bcm_tx_lock); + spin_lock_init(&op->bcm_rx_update_lock); op->can_id = msg_head->can_id; op->nframes = msg_head->nframes; op->cfsiz = CFSIZ(msg_head->flags); @@ -1238,6 +1331,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, kfree(op); return err; } + + bcm_rx_setup_rtr_check(msg_head, op, op->frames); } /* bcm_can_tx / bcm_tx_timeout_handler needs this */ @@ -1265,29 +1360,22 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, /* check flags */ if (op->flags & RX_RTR_FRAME) { - struct canfd_frame *frame0 = op->frames; - /* no timers in RTR-mode */ hrtimer_cancel(&op->thrtimer); hrtimer_cancel(&op->timer); - - /* - * funny feature in RX(!)_SETUP only for RTR-mode: - * copy can_id into frame BUT without RTR-flag to - * prevent a full-load-loopback-test ... ;-] - */ - if ((op->flags & TX_CP_CAN_ID) || - (frame0->can_id == op->can_id)) - frame0->can_id = op->can_id & ~CAN_RTR_FLAG; - } else { if (op->flags & SETTIMER) { - /* set timer value */ - op->ival1 = msg_head->ival1; - op->ival2 = msg_head->ival2; - op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1); - op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2); + /* set timers (locked) for newly created op */ + if (do_rx_register) { + spin_lock_bh(&op->bcm_rx_update_lock); + op->ival1 = msg_head->ival1; + op->ival2 = msg_head->ival2; + op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1); + op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2); + op->kt_lastmsg = 0; + spin_unlock_bh(&op->bcm_rx_update_lock); + } /* disable an active timer due to zero value? */ if (!op->kt_ival1) @@ -1297,9 +1385,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, * In any case cancel the throttle timer, flush * potentially blocked msgs and reset throttle handling */ - op->kt_lastmsg = 0; hrtimer_cancel(&op->thrtimer); + + spin_lock_bh(&op->bcm_rx_update_lock); bcm_rx_thr_flush(op); + spin_unlock_bh(&op->bcm_rx_update_lock); } if ((op->flags & STARTTIMER) && op->kt_ival1) -- 2.43.0