Re: [PATCH mptcp-net v3] mptcp: pm: fix userspace PM address ID overflow when all IDs are exhausted

Hi luoqing,

> When all MPTCP address IDs (1-255) are exhausted in the userspace PM,
> find_next_zero_bit() returns MPTCP_PM_MAX_ADDR_ID + 1 (256). This value
> overflows when stored in the u8 field e->addr.id, resulting in ID 0
> being stored and the entry being incorrectly added to the list.
>
> ID 0 is reserved for the initial connection in MPTCP, so this overflow
> can cause address conflicts.
>
> Note: the in-kernel PM already has an 'endpoints == MPTCP_PM_MAX_ADDR_ID'
> check in mptcp_pm_nl_append_new_local_addr() that returns -ERANGE before
> reaching find_next_zero_bit(), preventing this overflow. So this fix only
> addresses the userspace PM path.
>
> Store the find_next_zero_bit() result in a temporary unsigned int, check
> against MPTCP_PM_MAX_ADDR_ID, and return -ENOSPC if all IDs are truly
> exhausted. Properly free the allocated entry with sock_kfree_s() on error.

Thank you for the new version.

> Fixes: 4638de5aefe5 ("mptcp: handle local addrs announced by userspace PMs")
> Assisted-by: LLM
> Signed-off-by: Qing Luo <[email protected]>

(...)

> Based on your feedback, I will withdraw this patch submission. The core fix
> for the ID overflow is valid, but my patch became muddled with an unnecessary
> and potentially harmful behavioral change.
>
> If you believe the core fix (checking find_next_zero_bit's return value
> against MPTCP_PM_MAX_ADDR_ID) is still worth submitting on its own, I can
> prepare a clean v3 that only contains that fix and removes the needs_id logic
> change. Otherwise, I am happy to let this go.

I'm sorry, this is confusing: this is the v3 with only the (valid) fix,
no?

Do you have issues to send replies to my previous emails? Because
sending your replies here at the end of a patch is unusual, and these
comments are stripped when replying to an existing email.

>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 945aa5afc2dd..7d0e343c35ed 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -74,10 +74,17 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  			goto append_err;
>  		}
>
> -		if (!e->addr.id && needs_id)
> -			e->addr.id = find_next_zero_bit(id_bitmap,
> -							MPTCP_PM_MAX_ADDR_ID + 1,
> -							1);
> +		if (!e->addr.id && needs_id) {
> +			unsigned int id = find_next_zero_bit(id_bitmap,
> +							     MPTCP_PM_MAX_ADDR_ID + 1,
> +							     1);
> +			if (id > MPTCP_PM_MAX_ADDR_ID) {
> +				sock_kfree_s(sk, e, sizeof(*e));
> +				ret = -ENOSPC;
> +				goto append_err;
> +			}
> +			e->addr.id = id;
> +		}

The fix looks good to me, but I wonder if it wouldn't make more sense to
check this before allocating 'e', no?

e.g.

	if (!addr_match && !id_match) {
		struct mptcp_pm_addr_entry *e;
		unsigned int id;

		if (!entry->addr.id && needs_id) {
			id = find_next_zero_bit(id_bitmap,
						MPTCP_PM_MAX_ADDR_ID + 1, 1);
			if (id > MPTCP_PM_MAX_ADDR_ID) {
				ret = -ENOSPC;
				goto append_err;
		  	}
		} else {
			id = entry->addr.id;
		}

		e = sock_kmemdup(sk, entry, sizeof(*entry), GFP_ATOMIC);
		if (!e) {
			ret = -ENOMEM;
			goto append_err;
		}

		e->addr.id = id;
		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
		msk->pm.local_addr_used++;
		ret = e->addr.id;

WDYT?

A bit more code, but we avoid having to handle the free.

-- 
Matthieu Baerts (NGI0) <[email protected]>