can: bcm: add locking for bcm tx job updates
Subsystems (2)
Oliver Hartkopp
maintainerRFC PATCH 1/1 can: bcm: add locking for bcm tx job updates
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 <[email protected]>
Fixes bf74aa86e111 ("can: bcm: switch timer to HRTIMER_MODE_SOFT and remove hrtimer_tasklet")
Reported-by [email protected]
Signed-off-by Oliver Hartkopp <[email protected]>
--- net/can/bcm.c | 87 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 32 deletions(-)
net/can/bcm.c +55 -32
@@ -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 ;
Signature · 1 line
2.30.2