can: bcm: add locking for bcm_op runtime updates
Subsystems (2)
Oliver Hartkopp
maintainerPATCH 1/2 can: bcm: add locking for bcm_op runtime updates
The CAN broadcast manager (CAN BCM) can send a sequence of CAN frames via hrtimer. The content and also the length of the sequence can be changed resp reduced at runtime where the 'currframe' counter is then set to zero.
Although this appeared to be a safe operation the updates of 'currframe' can be triggered from user space and hrtimer context in bcm_can_tx(). Anderson Nascimento created a proof of concept that triggered a KASAN slab-out-of-bounds read access which can be prevented with a spin_lock_bh.
At the rework of bcm_can_tx() the 'count' variable has been moved into the protected section as this variable can be modified from both contexts too.
--- net/can/bcm.c | 66 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 21 deletions(-)
net/can/bcm.c +45 -21
@@ -56,10 +56,11 @@#include <linux/can.h> #include <linux/can/core.h> #include <linux/can/skb.h> #include <linux/can/bcm.h> #include <linux/slab.h> +#include <linux/spinlock.h> #include <net/sock.h> #include <net/net_namespace.h> /* * To send multiple CAN frame content within TX_SETUP or to filter
@@ -120,10 +121,11 @@ struct bcm_op {void *last_frames;struct canfd_frame sframe;struct canfd_frame last_sframe;struct sock *sk;struct net_device *rx_reg_dev; +spinlock_t bcm_tx_lock;/* protect currframe/count in runtime updates */ };struct bcm_sock {struct sock sk;int bound;
@@ -283,17 +285,22 @@ static int bcm_proc_show(struct seq_file *m, void *v)*/ static void bcm_can_tx (struct bcm_op *op) {struct sk_buff *skb;struct net_device *dev; -struct canfd_frame *cf = op->frames + op->cfsiz * op->currframe; +struct canfd_frame *cf;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); + dev =dev_get_by_index (sock_net (op->sk), op->ifindex);if (!dev) {/* RFC: should this bcm_op remove itself here? */ return ; }
@@ -310,18 +317,27 @@ static void bcm_can_tx(struct bcm_op *op)/* send with loopback */ skb->dev = dev;can_skb_set_owner (skb, op->sk); err =can_send (skb,1 ); + +/* update currframe and count under lock protection */ +spin_lock_bh (&op->bcm_tx_lock); +if (!err) op->frames_abs++; op->currframe++;/* reached last frame? */ if (op->currframe >= op->nframes) op->currframe =0 ; + +if (op->count >0 ) + op->count--; + +spin_unlock_bh (&op->bcm_tx_lock); out:dev_put (dev); }/*
@@ -428,11 +444,11 @@ 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;if (op->kt_ival1 && (op->count >0 )) { - op->count--; +bcm_can_tx (op);if (!op->count && (op->flags &TX_COUNTEVT )) {/* create notification to user */ memset (&msg_head,0 ,sizeof (msg_head)); msg_head.opcode =TX_EXPIRED ;
@@ -443,11 +459,10 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer) msg_head.can_id = op->can_id; msg_head.nframes =0 ;bcm_send_to_user (op, &msg_head,NULL ,0 ); } -bcm_can_tx (op); }else if (op->kt_ival2) {bcm_can_tx (op); }
@@ -954,20 +969,46 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, cf->can_id = msg_head->can_id; } } op->flags = msg_head->flags; +/* only lock for unlikely count/nframes/currframe changes */ +if (op->nframes != msg_head->nframes || + op->flags &TX_RESET_MULTI_IDX || + op->flags &SETTIMER ) { + +spin_lock_bh (&op->bcm_tx_lock); + +if (op->nframes != msg_head->nframes || + op->flags &TX_RESET_MULTI_IDX ) { +/* potentially update changed nframes */ + op->nframes = msg_head->nframes; +/* restart multiple frame transmission */ + op->currframe =0 ; + } + +if (op->flags &SETTIMER ) + op->count = msg_head->count; + +spin_unlock_bh (&op->bcm_tx_lock); + } + }else {/* insert new BCM operation for the given can_id */ op =kzalloc (OPSIZ ,GFP_KERNEL );if (!op)return -ENOMEM ; +spin_lock_init (&op->bcm_tx_lock); op->can_id = msg_head->can_id; op->cfsiz =CFSIZ (msg_head->flags); op->flags = msg_head->flags; + op->nframes = msg_head->nframes; + +if (op->flags &SETTIMER ) + op->count = msg_head->count;/* create array for CAN frames and copy the data */ if (msg_head->nframes >1 ) { op->frames = kmalloc_array(msg_head->nframes, op->cfsiz,
@@ -1021,26 +1062,12 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,/* 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 ; - } - -/* check flags */ - -if (op->flags &TX_RESET_MULTI_IDX ) { -/* start multiple frame transmission with index 0 */ - op->currframe =0 ; - } -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);
@@ -1053,15 +1080,12 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,hrtimer_cancel (&op->timer);/* spec: send CAN frame when starting timer */ op->flags |=TX_ANNOUNCE ; } -if (op->flags &TX_ANNOUNCE ) { +if (op->flags &TX_ANNOUNCE )bcm_can_tx (op); -if (op->count) - op->count--; - }if (op->flags &STARTTIMER )bcm_tx_start_timer (op);return msg_head->nframes * op->cfsiz +MHSIZ ;
Signature · 1 line
2.47.2
Oliver Hartkopp
maintainerPATCH 2/2 can: bcm: add missing rcu read protection for procfs content
When the procfs content is generated for a bcm_op which is in the process to be removed the procfs output might show unreliable data (UAF).
As the removal of bcm_op's is already implemented with rcu handling this patch adds the missing rcu_read_lock() and makes sure the list entries are properly removed under rcu protection.
--- net/can/bcm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
net/can/bcm.c +9 -4
@@ -217,11 +217,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)seq_printf (m," / bo %pK" , bo);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" ); -list_for_each_entry (op, &bo->rx_ops, list) { +rcu_read_lock (); + +list_for_each_entry_rcu (op, &bo->rx_ops, list) {unsigned long reduction;/* print only active entries & prevent division by zero */ if (!op->frames_abs)
@@ -273,10 +275,13 @@ static int bcm_proc_show(struct seq_file *m, void *v) (long long)ktime_to_us(op->kt_ival2)); seq_printf(m, "# sent %ld\n", op->frames_abs); }seq_putc (m,'\n' ); + +rcu_read_unlock (); +return 0 ; }#endif /* CONFIG_PROC_FS */ /*
@@ -856,11 +861,11 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,can_rx_unregister (sock_net (op->sk),NULL , op->can_id,REGMASK (op->can_id), bcm_rx_handler, op); -list_del (&op->list); +list_del_rcu (&op->list);bcm_remove_op (op);return 1 ;/* done */ } }
@@ -876,11 +881,11 @@ static int bcm_delete_tx_op(struct list_head *ops, struct bcm_msg_head *mh,struct bcm_op *op, *n;list_for_each_entry_safe (op, n, ops, list) {if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) && (op->flags &CAN_FD_FRAME ) == (mh->flags &CAN_FD_FRAME )) { -list_del (&op->list); +list_del_rcu (&op->list);bcm_remove_op (op);return 1 ;/* done */ } }
@@ -1294,11 +1299,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, err =can_rx_register (sock_net (sk),NULL , op->can_id,REGMASK (op->can_id), bcm_rx_handler, op,"bcm" , sk);if (err) {/* this bcm rx op is broken -> remove it */ -list_del (&op->list); +list_del_rcu (&op->list);bcm_remove_op (op);return err; } }
Signature · 1 line
2.47.2
Marc Kleine-Budde
maintainerRe: [PATCH 2/2] can: bcm: add missing rcu read protection for procfs content
Quoted 7 lines · Oliver Hartkopp
When the procfs content is generated for a
bcm_opwhich is in the process to be removed the procfs output might show unreliable data (UAF).As the removal of bcm_op's is already implemented with rcu handling this patch adds the missing
rcu_read_lock()and makes sure the list entries are properly removed under rcu protection.
Quoted 5 lines
Fixes f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()") # >= 5.4Reported-by Anderson Nascimento <[email protected]>Suggested-by Anderson Nascimento <[email protected]>Tested-by Anderson Nascimento <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>
Applied and added stable on Cc and moved the # >= 5.4 there.
regards,
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Quoted 13 lines · Oliver Hartkopp
The CAN broadcast manager (CAN BCM) can send a sequence of CAN frames via hrtimer. The content and also the length of the sequence can be changed resp reduced at runtime where the 'currframe' counter is then set to zero.
Although this appeared to be a safe operation the updates of 'currframe' can be triggered from user space and hrtimer context in
bcm_can_tx(). Anderson Nascimento created a proof of concept that triggered a KASAN slab-out-of-bounds read access which can be prevented with aspin_lock_bh.At the rework of
bcm_can_tx()the 'count' variable has been moved into the protected section as this variable can be modified from both contexts too.
Quoted 5 lines
Fixes ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")Reported-by Anderson Nascimento <[email protected]>Tested-by Anderson Nascimento <[email protected]>Reviewed-by Marc Kleine-Budde <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>
Applied to linux-can and added stable on Cc.
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |