On Mon, 2012-07-30 at 19:31 +0200, Eric Dumazet wrote:

On Mon, 2012-07-30 at 18:16 +0100, Ben Hutchings wrote:

quoted

A peer (or local user) may cause TCP to use a nominal MSS of as little
as 88 (actual MSS of 76 with timestamps).  Given that we have a
sufficiently prodigious local sender and the peer ACKs quickly enough,
it is nevertheless possible to grow the window for such a connection
to the point that we will try to send just under 64K at once.  This
results in a single skb that expands to 861 segments.

In some drivers with TSO support, such an skb will require hundreds of
DMA descriptors; a substantial fraction of a TX ring or even more than
a full ring.  The TX queue selected for the skb may stall and trigger
the TX watchdog repeatedly (since the problem skb will be retried
after the TX reset).  This particularly affects sfc, for which the
issue is designated as CVE-2012-3412.  However it may be that some
hardware or firmware also fails to handle such an extreme TSO request
correctly.

Therefore, limit the number of segments per skb to 100.  This should
make no difference to behaviour unless the actual MSS is less than
about 700.

Signed-off-by: Ben Hutchings <redacted>
---

Hmm, isnt GRO path also vulnerable ?

You mean, for forwarding?  If page fragments are used, the number of
segments is limited to MAX_SKB_FRAGS < 100.  But if skbs are aggregated
and build_skb() is not used (e.g. due to jumbo MTU) it appears we would
need an explicit limit.  Something like this:

---
From: Ben Hutchings <redacted>
Subject: [PATCH net] tcp: Limit number of segments merged by GRO

In the case where GRO aggregates skbs that cannot be converted to
page-fragments, there is currently no limit to the number of
segments that may be merged and subsequently re-segmented by GSO.

Apply the same limit as was introduced for locally-generated GSO skbs
in 'tcp: Limit number of segments generated by GSO per skb'.

Signed-off-by: Ben Hutchings <redacted>
---
 net/ipv4/tcp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 51d8daf..a052d07 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -3144,7 +3144,8 @@ out_check_final:
 					TCP_FLAG_RST | TCP_FLAG_SYN |
 					TCP_FLAG_FIN));
 
-	if (p && (!NAPI_GRO_CB(skb)->same_flow || flush))
+	if (p && (!NAPI_GRO_CB(skb)->same_flow || flush ||
+		  NAPI_GRO_CB(p)->count == TCP_MAX_GSO_SEGS))
 		pp = head;
 
 out:
---

An alternative would be to drop such frames in the ndo_start_xmit(), and
cap sk->sk_gso_max_size (since skb are no longer orphaned...)

I have implemented that workaround for the out-of-tree version of sfc.
For the in-tree driver, I thought it would be better to limit the number
of segments at source, which will avoid penalising any cases where the
window can grow so much larger than MSS.

Or you could introduce a new wk->sk_gso_max_segments, that your sfc
driver sets to whatever limit ?

Yes, that's another option.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.