Re: [MPTCP][PATCH v2 mptcp-next 10/16] mptcp: validate the data checksum

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

Hi Geliang,

On Mon, 29 Mar 2021, Geliang Tang wrote:

> This patch add three new members named data_csum, csum_len and map_csum in
> struct mptcp_subflow_context, implemented a new function named
> mptcp_validate_data_checksum(). Validate the data checksum in the function
> __mptcp_move_skbs_from_subflow.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/protocol.c | 35 +++++++++++++++++++++++++++++++++++
> net/mptcp/protocol.h |  3 +++
> net/mptcp/subflow.c  |  7 +++++--
> 3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 436969d95e36..0b4ab35e234a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -532,6 +532,35 @@ static bool mptcp_check_data_fin(struct sock *sk)
> 	return ret;
> }
>
> +static bool mptcp_validate_data_checksum(struct sock *ssk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +	struct csum_pseudo_header header;
> +	__wsum csum;
> +
> +	if (__mptcp_check_fallback(msk))
> +		goto out;
> +
> +	if (subflow->csum_len < subflow->map_data_len)
> +		goto out;
> +
> +	header.data_seq = subflow->map_seq;
> +	header.subflow_seq = subflow->map_subflow_seq;
> +	header.data_len = subflow->map_data_len;
> +	header.csum = subflow->map_csum;
> +
> +	csum = csum_partial(&header, sizeof(header), subflow->data_csum);
> +
> +	if (csum_fold(csum))
> +		return false;
> +	subflow->data_csum = 0;
> +	subflow->csum_len = 0;
> +
> +out:
> +	return true;
> +}
> +
> static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
> 					   struct sock *ssk,
> 					   unsigned int *bytes)
> @@ -600,6 +629,12 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
> 			if (tp->urg_data)
> 				done = true;
>
> +			if (READ_ONCE(msk->csum_enabled)) {
> +				subflow->data_csum = skb_checksum(skb, offset, len,
> +								  subflow->data_csum);
> +				subflow->csum_len += len;
> +				mptcp_validate_data_checksum(ssk);

The return value is ignored there so a bad checksum doesn't seem to have 
any effect.

There needs to be different handling for several different cases here.


1. Checksums are not enabled, code works same as before this patch set


2. All data needed for the checksum is present and the checksum is valid

    --> Checksum validation will have to look through the queue of skbs in 
sk_receive_queue to add up the available length and compare to map length, 
then if there is enough combined data to cover map_data_len, go ahead and 
validate the checksum. For example, map_data_len could be 4500, and the 
payload of the first skb might be only 1500 bytes. Would have to wait for 
skb2 and skb3 (each with 1500 bytes) to arrive before validating the 
checksum.

    --> If the checksum for all the data is valid, make sure all skbs 
covered by the checksum are moved to the msk with __mptcp_move_skb()


3. All data needed for the checksum is present and the checksum is invalid

    --> Ignore all data from mapping (it must not be acked at the MPTCP 
level), update MPTCP_MIB_DSSCSUMERR). Discard the skbs with bad data, but 
be careful with the last skb in case it has a new mapping.


4. Checksums are enabled but there is some kind of non-checksum condition 
to handle (FIN flag, etc)

    --> Probably best to treat like a failed checksum and ensure other 
required actions happen, like handling the FIN


Thanks,

Mat



> +			}
> 			if (__mptcp_move_skb(msk, ssk, skb, offset, len))
> 				moved += len;
> 			seq += len;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 6be10ebabcd5..d4bf264d16cc 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -394,6 +394,9 @@ struct mptcp_subflow_context {
> 	u32	map_subflow_seq;
> 	u32	ssn_offset;
> 	u32	map_data_len;
> +	__wsum	data_csum;
> +	u32	csum_len;
> +	__sum16	map_csum;
> 	u32	request_mptcp : 1,  /* send MP_CAPABLE */
> 		request_join : 1,   /* send MP_JOIN */
> 		request_bkup : 1,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 75664da251a6..df7ad478bb2e 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -944,9 +944,12 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> 	subflow->map_data_len = data_len;
> 	subflow->map_valid = 1;
> 	subflow->mpc_map = mpext->mpc_map;
> -	pr_debug("new map seq=%llu subflow_seq=%u data_len=%u",
> +	subflow->data_csum = 0;
> +	subflow->csum_len = 0;
> +	subflow->map_csum = mpext->csum;
> +	pr_debug("new map seq=%llu subflow_seq=%u data_len=%u csum=%u",
> 		 subflow->map_seq, subflow->map_subflow_seq,
> -		 subflow->map_data_len);
> +		 subflow->map_data_len, subflow->map_csum);
>
> validate_seq:
> 	/* we revalidate valid mapping on new skb, because we must ensure
> -- 
> 2.30.2
>
>
>

--
Mat Martineau
Intel