Re: [PATCH net 09/15] can: raw, isotp: copy socket options outside of lock_sock()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch does not remove the RTNL stall it describes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but CAN_BCM has
  the same RTNL-stall problem through a sibling notifier, and the series…

--

can: raw, isotp: copy socket options outside of lock_sock()

This commit moves the user space copies for the CAN_RAW and CAN_ISOTP
socket options, including the CAN_RAW_FILTER filters, so they happen
before lock_sock(). raw_getsockopt() now duplicates the filters under
the socket lock and copies them to user space after release_sock(). The
goal is to stop a stalled user copy from holding lock_sock() and blocking
raw_notify()/isotp_notify() under RTNL.

Link: https://patch.msgid.link/20261027-can-rx-lock-v12-4-eb5afc2d8b9b@hartkopp.net

> diff --git a/net/can/raw.c b/net/can/raw.c
> index d6517f71cadeb..0926d5686e419 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

> @@ -795,17 +730,84 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>  			  sockptr_t optval, unsigned int optlen)
>  {
>  	struct sock *sk = sock->sk;
> +	struct can_filter *filter = NULL;  /* dyn. alloc'ed filters */
> +	struct can_filter sfilter;         /* single filter */
> +	union {
> +		can_err_mask_t err_mask;
> +		int flag;
> +		struct can_raw_vcid_options vcid_opts;
> +	} val;
> +	unsigned int size = 0;
> +	int count = 0;
>  	int err;
>  
>  	if (level != SOL_CAN_RAW)
>  		return -EINVAL;
>  
> +	/* Copy the option values from user space before lock_sock(), so a
> +	 * stalled copy can not block raw_notify() waiting for lock_sock()
> +	 * under RTNL.
> +	 */

[Severity: High]
The comment says a stalled copy can not block raw_notify() waiting for
lock_sock() under RTNL. Is that true for the socket as a whole?

The same bound CAN_RAW socket can still hold the socket lock during a
user copy through the SOL_SOCKET path. sk_setsockopt() takes the lock
before it handles SO_ATTACH_FILTER:

net/core/sock.c:sk_setsockopt() {
    ...
	sockopt_lock_sock(sk);
    ...
			ret = sk_attach_filter(&fprog, sk);
    ...
}

and sk_attach_filter()->__get_filter() then copies the program from
user space:

	if (copy_from_user(prog->insns, fprog->filter, fsize)) {

SO_ATTACH_REUSEPORT_CBPF goes down the same path. On the getsockopt
side, sk_get_filter() calls copy_to_sockptr() under sockopt_lock_sock().
That is the SOL_SOCKET equivalent of the CAN_RAW_FILTER copy this patch
moves after release_sock().

Suppose fprog.filter points into a FUSE or userfaultfd backed mapping
that never resolves. On NETDEV_UNREGISTER or a netns change of the
bound device, raw_notify() then waits under RTNL:

net/can/raw.c:raw_notify() {
    ...
	case NETDEV_UNREGISTER:
		lock_sock(sk);
    ...
}

As far as I can tell, net/can/ has no capability checks. That would
make this reachable by any local user who can create and bind a CAN_RAW
socket.

The commit message says "The generic SOL_SOCKET options in
sk_setsockopt() are not affected by this patch". It does not say that
the RTNL stall is still reachable on the same socket. No later patch in
the series changes net/core/sock.c or net/core/filter.c.

Would it be better to stop raw_notify() and isotp_notify() from blocking
on lock_sock() under RTNL? The other option would be to also move the
generic filter copies out of the socket lock.

> +	switch (optname) {
> +	case CAN_RAW_FILTER:

[ ... ]

> +	if (size) {
> +		if (optlen != size)
> +			return -EINVAL;
> +
> +		if (copy_from_sockptr(&val, optval, size))
> +			return -EFAULT;
> +	}
> +
>  	lock_sock(sk);
>  
> -	err = raw_setsockopt_locked(sock, optname, optval, optlen);
> +	err = raw_setsockopt_locked(sock, optname, &val, filter, count);
>  
>  	release_sock(sk);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
CAN_BCM seems to have the same RTNL stall through its notifier. This
series does not address or mention it.

bcm_sendmsg() takes the socket lock and then dispatches to
bcm_tx_setup(), bcm_rx_setup() and bcm_tx_send(). These copy the CAN
frames from the user iovec while the lock is held:

net/can/bcm.c:bcm_sendmsg() {
    ...
	lock_sock(sk);
    ...
}

net/can/bcm.c:bcm_tx_setup() {
    ...
			err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

bcm_notify() takes the lock for every bcm socket in the netns under
RTNL. It does this before checking whether the socket is bound to the
vanishing device:

net/can/bcm.c:bcm_notify() {
    ...
	if (!net_eq(dev_net(dev), sock_net(sk)))
		return;

	switch (msg) {

	case NETDEV_UNREGISTER:
		lock_sock(sk);
    ...
}

Say a BCM socket sends TX_SETUP, RX_SETUP or TX_SEND with the frame
data in a FUSE or userfaultfd backed mapping that stalls. Can any CAN
device unregistration in that netns then block RTNL indefinitely?

No later patch in the series changes these paths. The only bcm.c
changes are proc and can_rx_register naming.

>  
> +	/* on error free the filters allocated for CAN_RAW_FILTER above */
> +	if (err && count > 1)
> +		kfree(filter);
> +
>  	return err;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103204305.3211144-2-mkl%40pengutronix.de