Re: [PATCH net v4] kcm: reject attaching sockets with a ULP, and TLS on KCM sockets

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 · Medium: 1 · 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).

- [Critical] Type confusion and slab out-of-bounds write in
  kcm_attach()/kcm_unattach() with a raw socket created by…
- [High] Check-then-use race in do_tcp_getsockopt(TCP_ULP), newly
  reachable because kcm_unattach() clears icsk_ulp_ops on a live socket…
- [Medium] Module-lifetime race on kcm_ulp_ops.

--

kcm: reject attaching sockets with a ULP, and TLS on KCM sockets

This patch makes kcm_attach() refuse lower sockets that already have a
ULP. It then installs a dummy, unregistered "kcm" tcp_ulp_ops in
icsk_ulp_ops so that tcp_set_ulp() refuses kTLS, or any other ULP, on an
attached socket. kcm_unattach() clears icsk_ulp_ops again.

Link: https://lore.kernel.org/netdev/3108e934-ec00-458b-8720-450554b35b55@linux.dev/

> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index a7f94d42036fb..e0d6536306f42 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -30,11 +30,19 @@

[ ... ]

>  unsigned int kcm_net_id;
>  
> +/* Not registered, so TCP_ULP can't select it. It only holds
> + * icsk_ulp_ops so tcp_set_ulp() refuses other ULPs like kTLS.
> + */
> +static const struct tcp_ulp_ops kcm_ulp_ops = {
> +	.name = "kcm",
> +};

[Severity: Medium]
What keeps kcm_ulp_ops alive for readers that access icsk_ulp_ops without
a lock?

For registered ULPs, __tcp_ulp_find_autoload() pins the owning module.
That reference is held until tcp_cleanup_ulp() runs at socket destroy, so
a reader that holds only a socket reference can safely dereference the
ops.

Here the ops are static data in the kcm module. There is no .owner and no
module reference is taken. The pointer is also cleared at unattach time,
not at destroy.

Readers such as tcp_diag_get_aux() and do_tcp_getsockopt(TCP_ULP) load
the pointer without RCU or the socket lock:

net/ipv4/tcp_diag.c:tcp_diag_get_aux() {
    ...
	ulp_ops = icsk->icsk_ulp_ops;
	if (ulp_ops) {
		err = tcp_diag_put_ulp(skb, sk, ulp_ops, net_admin);
    ...
}

Could this sequence read freed module memory?

reader                                  other task
loads &kcm_ulp_ops
                                        kcm_unattach()
                                        closes all KCM sockets
                                        rmmod kcm
dereferences ulp_ops->name / ->get_info

The getsockopt path can also sleep in copy_to_sockptr() while it is
copying from ->name. The synchronize_rcu() in free_module() would not
cover that window.

> @@ -1272,6 +1280,14 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
>  		goto out;
>  	}
>  
> +	/* KCM takes over read_sock() and the socket callbacks like a ULP
> +	 * does, the two can't stack.
> +	 */
> +	if (inet_csk_has_ulp(csk)) {
> +		err = -EOPNOTSUPP;
> +		goto out;
> +	}
> +

[ ... ]

> @@ -1305,6 +1321,7 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
>  	psock->save_write_space = csk->sk_write_space;
>  	psock->save_state_change = csk->sk_state_change;
>  	csk->sk_user_data = psock;
> +	inet_csk(csk)->icsk_ulp_ops = &kcm_ulp_ops;

[Severity: Critical]
Is csk guaranteed to be an inet_connection_sock at this point?

The existing checks at the top of kcm_attach() only look at the family,
the protocol and the state:

	if ((csk->sk_family != AF_INET && csk->sk_family != AF_INET6) ||
	    csk->sk_protocol != IPPROTO_TCP) {

A socket created with socket(AF_INET, SOCK_RAW, IPPROTO_TCP) passes this
check. inet_create() keeps sk_protocol = IPPROTO_TCP but leaves IS_ICSK
clear, because the raw protosw entry lacks INET_PROTOSW_ICSK.

Calling connect() on that socket goes through __ip4_datagram_connect(),
which sets sk_state = TCP_ESTABLISHED. So the socket also passes the
LISTEN/CLOSE check.

The new inet_csk_has_ulp() check then returns false, because it tests
IS_ICSK first:

	return inet_test_bit(IS_ICSK, sk) && !!inet_csk(sk)->icsk_ulp_ops;

strp_init() does not require read_sock either. As a result, this store
runs on an object that was allocated as struct raw_sock (or raw6_sock).

Does this write past the end of the raw_sock into the next slab object?

On x86_64, icsk_ulp_ops seems to be about sizeof(struct inet_sock) + 208
bytes into the object. struct raw_sock ends no more than
sizeof(struct inet_sock) + 192 bytes in. For raw6_sock, the store would
land in drop_counters padding or in the embedded ipv6_pinfo.

kcm_unattach() later stores NULL at the same offset:

	inet_csk(csk)->icsk_ulp_ops = NULL;

The path needs CAP_NET_RAW, which an unprivileged user namespace provides.
It also needs an AF_KCM socket and a socket filter BPF program:

socket(AF_INET, SOCK_RAW, IPPROTO_TCP)
connect()
ioctl(kcmfd, SIOCKCMATTACH)
  kcm_attach_ioctl()
    kcm_attach()
      inet_csk(csk)->icsk_ulp_ops = &kcm_ulp_ops;

Before this patch, the same attach did not corrupt any memory, because
strp_read_sock() just failed when read_sock was missing.

Should kcm_attach() also check sk_type == SOCK_STREAM, or the IS_ICSK bit,
before it touches inet_csk(csk)?

>  	WRITE_ONCE(csk->sk_data_ready, psock_data_ready);
>  	WRITE_ONCE(csk->sk_write_space, psock_write_space);
>  	csk->sk_state_change = psock_state_change;

[ ... ]

> @@ -1382,6 +1399,7 @@ static void kcm_unattach(struct kcm_psock *psock)
>  	 */
>  	write_lock_bh(&csk->sk_callback_lock);
>  	csk->sk_user_data = NULL;
> +	inet_csk(csk)->icsk_ulp_ops = NULL;

[Severity: High]
Can this race with getsockopt(TCP_ULP) on the same TCP socket?

Before this patch, icsk_ulp_ops on a socket reachable from userspace only
changed from NULL to non-NULL, and it was cleared only at destroy. Now it
returns to NULL on a live socket. Userspace still holds its TCP fd after
SIOCKCMATTACH.

do_tcp_getsockopt() reads the pointer twice without the socket lock. In
between, it does a user copy that can fault and sleep:

net/ipv4/tcp.c:do_tcp_getsockopt() {
    ...
	case TCP_ULP:
		...
		if (!icsk->icsk_ulp_ops) {
			...
			return 0;
		}
		if (copy_to_sockptr(optlen, &len, sizeof(int)))
			return -EFAULT;
		if (copy_to_sockptr(optval, icsk->icsk_ulp_ops->name, len))
			return -EFAULT;
    ...
}

Suppose thread A is in getsockopt(tcpfd, SOL_TCP, TCP_ULP) while thread B
runs ioctl(kcmfd, SIOCKCMUNATTACH) or closes the mux:

thread A                                thread B
do_tcp_getsockopt()
  icsk->icsk_ulp_ops != NULL
  copy_to_sockptr(optlen, ...)
                                        kcm_unattach()
                                          icsk_ulp_ops = NULL
  copy_to_sockptr(optval,
      icsk->icsk_ulp_ops->name, len)

The second read sees NULL, and the copy reads from
offsetof(struct tcp_ulp_ops, name).

On arm64, the source-side loads in copy_to_user have no extable entry.
Would that oops? KASAN would also report a null-ptr-deref here.

Attach and unattach can be repeated on the same socket. Neither store
uses WRITE_ONCE().

>  	WRITE_ONCE(csk->sk_data_ready, psock->save_data_ready);
>  	WRITE_ONCE(csk->sk_write_space, psock->save_write_space);
>  	csk->sk_state_change = psock->save_state_change;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029203428.3057054-1-rjethwani%40purestorage.com