Re: [PATCH net v2 05/15] mptcp: options: handle MPC data + csum reqd + no csum

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1 (1)

- [Low] Residual uninitialised read of `mp_opt->csum`: the patch closes
  the functional consumer of the uninitialised checksum…

Pre-existing issues:
- [Low] Reported concern (assessed as an intentional, documented
  behaviour change): a passive endpoint whose local checksum sysctl is…

--

mptcp: options: handle MPC data + csum reqd + no csum

The MP_CAPABLE parser now only honours the checksum required flag for
options that do not carry a data-len, so a peer that sets the flag on a
data-bearing MP_CAPABLE without appending the 2 bytes of checksum no
longer makes the receiver use an uninitialised csum field.

Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.1-23 [1]
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=1

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index b8318e030138..92f27b9e087a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -93,7 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		 * In other words, the only way for checksums not to be used
>  		 * is if both hosts in their SYNs set A=0."
>  		 */
> -		if (flags & MPTCP_CAP_CHECKSUM_REQD)
> +		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
> +		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
>  			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;

[Severity: Low]
The commit message says "uninitialised bytes could be used for the 'csum'
field".  Is the uninitialised csum field itself still read after this
change?

In mptcp_parse_option(), mp_opt->csum is only assigned in the 24-byte
form:

	if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) {
		mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
		mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
		ptr += 2;
	}
	pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u\n",
		 version, flags, opsize, mp_opt->sndr_key,
		 mp_opt->rcvr_key, mp_opt->data_len, mp_opt->csum);

but the pr_debug() passes mp_opt->csum for every opsize, including the
22-byte MP_CAPABLE + data form this patch is about.

The struct is an uninitialised stack object in mptcp_incoming_options()
and in net/mptcp/subflow.c, and mptcp_get_options() only clears the
4-byte status group:

	*(u32 *)&mp_opt->status = 0;

while protocol.h declares csum outside of that group:

	u16	data_len;
	__sum16	csum;
	struct_group(status,

So when that dynamic-debug callsite is enabled, 2 bytes of stale stack
are formatted into dmesg on a remotely reachable path.  The same
pr_debug() also prints sndr_key/rcvr_key/data_len for short opsizes,
so the debug-print read predates this commit; it is mentioned here
because the commit message frames the fix as removing the uninitialised
use of csum, and a stable backporter may read it that way.

Would adding mp_opt->csum = 0 (or extending the region cleared in
mptcp_get_options()) address this at the root, rather than relying on
every consumer testing OPTION_MPTCP_CSUMREQD?

[Severity: Low]
This isn't a bug introduced by this patch, but a question about the
passive side losing the peer's checksum requirement in one corner case.

subflow_init_req() derives the requirement from the local sysctl only:

	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));

and subflow_check_req() does not preserve OPTION_MPTCP_CSUMREQD from the
SYN, so mptcp_sk_clone_init() is the only place where the passive side
picks up the peer's flag:

	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
		WRITE_ONCE(msk->csum_enabled, true);

With the new opsize test, a 22-byte MP_CAPABLE + data with the A bit set
no longer sets that suboption, so msk->csum_enabled stays false and
get_mapping_status() accepts the mapping without validation:

	if (unlikely(subflow->map_csum_reqd != csum_reqd))
		return MAPPING_INVALID;

For a Linux sender the two signals cannot diverge, since
mptcp_established_options_mp() and mptcp_write_options() both derive the
A flag and the checksum field from opts->csum_reqd, and RFC 8684 omits
the checksum field only when checksums are not in use, so only a
self-contradictory peer reaches this path.  The commit message already
covers that trade-off.  The gap where the peer's SYN A bit is never
stored in subflow_req is pre-existing and unchanged here.

Would it be worth a short note in the commit message that the passive
side's only source for the peer's A bit is the third ACK, so the new
condition also drops that signal for a data-bearing third ACK?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-net-mptcp-misc-fixes-7-3-rc1-v2-0-df1de70348b6%40kernel.org