Re: [PATCH mptcp-next v4 2/3] mptcp: inherit sk_reuse and fix dev bind override

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: wrong sk_uid during subflow bind when syncing sk_reuseport
--

commit 1d41a74d8cd4f5351a1f4186bfb92fe0ffeada01
Author: Geliang Tang <geliang@kernel.org>

mptcp: inherit sk_reuse and fix dev bind override

This commit propagates sk_reuse and sk_reuseport from the master socket to
subflows, increments setsockopt_seq when SOL_SOCKET options are set, and
avoids overriding sk_bound_dev_if in __mptcp_subflow_connect() when
local->ifindex is zero.

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 922f6ae5c80cb..952bc015eee87 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c

[ ... ]

> @@ -1706,6 +1708,9 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>  		assign_bit(b, &inet_sk(ssk)->inet_flags, src & BIT(b));
>  
>  	WRITE_ONCE(inet_sk(ssk)->local_port_range, READ_ONCE(inet_sk(sk)->local_port_range));
> +
> +	ssk->sk_reuse = sk->sk_reuse;
> +	ssk->sk_reuseport = sk->sk_reuseport;
>  }
>  
>  void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)

[Severity: High]
When syncing sk_reuseport to ssk here in sync_socket_options(), does ssk
have the right sk_uid before kernel_bind() runs in
__mptcp_subflow_connect()?

In mptcp_subflow_create_socket(), sock_create_kern() initializes
SOCK_INODE(sf)->i_uid to current_fsuid(), and sock_init_data() copies that
into sf->sk->sk_uid. Although mptcp_subflow_create_socket() then updates
SOCK_INODE(sf)->i_uid to the master socket's UID:

net/mptcp/subflow.c:mptcp_subflow_create_socket() {
	...
	SOCK_INODE(sf)->i_ino = SOCK_INODE(sk->sk_socket)->i_ino;
	SOCK_INODE(sf)->i_uid = SOCK_INODE(sk->sk_socket)->i_uid;
	SOCK_INODE(sf)->i_gid = SOCK_INODE(sk->sk_socket)->i_gid;
	...
}

ssk->sk_uid is not updated until mptcp_sock_graft() calls sk_set_socket()
at the end of __mptcp_subflow_connect(), after kernel_bind() and
kernel_connect():

net/mptcp/subflow.c:__mptcp_subflow_connect() {
	...
	err = mptcp_subflow_create_socket(sk, local->addr.family, &sf);
	...
	err = kernel_bind(sf, (struct sockaddr_unsized *)&addr, addrlen);
	...
	err = kernel_connect(sf, (struct sockaddr_unsized *)&addr, addrlen, O_NONBLOCK);
	...
	mptcp_sock_graft(ssk, sk->sk_socket);
}

When additional subflows are created asynchronously via mptcp_worker or a
privileged path manager daemon, current_fsuid() is 0 (root) rather than the
UID of the owning MPTCP socket.

During kernel_bind() -> inet_csk_get_port(), the SO_REUSEPORT checks in
sk_reuseport_match(), inet_csk_bind_conflict() -> inet_bind_conflict(), and
inet_csk_update_fastreuse() all read sk_uid(ssk):

net/ipv4/inet_connection_sock.c:inet_bind_conflict() {
	...
		} else if (!reuseport_ok || !sk->sk_reuseport ||
			   !sk2->sk_reuseport || !reuseport_cb_ok ||
			   (sk2->sk_state != TCP_TIME_WAIT &&
			    !uid_eq(uid, sk_uid(sk2)))) {
			return true;
		}
	...
}

Because sk_uid(ssk) is still 0 during kernel_bind(), won't an unprivileged
user's subflow fail with -EADDRINUSE when binding to the same port as the
initial subflow or another SO_REUSEPORT socket owned by that user, while
also matching against root-owned SO_REUSEPORT sockets and recording
tb->fastuid as 0 in inet_csk_update_fastreuse()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792470313.git.tanggeliang@kylinos.cn?part=2