Re: [PATCH net 18/19] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

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

Pre-existing issues:
- [High] Concurrent `setsockopt(CAN_ISOTP_LL_OPTS)` and `isotp_sendmsg()` can cause `skb_over_panic` due to unlocked reads of `so->ll.mtu`.
- [High] Receiving a First Frame (FF) triggers a `kmalloc` with `GFP_ATOMIC` up to `max_pdu_size` (defaults to 8300 bytes, up to 1MB), which can exhaust the kernel's atomic memory pool.
- [Medium] The return values of `can_rx_register()` are ignored in `isotp_bind()`, leading to silently dropped packets and kernel log warnings on socket release.
--

--- Patch [18]: [PATCH net 18/19] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER ---
commit 9321d1d072ae2c3d91c2e9ffaeeff213350e562d
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

isotp_release() looked up the bound network device via dev_get_by_index()
using the stored ifindex. During device unregistration the device is
unlisted from the ifindex hash before the NETDEV_UNREGISTER notifier
chain runs, so a concurrent isotp_release() could find no device.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index d30937345bcdc..44c044eb83e12 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -978,6 +980,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			goto err_event_drop;
>  	}
>  
> +	/* so->bound is only checked once above - a wakeup may have
> +	 * unbound/rebound the socket meanwhile, so re-validate it
> +	 */
> +	if (!so->bound) {
> +		err = -EADDRNOTAVAIL;
> +		goto err_out_drop;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but can this lead to an skb_over_panic due to
unlocked reads of so->ll.mtu?

If isotp_sendmsg() is called and passes the so->bound check, and a
concurrent NETDEV_UNREGISTER notifier clears so->bound:

CPU1                                CPU2
isotp_sendmsg()                     
  // passes so->bound check
                                    NETDEV_UNREGISTER notifier
                                      so->bound = 0;
                                    
                                    setsockopt(CAN_ISOTP_LL_OPTS)
                                      // passes !so->bound check
                                      memcpy(&so->ll, &ll, sizeof(ll));

Could isotp_sendmsg() then execute sock_alloc_send_skb() with the old
so->ll.mtu, and later call skb_put_zero() with the newly modified, larger
so->ll.mtu, causing a buffer overflow panic?

>  	/* PDU size > default => try max_pdu_size */
>  	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {

[ ... ]

> @@ -1354,16 +1373,25 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
>  			isotp_rcv_echo, sk, "isotpe", sk);
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but should we be checking the
return values of can_rx_register() here?

If can_rx_register() fails (e.g., due to memory pressure returning -ENOMEM),
the error appears to be ignored, and the socket is incorrectly marked as
successfully bound.

When the socket is later closed, isotp_release() calls can_rx_unregister(),
which would fail to find the receiver entry and trigger a kernel warning.

> -	dev_put(dev);
> -
>  	/* switch to new settings */
>  	so->ifindex = ifindex;
>  	so->rxid = rx_id;

[ ... ]

Also, regarding a separate pre-existing issue in isotp_rcv_ff():

[Severity: High]
This is a pre-existing issue, but does receiving a First Frame (FF) trigger
a potentially large allocation in atomic context?

net/can/isotp.c:isotp_rcv_ff() {
    ...
    u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);
    if (newbuf) {
        so->rx.buf = newbuf;
        so->rx.buflen = max_pdu_size;
    }
    ...
}

Since max_pdu_size can be up to 1MB, could an attacker broadcasting First
Frames with large PDU sizes rapidly exhaust the kernel's atomic memory pool?

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