Re: [PATCH mptcp] mptcp: fix uninitialized local_id in syncookie MP_JOIN reconstruction

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The `request_bkup` and `thmac` fields in `mptcp_subflow_request_sock` remain uninitialized when processing MP_JOIN SYN cookies, leading to state corruption and KMSAN violations.
--

--- Patch [1]: [PATCH mptcp] mptcp: fix uninitialized local_id in syncookie MP_JOIN reconstruction ---
commit 0b8fe993e0c1f5676def1ed9e644c11a85aca0ae
Author: Harshit Varu <[email protected]>

mptcp: fix uninitialized local_id in syncookie MP_JOIN reconstruction

This commit fixes an issue in MP_JOIN SYN cookie processing where the
local_id was not restored from the saved cookie entry. By restoring
subflow_req->local_id, it prevents the use of stale memory from previously
freed request sockets.

> diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c
> index 7f22526346a7e..b5cac57011221 100644
> --- a/net/mptcp/syncookies.c
> +++ b/net/mptcp/syncookies.c
> @@ -118,6 +118,7 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl
>  	subflow_req->local_nonce = e->local_nonce;
>  	subflow_req->backup = e->backup;
>  	subflow_req->remote_id = e->join_id;
> +	subflow_req->local_id = e->local_id;

[Severity: Medium]
This is a pre-existing issue, but do we also need to initialize request_bkup
and thmac here to prevent similar stale memory leaks?

It looks like these fields are not stored in the join_entry struct during
mptcp_join_store_state(), and therefore remain uninitialized in
mptcp_token_join_cookie_init_state(). 

Since the request socket slab is SLAB_TYPESAFE_BY_RCU and not zeroed, does
this cause uninitialized stale data to be copied into the new subflow context
when the child socket is cloned?

net/mptcp/subflow.c:subflow_ulp_clone() {
    ...
    new_ctx->backup = subflow_req->backup;
    new_ctx->request_bkup = subflow_req->request_bkup;
    WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
    new_ctx->token = subflow_req->token;
    new_ctx->thmac = subflow_req->thmac;
    ...
}

Could this lead to state corruption for the path priority or uninitialized
memory access warnings if those fields are accessed later during packet
scheduling?

>  	subflow_req->token = e->token;
>  	subflow_req->msk = msk;
>  	spin_unlock_bh(&join_entry_locks[i]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1