Re: [PATCH mptcp-next] mptcp: pm: reduce entries iterations on connect

[Paolo Abeni <pabeni@redhat.com>]

On 7/19/24 16:26, Matthieu Baerts (NGI0) wrote:
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 1b0e1617e90a..9fed7c92e52b 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -633,8 +633,9 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
>    */
>   static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>   					     struct mptcp_addr_info *remote,
> -					     struct mptcp_addr_info *addrs)
> +					     struct mptcp_pm_addr_entry *entries)
>   {
> +	struct mptcp_pm_addr_entry new_entry;
>   	struct sock *sk = (struct sock *)msk;
>   	struct mptcp_pm_addr_entry *entry;
>   	struct mptcp_addr_info mpc_addr;
> @@ -655,14 +656,14 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>   			continue;
>   
>   		if (msk->pm.subflows < subflows_max) {
> -			msk->pm.subflows++;
> -			addrs[i] = entry->addr;
> +			memcpy(&new_entry, entry, sizeof(new_entry));
>   
>   			/* Special case for ID0: set the correct endpoint */
>   			if (mptcp_addresses_equal(&entry->addr, &mpc_addr, entry->addr.port))
> -				addrs[i].id = 0;
> +				new_entry.addr.id = 0;
>   
> -			i++;
> +			msk->pm.subflows++;
> +			entries[i++] = new_entry;

'new_entry' is escaping the rcu protected section, dereferencing 
'entries' after the rcu unlock below could cause UaF.

Note, AFAICS we already have a similar problem in select_local_address().

One possibility would be to do a deep copy of mptcp_pm_addr_entry, but 
that will waste a lot of memory on the stack. What about to copy the id 
separately?

Thanks,

Paolo


>   		}
>   	}
>   	rcu_read_unlock();
> @@ -671,21 +672,19 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>   	 * 'IPADDRANY' local address
>   	 */
>   	if (!i) {
> -		struct mptcp_addr_info local;
> -
> -		memset(&local, 0, sizeof(local));
> -		local.family =
> +		memset(&new_entry.addr, 0, sizeof(new_entry.addr));
> +		new_entry.addr.family =
>   #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>   			       remote->family == AF_INET6 &&
>   			       ipv6_addr_v4mapped(&remote->addr6) ? AF_INET :
>   #endif
>   			       remote->family;
>   
> -		if (!mptcp_pm_addr_families_match(sk, &local, remote))
> +		if (!mptcp_pm_addr_families_match(sk, &new_entry.addr, remote))
>   			return 0;
>   
>   		msk->pm.subflows++;
> -		addrs[i++] = local;
> +		entries[i++] = new_entry;
>   	}
>   
>   	return i;
> @@ -693,7 +692,7 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>   
>   static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>   {
> -	struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
> +	struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];
>   	struct sock *sk = (struct sock *)msk;
>   	unsigned int add_addr_accept_max;
>   	struct mptcp_addr_info remote;
> @@ -722,13 +721,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>   	/* connect to the specified remote address, using whatever
>   	 * local address the routing configuration will pick.
>   	 */
> -	nr = fill_local_addresses_vec(msk, &remote, addrs);
> +	nr = fill_local_addresses_vec(msk, &remote, entries);
>   	if (nr == 0)
>   		return;
>   
>   	spin_unlock_bh(&msk->pm.lock);
>   	for (i = 0; i < nr; i++)
> -		if (__mptcp_subflow_connect(sk, &addrs[i], &remote) == 0)
> +		if (__mptcp_subflow_connect(sk, &entries[i], &remote) == 0)
>   			sf_created = true;
>   	spin_lock_bh(&msk->pm.lock);
>   
> @@ -1379,28 +1378,6 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
>   	return ret;
>   }
>   
> -int mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id,
> -					    u8 *flags, int *ifindex)
> -{
> -	struct mptcp_pm_addr_entry *entry;
> -	struct sock *sk = (struct sock *)msk;
> -	struct net *net = sock_net(sk);
> -
> -	/* No entries with ID 0 */
> -	if (id == 0)
> -		return 0;
> -
> -	rcu_read_lock();
> -	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
> -	if (entry) {
> -		*flags = entry->flags;
> -		*ifindex = entry->ifindex;
> -	}
> -	rcu_read_unlock();
> -
> -	return 0;
> -}
> -
>   static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
>   				      const struct mptcp_addr_info *addr)
>   {
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f0a4590506c6..97b09dffff6d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -119,23 +119,6 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
>   	return NULL;
>   }
>   
> -int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> -						   unsigned int id,
> -						   u8 *flags, int *ifindex)
> -{
> -	struct mptcp_pm_addr_entry *match;
> -
> -	spin_lock_bh(&msk->pm.lock);
> -	match = mptcp_userspace_pm_lookup_addr_by_id(msk, id);
> -	spin_unlock_bh(&msk->pm.lock);
> -	if (match) {
> -		*flags = match->flags;
> -		*ifindex = match->ifindex;
> -	}
> -
> -	return 0;
> -}
> -
>   int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>   				    struct mptcp_addr_info *skc)
>   {
> @@ -394,7 +377,7 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
>   
>   	lock_sock(sk);
>   
> -	err = __mptcp_subflow_connect(sk, &local.addr, &addr_r);
> +	err = __mptcp_subflow_connect(sk, &local, &addr_r);
>   
>   	release_sock(sk);
>   
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f2eb5273d752..259e247b0862 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -722,7 +722,7 @@ bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
>   void mptcp_local_address(const struct sock_common *skc, struct mptcp_addr_info *addr);
>   
>   /* called with sk socket lock held */
> -int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> +int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_addr_entry *local,
>   			    const struct mptcp_addr_info *remote);
>   int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
>   				struct socket **new_sock);
> @@ -1015,14 +1015,6 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>   struct mptcp_pm_add_entry *
>   mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
>   				const struct mptcp_addr_info *addr);
> -int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> -					 unsigned int id,
> -					 u8 *flags, int *ifindex);
> -int mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id,
> -					    u8 *flags, int *ifindex);
> -int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> -						   unsigned int id,
> -						   u8 *flags, int *ifindex);
>   int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
>   int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info);
>   int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 39e2cbdf3801..0835e71118b9 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1544,26 +1544,24 @@ void mptcp_info2sockaddr(const struct mptcp_addr_info *info,
>   #endif
>   }
>   
> -int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> +int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_addr_entry *local,
>   			    const struct mptcp_addr_info *remote)
>   {
>   	struct mptcp_sock *msk = mptcp_sk(sk);
>   	struct mptcp_subflow_context *subflow;
> +	int local_id = local->addr.id;
>   	struct sockaddr_storage addr;
>   	int remote_id = remote->id;
> -	int local_id = loc->id;
>   	int err = -ENOTCONN;
>   	struct socket *sf;
>   	struct sock *ssk;
>   	u32 remote_token;
>   	int addrlen;
> -	int ifindex;
> -	u8 flags;
>   
>   	if (!mptcp_is_fully_established(sk))
>   		goto err_out;
>   
> -	err = mptcp_subflow_create_socket(sk, loc->family, &sf);
> +	err = mptcp_subflow_create_socket(sk, local->addr.family, &sf);
>   	if (err)
>   		goto err_out;
>   
> @@ -1573,23 +1571,32 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>   		get_random_bytes(&subflow->local_nonce, sizeof(u32));
>   	} while (!subflow->local_nonce);
>   
> -	if (local_id)
> +	/* if 'IPADDRANY', the ID will be set later, after the routing */
> +	if (local->addr.family == AF_INET) {
> +		if (!local->addr.addr.s_addr)
> +			local_id = -1;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	} else if (sk->sk_family == AF_INET6) {
> +		if (ipv6_addr_any(&local->addr.addr6))
> +			local_id = -1;
> +#endif
> +	}
> +
> +	if (local_id >= 0)
>   		subflow_set_local_id(subflow, local_id);
>   
> -	mptcp_pm_get_flags_and_ifindex_by_id(msk, local_id,
> -					     &flags, &ifindex);
>   	subflow->remote_key_valid = 1;
>   	subflow->remote_key = READ_ONCE(msk->remote_key);
>   	subflow->local_key = READ_ONCE(msk->local_key);
>   	subflow->token = msk->token;
> -	mptcp_info2sockaddr(loc, &addr, ssk->sk_family);
> +	mptcp_info2sockaddr(&local->addr, &addr, ssk->sk_family);
>   
>   	addrlen = sizeof(struct sockaddr_in);
>   #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>   	if (addr.ss_family == AF_INET6)
>   		addrlen = sizeof(struct sockaddr_in6);
>   #endif
> -	ssk->sk_bound_dev_if = ifindex;
> +	ssk->sk_bound_dev_if = local->ifindex;
>   	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
>   	if (err)
>   		goto failed;
> @@ -1600,7 +1607,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
>   	subflow->remote_token = remote_token;
>   	WRITE_ONCE(subflow->remote_id, remote_id);
>   	subflow->request_join = 1;
> -	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +	subflow->request_bkup = !!(local->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>   	subflow->subflow_id = msk->subflow_id++;
>   	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
>   
> 
> ---
> base-commit: 52d9822897dc3649af506ed28135dedf9cf8ba3f
> change-id: 20240719-mptcp-pm-refact-connect-20050690bdbc
> prerequisite-change-id: 20240620-mptcp-pm-avail-f5e3957be441:v3
> prerequisite-patch-id: a804d4bf78954addfee863b9ae1b19ea01a7103f
> prerequisite-patch-id: 1cbde162f714bd28430c4985fb701762e536021c
> prerequisite-patch-id: 1cef710d16564a7f101184bbe9aaf1bb09d82743
> prerequisite-patch-id: d05eb1ef921bac68264c994daced70f46e707868
> prerequisite-patch-id: b7f76b3d50c14f862433d170fd48c30076da649b
> prerequisite-patch-id: f1e8aab49982c3de4092b9940d5dca1586dabf7f
> prerequisite-patch-id: 8ba292b3b2b681ba08dbfe22470ca01b9100c0f1
> prerequisite-patch-id: 6b45b393a5341c38a1ebbdeb212989c7e53de3bb
> prerequisite-patch-id: e5a410260d84101e6d099487545da0c9f19ff9d7
> prerequisite-patch-id: 593236babe3ceb10d682f8a8e8acc8b095e98b58
> prerequisite-patch-id: 7b94591f5d92cd183b3713f360eb29ca72d3c129
> prerequisite-patch-id: 07ddff8eebd1cfc9db306546307eae5157451ded
> prerequisite-patch-id: 55cc1b1f59a365757e4f0d23292479d4d12f1534
> prerequisite-patch-id: c6ba8859f84b0b726cdf57fa5ebcbf83d82f949b
> prerequisite-patch-id: a40ab15bd28a982b57f7bbc66564086a79e77070
> prerequisite-patch-id: 2988a4bcdb5a53ef659c15924a3cbfc6888b55ac
> prerequisite-patch-id: a5ea2de5eeaf719483e2fa7977afd94ba5fe507d
> prerequisite-patch-id: 9b37053038fcb0e952f21cb39389e7446e11b02a
> prerequisite-patch-id: 36b922942e9510b19c0c45646307c433049c756a
> prerequisite-patch-id: a565fd838caf63e4474de807b04b7fcde2acdb62
> 
> Best regards,