| Message ID | 1a927c595adf46cf5ff186ca6b129f12fb70f492.1600375771.git.pabeni@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] mptcp: fix integer overflow in mptcp_subflow_discard_data() | expand |
On Thu, 17 Sep 2020, Paolo Abeni wrote: > Christoph reported an infinite loop in the subflow receive path > under stress condition. > > If there are multiple subflows, each of them using a large send > buffer, the delta between the sequence number used by > MPTCP-level retransmission can and the current msk->ack_seq > can be greater than MAX_INT. > > In the above scenario, when calling mptcp_subflow_discard_data(), > such delta will be truncated to int, and could result in a negative > number: no bytes will be dropped, and subflow_check_data_avail() > will try again to process the same packet, looping forever. > > This change addresses the issue by expanding the 'limit' size to 64 > bits, so that overflows are not possible anymore. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/87 > Fixes: 6719331c2f73 ("mptcp: trigger msk processing even for OoO data") > Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net-next patch, as the culprit commit is only on net-next currently > --- > net/mptcp/subflow.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Thanks Paolo! Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> -- Mat Martineau Intel
From: Paolo Abeni <pabeni@redhat.com> Date: Thu, 17 Sep 2020 23:07:24 +0200 > Christoph reported an infinite loop in the subflow receive path > under stress condition. > > If there are multiple subflows, each of them using a large send > buffer, the delta between the sequence number used by > MPTCP-level retransmission can and the current msk->ack_seq > can be greater than MAX_INT. > > In the above scenario, when calling mptcp_subflow_discard_data(), > such delta will be truncated to int, and could result in a negative > number: no bytes will be dropped, and subflow_check_data_avail() > will try again to process the same packet, looping forever. > > This change addresses the issue by expanding the 'limit' size to 64 > bits, so that overflows are not possible anymore. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/87 > Fixes: 6719331c2f73 ("mptcp: trigger msk processing even for OoO data") > Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net-next patch, as the culprit commit is only on net-next currently Applied, thank you.
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index a2ae3087e24d..34d6230df017 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -807,7 +807,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk, } static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb, - unsigned int limit) + u64 limit) { struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); bool fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN;
Christoph reported an infinite loop in the subflow receive path under stress condition. If there are multiple subflows, each of them using a large send buffer, the delta between the sequence number used by MPTCP-level retransmission can and the current msk->ack_seq can be greater than MAX_INT. In the above scenario, when calling mptcp_subflow_discard_data(), such delta will be truncated to int, and could result in a negative number: no bytes will be dropped, and subflow_check_data_avail() will try again to process the same packet, looping forever. This change addresses the issue by expanding the 'limit' size to 64 bits, so that overflows are not possible anymore. Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/87 Fixes: 6719331c2f73 ("mptcp: trigger msk processing even for OoO data") Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net-next patch, as the culprit commit is only on net-next currently --- net/mptcp/subflow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)