Re: [PATCH net 03/19] can: raw: add locking for raw flags bitfield

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Holding `rtnl_lock()` across user-space memory copies enables a system-wide Denial of Service.
--

--- Patch [3]: [PATCH net 03/19] can: raw: add locking for raw flags bitfield ---
commit 64cfa8c3c841a4b23931e54d2dfb767381bcd986
Author: Oliver Hartkopp <[email protected]>

can: raw: add locking for raw flags bitfield

This commit integrates separate integer values into a single bitfield and
adds locking to prevent concurrent access during read-modify-write
operations in raw_setsockopt. It introduces a raw_setsockopt_locked
function wrapped by a new raw_setsockopt to handle the locking.

Closes: https://lore.kernel.org/linux-can/[email protected]/
Link: https://patch.msgid.link/[email protected]

> diff --git a/net/can/raw.c b/net/can/raw.c
> index a26942e78e688..82d9c0499c95f 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

> @@ -769,6 +743,26 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>  	return err;
>  }
>  
> +static int raw_setsockopt(struct socket *sock, int level, int optname,
> +			  sockptr_t optval, unsigned int optlen)
> +{
> +	struct sock *sk = sock->sk;
> +	int err;
> +
> +	if (level != SOL_CAN_RAW)
> +		return -EINVAL;
> +
> +	rtnl_lock();
> +	lock_sock(sk);
> +
> +	err = raw_setsockopt_locked(sock, optname, optval, optlen);

[Severity: High]
Does holding rtnl_lock() across raw_setsockopt_locked() allow a system-wide
Denial of Service?

Inside raw_setsockopt_locked(), user-space memory copies are performed
before rtnl_lock() was historically taken:

	case CAN_RAW_FILTER:
		...
		if (count > 1) {
			filter = memdup_sockptr(optval, optlen);
			...
		} else if (count == 1) {
			if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
				...
		}

Could a local user pass a memory pointer backed by userfaultfd or a slow FUSE
mount, intentionally stalling the page fault during memdup_sockptr() or
copy_from_sockptr(), and freeze network configuration for the whole system by
holding the global rtnl_lock?

It seems the isotp.c example cited in the commit message only takes
lock_sock() in its wrapper, not rtnl_lock().

> +
> +	release_sock(sk);
> +	rtnl_unlock();
> +
> +	return err;
> +}

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