can: bcm: extend bcm_tx_lock usage for data and timer updates

Stage new CAN frame content for an existing tx op into a kmalloc()'d
buffer and validate it there, mirroring the approach already used in
bcm_rx_setup(). Only copy the validated data into op->frames while
holding op->bcm_tx_lock, so bcm_can_tx() and bcm_tx_timeout_handler()
can no longer observe a partially updated or unvalidated frame.

Add a missing error path for memcpy_from_msg() when copying CAN frame
data from userspace.

Also move the kt_ival1/kt_ival2/ival1/ival2 updates in bcm_tx_setup()
under op->bcm_tx_lock, and read kt_ival1/kt_ival2/count under the same
lock in bcm_tx_set_expiry() and bcm_tx_timeout_handler(), closing the
torn 64-bit ktime_t read on 32-bit platforms.

Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260714-bcm_fixes-v15-6-562f7e3e42da@hartkopp.net
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 5c1e83eeb4ff3dda445ab3db4e207119da2ad0a4..68a62f605432f5ed6e273280cf4944af86e1aa0c 100644 (file)
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -128,7 +128,7 @@ struct bcm_op {
        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 */
+       spinlock_t bcm_tx_lock; /* protect tx data and timer updates */
        spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
 };
 
@@ -472,12 +472,18 @@ static bool bcm_tx_set_expiry(struct bcm_op *op, struct hrtimer *hrt)
 {
        ktime_t ival;
 
+       spin_lock_bh(&op->bcm_tx_lock);
+
        if (op->kt_ival1 && op->count)
                ival = op->kt_ival1;
-       else if (op->kt_ival2)
+       else if (op->kt_ival2) {
                ival = op->kt_ival2;
-       else
+       } else {
+               spin_unlock_bh(&op->bcm_tx_lock);
                return false;
+       }
+
+       spin_unlock_bh(&op->bcm_tx_lock);
 
        hrtimer_set_expires(hrt, ktime_add(ktime_get(), ival));
        return true;
@@ -494,25 +500,47 @@ 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 tx_ival1, tx_ival2;
+
+       /* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
+        * ktime_t reads racing with concurrent bcm_tx_setup() updates
+        */
+       spin_lock_bh(&op->bcm_tx_lock);
+       tx_ival1 = op->kt_ival1 && (op->count > 0);
+       tx_ival2 = !!op->kt_ival2;
+       spin_unlock_bh(&op->bcm_tx_lock);
+
+       if (tx_ival1) {
+               u32 flags, count;
+               struct bcm_timeval ival1, ival2;
 
-       if (op->kt_ival1 && (op->count > 0)) {
                bcm_can_tx(op, NULL);
-               if (!op->count && (op->flags & TX_COUNTEVT)) {
 
+               /* snapshot variables under lock to avoid torn reads racing
+                * with concurrent bcm_tx_setup() updates
+                */
+               spin_lock_bh(&op->bcm_tx_lock);
+               flags = op->flags;
+               count = op->count;
+               ival1 = op->ival1;
+               ival2 = op->ival2;
+               spin_unlock_bh(&op->bcm_tx_lock);
+
+               if (!count && (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.flags   = flags;
+                       msg_head.count   = count;
+                       msg_head.ival1   = ival1;
+                       msg_head.ival2   = ival2;
                        msg_head.can_id  = op->can_id;
                        msg_head.nframes = 0;
 
                        bcm_send_to_user(op, &msg_head, NULL, 0);
                }
 
-       } else if (op->kt_ival2) {
+       } else if (tx_ival2) {
                bcm_can_tx(op, NULL);
        }
 
@@ -1036,6 +1064,8 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
        /* check the given can_id */
        op = bcm_find_op(&bo->tx_ops, msg_head, ifindex);
        if (op) {
+               void *new_frames;
+
                /* update existing BCM operation */
 
                /*
@@ -1046,11 +1076,23 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                if (msg_head->nframes > op->nframes)
                        return -E2BIG;
 
-               /* update CAN frames content */
+               /* get new CAN frames content into a staging buffer before
+                * locking: validate and normalize the frames there so that
+                * bcm_can_tx() / bcm_tx_timeout_handler() never observe a
+                * partially updated or unvalidated frame in op->frames
+                */
+               new_frames = kmalloc(msg_head->nframes * op->cfsiz, GFP_KERNEL);
+               if (!new_frames)
+                       return -ENOMEM;
+
                for (i = 0; i < msg_head->nframes; i++) {
 
-                       cf = op->frames + op->cfsiz * i;
+                       cf = new_frames + op->cfsiz * i;
                        err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
+                       if (err < 0) {
+                               kfree(new_frames);
+                               return err;
+                       }
 
                        if (op->flags & CAN_FD_FRAME) {
                                if (cf->len > 64)
@@ -1060,36 +1102,38 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                                        err = -EINVAL;
                        }
 
-                       if (err < 0)
+                       if (err < 0) {
+                               kfree(new_frames);
                                return err;
+                       }
 
                        if (msg_head->flags & TX_CP_CAN_ID) {
                                /* copy can_id into frame */
                                cf->can_id = msg_head->can_id;
                        }
                }
+
+               spin_lock_bh(&op->bcm_tx_lock);
+
+               /* update CAN frames content */
+               memcpy(op->frames, new_frames, msg_head->nframes * op->cfsiz);
+
                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);
+                   op->flags & TX_RESET_MULTI_IDX) {
+                       /* potentially update changed nframes */
+                       op->nframes = msg_head->nframes;
+                       /* restart multiple frame transmission */
+                       op->currframe = 0;
+               }
 
-                       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;
 
-                       if (op->flags & SETTIMER)
-                               op->count = msg_head->count;
+               spin_unlock_bh(&op->bcm_tx_lock);
 
-                       spin_unlock_bh(&op->bcm_tx_lock);
-               }
+               kfree(new_frames);
 
        } else {
                /* insert new BCM operation for the given can_id */
@@ -1165,10 +1209,12 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
        if (op->flags & SETTIMER) {
                /* set timer values */
+               spin_lock_bh(&op->bcm_tx_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);
+               spin_unlock_bh(&op->bcm_tx_lock);
 
                /* disable an active timer due to zero values? */
                if (!op->kt_ival1 && !op->kt_ival2)