From:   Oliver Hartkopp <socketcan@hartkopp.net>
To:     linux-can@vger.kernel.org
Cc:     Oliver Hartkopp <socketcan@hartkopp.net>,
        Dmitry Vyukov <dvyukov@google.com>,
        syzbot+e1786f049e71693263bf@syzkaller.appspotmail.com
Subject: [RFC PATCH] can: bcm: add locking for bcm tx job updates
Date:   Thu, 11 May 2023 19:46:44 +0200
Message-Id: <20230511174644.8849-1-socketcan@hartkopp.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Syzbot detected a concurrent write of op->currframe which is modified in
bcm_can_tx() which can be triggerd from user context in bcm_tx_setup()
and from the soft hrtimer callback bcm_tx_timeout_handler().

This patch reorders the code to minimize the potential concurrent code
section in bcm_tx_setup() and bcm_tx_timeout_handler() which modifies
op->currframe, op->count and executes bcm_can_tx(). This minimized code
section is then protected with spin_lock_bh().

The referenced commit in the "Fixes:" tag did not really cause the issue
but is the first commit where the fix could be reasonably backported.

Cc: Dmitry Vyukov <dvyukov@google.com>
Fixes: bf74aa86e111 ("can: bcm: switch timer to HRTIMER_MODE_SOFT and remove hrtimer_tasklet")
Reported-by: syzbot+e1786f049e71693263bf@syzkaller.appspotmail.com
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 87 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 32 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index a962ec2b8ba5..34edd258f086 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -41,10 +41,11 @@
  */
 
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/spinlock.h>
 #include <linux/hrtimer.h>
 #include <linux/list.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/uio.h>
@@ -99,10 +100,11 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
 }
 
 struct bcm_op {
 	struct list_head list;
 	struct rcu_head rcu;
+	spinlock_t tx_setup_lock; /* protect setup/hrtimer access */
 	int ifindex;
 	canid_t can_id;
 	u32 flags;
 	unsigned long frames_abs, frames_filtered;
 	struct bcm_timeval ival1, ival2;
@@ -400,33 +402,41 @@ static void bcm_tx_start_timer(struct bcm_op *op)
 /* bcm_tx_timeout_handler - performs cyclic CAN frame transmissions */
 static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
 {
 	struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
 	struct bcm_msg_head msg_head;
+	bool send_notification = false;
+
+	spin_lock_bh(&op->tx_setup_lock);
 
 	if (op->kt_ival1 && (op->count > 0)) {
 		op->count--;
-		if (!op->count && (op->flags & TX_COUNTEVT)) {
-
-			/* create notification to user */
-			memset(&msg_head, 0, sizeof(msg_head));
-			msg_head.opcode  = TX_EXPIRED;
-			msg_head.flags   = op->flags;
-			msg_head.count   = op->count;
-			msg_head.ival1   = op->ival1;
-			msg_head.ival2   = op->ival2;
-			msg_head.can_id  = op->can_id;
-			msg_head.nframes = 0;
-
-			bcm_send_to_user(op, &msg_head, NULL, 0);
-		}
+		if (!op->count && (op->flags & TX_COUNTEVT))
+			send_notification = true;
+
 		bcm_can_tx(op);
 
 	} else if (op->kt_ival2) {
 		bcm_can_tx(op);
 	}
 
+	spin_unlock_bh(&op->tx_setup_lock);
+
+	if (send_notification) {
+		/* create notification to user */
+		memset(&msg_head, 0, sizeof(msg_head));
+		msg_head.opcode  = TX_EXPIRED;
+		msg_head.flags   = op->flags;
+		msg_head.count   = 0; /* zero at time of check */
+		msg_head.ival1   = op->ival1;
+		msg_head.ival2   = op->ival2;
+		msg_head.can_id  = op->can_id;
+		msg_head.nframes = 0;
+
+		bcm_send_to_user(op, &msg_head, NULL, 0);
+	}
+
 	return bcm_tx_set_expiry(op, &op->timer) ?
 		HRTIMER_RESTART : HRTIMER_NORESTART;
 }
 
 /*
@@ -860,10 +870,12 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			int ifindex, struct sock *sk)
 {
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 	struct canfd_frame *cf;
+	ktime_t setup_ival1 = ktime_set(0, 0);
+	ktime_t setup_ival2 = ktime_set(0, 0);
 	unsigned int i;
 	int err;
 
 	/* we need a real device to send frames */
 	if (!ifindex)
@@ -975,53 +987,64 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 		/* currently unused in tx_ops */
 		hrtimer_init(&op->thrtimer, CLOCK_MONOTONIC,
 			     HRTIMER_MODE_REL_SOFT);
 
+		spin_lock_init(&op->tx_setup_lock);
+
 		/* add this bcm_op to the list of the tx_ops */
 		list_add(&op->list, &bo->tx_ops);
 
 	} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */
 
-	if (op->nframes != msg_head->nframes) {
-		op->nframes   = msg_head->nframes;
-		/* start multiple frame transmission with index 0 */
-		op->currframe = 0;
-	}
+	if (op->flags & SETTIMER) {
+		setup_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
+		setup_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
 
-	/* check flags */
+		/* disable an active timer due to zero values? */
+		if (!setup_ival1 && !setup_ival2)
+			hrtimer_cancel(&op->timer);
+	}
 
-	if (op->flags & TX_RESET_MULTI_IDX) {
-		/* start multiple frame transmission with index 0 */
-		op->currframe = 0;
+	if (op->flags & STARTTIMER) {
+		hrtimer_cancel(&op->timer);
+		/* spec: send CAN frame when starting timer */
+		op->flags |= TX_ANNOUNCE;
 	}
 
+	spin_lock_bh(&op->tx_setup_lock);
+
 	if (op->flags & SETTIMER) {
 		/* set timer values */
 		op->count = msg_head->count;
 		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_ival1 = setup_ival1;
+		op->kt_ival2 = setup_ival2;
+	}
 
-		/* disable an active timer due to zero values? */
-		if (!op->kt_ival1 && !op->kt_ival2)
-			hrtimer_cancel(&op->timer);
+	if (op->nframes != msg_head->nframes) {
+		op->nframes = msg_head->nframes;
+		/* start multiple frame transmission with index 0 */
+		op->currframe = 0;
 	}
 
-	if (op->flags & STARTTIMER) {
-		hrtimer_cancel(&op->timer);
-		/* spec: send CAN frame when starting timer */
-		op->flags |= TX_ANNOUNCE;
+	/* check flags */
+
+	if (op->flags & TX_RESET_MULTI_IDX) {
+		/* start multiple frame transmission with index 0 */
+		op->currframe = 0;
 	}
 
 	if (op->flags & TX_ANNOUNCE) {
 		bcm_can_tx(op);
 		if (op->count)
 			op->count--;
 	}
 
+	spin_unlock_bh(&op->tx_setup_lock);
+
 	if (op->flags & STARTTIMER)
 		bcm_tx_start_timer(op);
 
 	return msg_head->nframes * op->cfsiz + MHSIZ;
 
-- 
2.30.2