Re: [PATCH net 07/19] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] `bcm_delete_rx_op()` and `bcm_release()` silently skip unregistering CAN receivers when `dev_get_by_index()` fails, leading to a leaked receiver and Use-After-Free.
- [High] Missing RCU protection and barrier for `bo->tx_ops` and `bo->rx_ops` list modifications.
- [High] Unprotected state modifications on CAN frames in the TX path causing a data race.
- [High] Unprotected state modifications on CAN frames in the RX path causing a data race.
- [Medium] Lockless reads of `bo->ifindex` contradict the patch's assumption and cause data races.
--

--- Patch [7]: [PATCH net 07/19] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit a163b4c749b944874d23ca2a810e83a4afdbe1b5
Author: Oliver Hartkopp <[email protected]>

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

This patch addresses a race condition between bcm_sendmsg and concurrent
bcm_notify or bcm_connect calls by moving the ifindex read under the
socket lock. It also fixes a bug where bcm_rx_setup would silently
return success if the target device disappeared concurrently.

Link: https://patch.msgid.link/[email protected]

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index bdf53241bd7b3..b612135b017d1 100644
> --- 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

[Severity: High]
This is a pre-existing issue, but does this code safely manage list updates
for the transmit and receive operations?

In bcm_rx_setup() and bcm_tx_setup(), the code uses the standard list_add()
function rather than list_add_rcu():

bcm_rx_setup() {
    ...
    list_add(&op->list, &bo->rx_ops);
}

This means there is no smp_store_release() barrier to ensure newly populated
fields of the bcm_op structure are visible before the node is linked.

Concurrently, bcm_proc_show() locklessly traverses bo->tx_ops using
list_for_each_entry() instead of the rcu variant:

bcm_proc_show() {
    ...
    list_for_each_entry(op, &bo->tx_ops, list) {

Could this data race cause a procfs reader to observe torn pointers or
uninitialized memory during traversal?

[Severity: High]
This is also a pre-existing issue, but is it safe to overwrite the
comparison frames directly from userspace?

In bcm_rx_setup(), op->frames is updated directly from process context:

bcm_rx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);

Since this can sleep, it cannot disable softirqs. Concurrently, CAN packet
reception can trigger bcm_rx_handler() in softirq (NAPI) context.

The handler locklessly compares the incoming frame against the actively
overwritten op->frames:

bcm_rx_handler() {
    ...
    if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==

Could this lead to incorrect filtering logic and misclassified or wrongly
accepted CAN frames?

[ ... ]
> @@ -1396,12 +1401,13 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
[ ... ]
> -	lock_sock(sk);
> -
>  	switch (msg_head.opcode) {
>  
>  	case TX_SETUP:

[Severity: High]
This is a pre-existing issue, but does this code protect the CAN frames
against concurrent transmission?

When bcm_sendmsg() calls bcm_tx_setup(), it directly modifies the
op->frames buffer from userspace:

bcm_tx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);

Because copying from userspace can sleep, this happens without holding the
bcm_tx_lock spinlock. Concurrently, the transmission timer can fire and
invoke bcm_can_tx(), which computes a pointer to op->frames while holding
the lock, but drops the lock before calling skb_put_data():

bcm_can_tx() {
    ...
    skb_put_data(skb, cf, op->cfsiz);

Can this race condition result in partially updated or torn CAN frames
being transmitted on the CAN bus?

[ ... ]
> @@ -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.
> +			 */

[Severity: Medium]
Is this comment accurate?

The comment states bo->ifindex is only ever accessed under lock_sock(),
which justifies skipping memory barriers for lockless fast-path accesses.

However, bcm_notify() reads it locklessly when handling a NETDEV_DOWN
event:

bcm_notify() {
    ...
    if (bo->bound && bo->ifindex == dev->ifindex) {

And bcm_proc_show() also reads bo->ifindex locklessly for procfs output
generation:

bcm_proc_show() {
    ...
    seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));

Because bcm_connect() uses WRITE_ONCE(bo->bound, 1) without a paired CPU
memory barrier for bo->ifindex, couldn't concurrent paths observe an
inconsistent torn state?

> +			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;
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but could this teardown path
skip unregistering CAN receivers and cause a use-after-free?

When a network device is unregistered, it is removed from the network
namespace index before the NETDEV_UNREGISTER notifiers run. If a socket
is closed during this exact window, dev_get_by_index() fails and returns
NULL:

bcm_release() {
    ...
    dev = dev_get_by_index(net, op->ifindex);
    if (dev) {
        bcm_rx_unreg(dev, op);
        dev_put(dev);
    }

As a result, bcm_rx_unreg() is silently skipped, but the bcm_op is
removed from bo->rx_ops and freed via RCU. The bcm_rx_handler remains
registered in the CAN core pointing to the freed memory.

A similar issue exists in bcm_delete_rx_op(). Could a subsequent packet
reception trigger the handler and corrupt memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7