[PATCH net v2] net/smc: fix out-of-bounds read when sk_user_data holds a sk_psock

[PATCH net v2] net/smc: fix out-of-bounds read when sk_user_data holds a sk_psock

[Sechang Lim]

SMC stores its smc_sock in the clcsock's sk_user_data tagged
SK_USER_DATA_NOCOPY and reads it back with smc_clcsock_user_data(), which
only strips that flag. sockmap stores a sk_psock in the same field tagged
SK_USER_DATA_NOCOPY | SK_USER_DATA_PSOCK. Nothing keeps both off one
socket, and SMC then casts the sk_psock to an smc_sock.

A passive-open child hits this. It inherits the listener's
smc_clcsock_data_ready(), but sk_clone_lock() clears its NOCOPY
sk_user_data, and a BPF sock_ops program then adds the child to a sockmap,
installing a sk_psock in that field. The inherited callback reads it as an
smc_sock and dereferences a clcsk_* pointer past the end of the sk_psock:

  BUG: KASAN: slab-out-of-bounds in smc_clcsock_data_ready+0x84/0x200 net/smc/af_smc.c:2637
  Read of size 8 at addr ffff8880013b8674 by task syz.6.12484/67930
   <IRQ>
   smc_clcsock_data_ready+0x84/0x200 net/smc/af_smc.c:2637
   tcp_urg+0x24d/0x360 net/ipv4/tcp_input.c:6264
   tcp_rcv_state_process+0x280d/0x4940 net/ipv4/tcp_input.c:7336
   tcp_child_process+0x371/0xa50 net/ipv4/tcp_minisocks.c:1002
   tcp_v4_rcv+0x1eaa/0x2a00 net/ipv4/tcp_ipv4.c:2186
   [...]
   </IRQ>

  Allocated by task 67930:
   sk_psock_init+0x142/0x740 net/core/skmsg.c:766
   sock_hash_update_common+0xd3/0x990 net/core/sock_map.c:1010
   bpf_sock_hash_update+0x114/0x170 net/core/sock_map.c:1229
   __cgroup_bpf_run_filter_sock_ops+0x74/0xa0 kernel/bpf/cgroup.c:1727
   tcp_init_transfer+0x1085/0x1100 net/ipv4/tcp_input.c:6693
   [...]

sk_psock() already guards the other side, returning NULL unless
SK_USER_DATA_PSOCK is set. Make smc_clcsock_user_data() and its RCU
variant return the smc_sock only when sk_user_data carries SMC's tag
alone. A sk_psock then reads back as NULL, which the data_ready and
fallback callbacks already handle.

Fixes: a60a2b1e0af1 ("net/smc: reduce active tcp_listen workers")
Signed-off-by: Sechang Lim <rhkrqnwk98@gmail.com>
---
 net/smc/smc.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 52145df83f6e..88dfb459b7cc 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -342,13 +342,25 @@ static inline void smc_init_saved_callbacks(struct smc_sock *smc)
 
 static inline struct smc_sock *smc_clcsock_user_data(const struct sock *clcsk)
 {
-	return (struct smc_sock *)
-	       ((uintptr_t)clcsk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+	uintptr_t data = (uintptr_t)clcsk->sk_user_data;
+
+	/*
+	 * Return the smc_sock only if the slot carries SMC's tag alone.
+	 * sockmap stores a sk_psock here tagged SK_USER_DATA_PSOCK; it is
+	 * not an smc_sock and must not be dereferenced as one.
+	 */
+	if ((data & ~SK_USER_DATA_PTRMASK) != SK_USER_DATA_NOCOPY)
+		return NULL;
+	return (struct smc_sock *)(data & SK_USER_DATA_PTRMASK);
 }
 
 static inline struct smc_sock *smc_clcsock_user_data_rcu(const struct sock *clcsk)
 {
-	return (struct smc_sock *)rcu_dereference_sk_user_data(clcsk);
+	uintptr_t data = (uintptr_t)rcu_dereference(__sk_user_data(clcsk));
+
+	if ((data & ~SK_USER_DATA_PTRMASK) != SK_USER_DATA_NOCOPY)
+		return NULL;
+	return (struct smc_sock *)(data & SK_USER_DATA_PTRMASK);
 }
 
 /* save target_cb in saved_cb, and replace target_cb with new_cb */
-- 
2.43.0

Re: [PATCH net v2] net/smc: fix out-of-bounds read when sk_user_data holds a sk_psock

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Overwriting `sk_user_data` during SMC fallback destroys BPF `psock` tags, leading to an infinite loop in `sock_map_close`.
--

commit cfd1a0bff0746ee614e26837804e04bb4e94ae6c
Author: Sechang Lim <rhkrqnwk98@gmail.com>

net/smc: fix out-of-bounds read when sk_user_data holds a sk_psock

This commit modifies smc_clcsock_user_data() and its RCU variant to
only return the smc_sock when the sk_user_data field carries SMC's tag
alone. This prevents SMC from interpreting a sk_psock structure placed
in sk_user_data by sockmap as an smc_sock, which was leading to
out-of-bounds reads.

> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 52145df83f6e7..88dfb459b7cc4 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -342,13 +342,25 @@ static inline void smc_init_saved_callbacks(struct smc_sock *smc)
>  
>  static inline struct smc_sock *smc_clcsock_user_data(const struct sock *clcsk)
>  {
> -	return (struct smc_sock *)
> -	       ((uintptr_t)clcsk->sk_user_data & ~SK_USER_DATA_NOCOPY);
> +	uintptr_t data = (uintptr_t)clcsk->sk_user_data;
> +
> +	/*
> +	 * Return the smc_sock only if the slot carries SMC's tag alone.
> +	 * sockmap stores a sk_psock here tagged SK_USER_DATA_PSOCK; it is
> +	 * not an smc_sock and must not be dereferenced as one.
> +	 */
> +	if ((data & ~SK_USER_DATA_PTRMASK) != SK_USER_DATA_NOCOPY)
> +		return NULL;
> +	return (struct smc_sock *)(data & SK_USER_DATA_PTRMASK);
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but the patch fails to address
the interaction between SMC and sockmap during fallback.

When an SMC socket falls back to TCP, smc_fback_replace_callbacks()
unconditionally overwrites the inner TCP socket's sk_user_data:

net/smc/af_smc.c:smc_fback_replace_callbacks() {
    ...
    __rcu_assign_sk_user_data_with_flags(clcsk, smc, SK_USER_DATA_NOCOPY);
    ...
}

If a BPF sock_ops program had previously attached the inner TCP socket to a
sockmap, this overwrite erases the SK_USER_DATA_PSOCK tag and leaks the
psock pointer.

Later, when the inner TCP socket is closed, its sk_prot->close function
(modified by BPF to sock_map_close) is executed. Inside sock_map_close(),
sk_psock_get(sk) returns NULL because the tag is missing, and the function
evaluates saved_close = READ_ONCE(sk->sk_prot)->close:

net/core/sock_map.c:sock_map_close() {
    ...
    if (unlikely(saved_close == sock_map_close))
        goto retry;
    ...
}

Since the protocol was never restored via sk_psock_drop, saved_close
evaluates to sock_map_close.

Could this cause an infinite goto retry loop and a lockup if triggered by
userspace? Is there a way to handle the fallback case without dropping the
existing psock tag?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619150342.3626224-1-rhkrqnwk98@gmail.com?part=1