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@xxxxxxxxxx>
Fixes: bf74aa86e111 ("can: bcm: switch timer to HRTIMER_MODE_SOFT and remove hrtimer_tasklet")
Reported-by: syzbot+e1786f049e71693263bf@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
---
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
