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

[sashiko-bot@kernel.org]

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