can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
mutate both fields under that same lock. Because the lockless reads
and the locked writes are unordered with respect to each other, a
racing bcm_notify() (device unregister) or bcm_connect() (concurrent
bind on another thread sharing the socket) can make bcm_sendmsg()
observe an inconsistent combination, e.g. a stale bound=1 together
with the now-cleared ifindex=0, silently turning a socket bound to a
specific CAN interface into one that also matches "any" interface.

Keep the lockless bo->bound check purely as a fast-path reject, and
move the ifindex read (and a bo->bound re-check) into the locked
section, where every writer already serializes. This removes the
possibility of observing the two fields torn against each other,
rather than trying to fix it with more READ_ONCE()/WRITE_ONCE() pairs
on two independently updated fields. Annotate the now-purely-lockless
bo->bound accesses consistently across all its write sites.

Also fix bcm_rx_setup() silently returning success when the target
device disappears concurrently instead of reporting -ENODEV, so a
broken RX op is no longer left registered as if it had succeeded.

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by: Ginger <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260714-bcm_fixes-v15-2-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 bdf53241bd7b3d68eaec0b43b15f18c03595ca3e..b612135b017d1eb83d3407536bceb2341e1caa89 100644 (file)
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
                                op->rx_reg_dev = dev;
                                dev_put(dev);
+                       } else {
+                               /* the requested device is gone - do not
+                                * silently succeed without registering
+                                */
+                               err = -ENODEV;
                        }
 
                } else
@@ -1396,12 +1401,13 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 {
        struct sock *sk = sock->sk;
        struct bcm_sock *bo = bcm_sk(sk);
-       int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
+       int ifindex;
        struct bcm_msg_head msg_head;
        int cfsiz;
        int ret; /* read bytes or error codes as return value */
 
-       if (!bo->bound)
+       /* Lockless fast-path check for bound socket */
+       if (!READ_ONCE(bo->bound))
                return -ENOTCONN;
 
        /* check for valid message length from userspace */
@@ -1417,17 +1423,38 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
        if ((size - MHSIZ) % cfsiz)
                return -EINVAL;
 
+       lock_sock(sk);
+
+       /* Re-validate under the socket lock: a concurrent bcm_notify()
+        * may have unbound this socket (device removal) after the
+        * lockless fast-path check above. bo->ifindex is only ever
+        * mutated under lock_sock(), so reading it here - instead of
+        * before taking the lock - guarantees it can't be observed
+        * torn against bo->bound.
+        */
+       if (!bo->bound) {
+               ret = -ENOTCONN;
+               goto out_release;
+       }
+
+       /* default ifindex for this bcm_op */
+       ifindex = bo->ifindex;
+
        /* check for alternative ifindex for this bcm_op */
 
        if (!ifindex && msg->msg_name) {
                /* no bound device as default => check msg_name */
                DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
 
-               if (msg->msg_namelen < BCM_MIN_NAMELEN)
-                       return -EINVAL;
+               if (msg->msg_namelen < BCM_MIN_NAMELEN) {
+                       ret = -EINVAL;
+                       goto out_release;
+               }
 
-               if (addr->can_family != AF_CAN)
-                       return -EINVAL;
+               if (addr->can_family != AF_CAN) {
+                       ret = -EINVAL;
+                       goto out_release;
+               }
 
                /* ifindex from sendto() */
                ifindex = addr->can_ifindex;
@@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
                        struct net_device *dev;
 
                        dev = dev_get_by_index(sock_net(sk), ifindex);
-                       if (!dev)
-                               return -ENODEV;
+                       if (!dev) {
+                               ret = -ENODEV;
+                               goto out_release;
+                       }
 
                        if (dev->type != ARPHRD_CAN) {
                                dev_put(dev);
-                               return -ENODEV;
+                               ret = -ENODEV;
+                               goto out_release;
                        }
 
                        dev_put(dev);
                }
        }
 
-       lock_sock(sk);
-
        switch (msg_head.opcode) {
 
        case TX_SETUP:
@@ -1499,6 +1527,7 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
                break;
        }
 
+out_release:
        release_sock(sk);
 
        return ret;
@@ -1535,7 +1564,12 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
                                bo->bcm_proc_read = NULL;
                        }
 #endif
-                       bo->bound   = 0;
+                       /* Paired with the lockless fast-path check in
+                        * bcm_sendmsg(); bo->ifindex itself is only ever
+                        * accessed under lock_sock() so it needs no
+                        * annotation.
+                        */
+                       WRITE_ONCE(bo->bound, 0);
                        bo->ifindex = 0;
                        notify_enodev = 1;
                }
@@ -1676,7 +1710,7 @@ static int bcm_release(struct socket *sock)
 
        /* remove device reference */
        if (bo->bound) {
-               bo->bound   = 0;
+               WRITE_ONCE(bo->bound, 0);
                bo->ifindex = 0;
        }
 
@@ -1746,7 +1780,10 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
        }
 #endif /* CONFIG_PROC_FS */
 
-       bo->bound = 1;
+       /* bo->ifindex above is fully assigned before this point; pairs
+        * with the lockless fast-path check in bcm_sendmsg()
+        */
+       WRITE_ONCE(bo->bound, 1);
 
 fail:
        release_sock(sk);