From: luoqing <luoqing@kylinos.cn>

When all MPTCP address IDs (1-255) are exhausted, 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.

ID 0 has special meaning in MPTCP (it's reserved for the initial connection),
so this overflow can cause confusion and incorrect behavior, including
unintentional ID 0 reuse or address conflicts.

Signed-off-by: luoqing <luoqing@kylinos.cn>
---
 net/mptcp/pm_kernel.c    | 11 ++++++++---
 net/mptcp/pm_userspace.c | 15 +++++++++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 424f1a7f9248..a0fc9cabd770 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -795,9 +795,14 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 
 	if (!entry->addr.id) {
 find_next:
-		entry->addr.id = find_next_zero_bit(pernet->id_bitmap,
-						    MPTCP_PM_MAX_ADDR_ID + 1,
-						    pernet->next_id);
+		unsigned int id = find_next_zero_bit(pernet->id_bitmap,
+						     MPTCP_PM_MAX_ADDR_ID + 1,
+						     pernet->next_id);
+		if (id > MPTCP_PM_MAX_ADDR_ID) {
+			ret = -ENOSPC;
+			goto out;
+		}
+		entry->addr.id = id;
 		if (!entry->addr.id && pernet->next_id != 1) {
 			pernet->next_id = 1;
 			goto find_next;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d100867e9202..c48fd905f7a0 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;
+		}
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
 		msk->pm.local_addr_used++;
 		ret = e->addr.id;
-- 
2.25.1

Hi,

Thank you for this patch. Here is a very quick review done in between
tasks.

(-people who were in Cc: no need to add them when sending only to the
MPTCP ML.)

Also, please use a prefix: mptcp-net for fixes, mptcp-next for features.

The CI reported a few issues at build time, please check that.

> When all MPTCP address IDs (1-255) are exhausted, 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.

Are you sure? With the in-kernel PM? Do you have a reproducer?

> ID 0 has special meaning in MPTCP (it's reserved for the initial connection),
> so this overflow can cause confusion and incorrect behavior, including
> unintentional ID 0 reuse or address conflicts.

A fix should have a Fixes tag, please add one.

Also, if you were assisted by a tool, please add the Assisted-by tag.

> Signed-off-by: luoqing <luoqing@kylinos.cn>

For "legal" reasons, you are supposed to put your full name. Having only
one "word" for your full name, without capital letters looks wrong, no?

>
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 331f6fa99014..1bd81c94b971 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -795,9 +795,14 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
>
>  	if (!entry->addr.id) {
>  find_next:
> -		entry->addr.id = find_next_zero_bit(pernet->id_bitmap,
> -						    MPTCP_PM_MAX_ADDR_ID + 1,
> -						    pernet->next_id);
> +		unsigned int id = find_next_zero_bit(pernet->id_bitmap,
> +						     MPTCP_PM_MAX_ADDR_ID + 1,
> +						     pernet->next_id);
> +		if (id > MPTCP_PM_MAX_ADDR_ID) {
> +			ret = -ENOSPC;
> +			goto out;
> +		}
> +		entry->addr.id = id;
>  		if (!entry->addr.id && pernet->next_id != 1) {

The overflow is a normal case: find_next_zero_bit() will find the next
zero bit from next_id. If there it didn't find any, this block here
restart find_next_zero_bit() but from the beginning.

>  			pernet->next_id = 1;
>  			goto find_next;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index ad6ba658e5a5..0a1e835ae049 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,

Do you need to change this line? In case of error, 'e' is dropped.

> -							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;

I didn't check the code, but here, it looks like such check is needed.
But please, provide a test reproducing the issue, e.g. by adding a test
in userspace_pm.sh. (Be careful that such test should also run on older
kernels where the limits are lower, see what is done in pm_netlink.sh)

-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

Hi luoqing,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_pm_netlink ⚠️ 
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_pm_netlink ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/29320601438

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7832e6717726
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1127208


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Hi luoqing,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/29320601476

Status: failure
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7832e6717726
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1127208

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)