GRO/GSO hiding PMTU?

Submitted by David Miller on Feb. 11, 2011, 6:35 a.m.

Details

Message ID 20110210.223544.189709102.davem@davemloft.net
State RFC
Delegated to: David Miller
Headers show

Commit Message

David Miller Feb. 11, 2011, 6:35 a.m.
From: David Miller <davem@davemloft.net>
Date: Thu, 10 Feb 2011 22:22:16 -0800 (PST)

> I gave it a shot but it isn't easy.  We can figure out the length of
> the IP headers just fine, but the rest of the value we need to add
> to the MSS (the TCP header length) is transport specific which kind
> of implies a transport dependent gso proto op of some sort.
> 
> Or we just hack it, admit that only TCP creates GSO packets, and
> directly check for TCP protcol and then inspect the TCP header
> length :-)

Herbert how does this look for now?

Of course, we need to do something similar in all kinds of other spots.

Even places like bridging :-/

--------------------
ipv4: Check MSS properly in ip_forward() GSO check.

When we forward packets we decide whether we should send
a frag-needed ICMP back based upon the skb length.

But if this is a GSO packet, we wholesale elide the length
check entirely.

This is wrong, we do have to check things.  Except that the
length validation in this case is not straighforward.

We have to take the gso_size (which is the MSS) and add in
the IP and TCP header to arrive at the length we should use
to compare against the MTU.

Signed-off-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Feb. 11, 2011, 6:41 a.m.
On Thu, Feb 10, 2011 at 10:35:44PM -0800, David Miller wrote:
>
> Herbert how does this look for now?

This should work.

> Of course, we need to do something similar in all kinds of other spots.
> 
> Even places like bridging :-/

Yeah every place that does skb->len and skb_is_gso checks will need
this.

> +static bool send_frag_needed(struct sk_buff *skb, struct rtable *rt)
> +{
> +	unsigned int len_to_check = skb->len;
> +
> +	if (skb_is_gso(skb)) {
> +		unsigned int gso_size = skb_shinfo(skb)->gso_size;
> +		unsigned int ihl = ip_hdr(skb)->ihl * 4;
> +		struct tcphdr th_stack, *th;
> +
> +		if (WARN_ON_ONCE(ip_hdr(skb)->protocol != IPPROTO_TCP))
> +			return false;
> +
> +		th = skb_header_pointer(skb, ihl, sizeof(th_stack),
> +					&th_stack);
> +		if (!th)
> +			return false;
> +
> +		len_to_check = gso_size + ihl + (th->doff * 4);

I think we need to do some length verifications here because for
a malicious guest-generated packet the TCP header may not be present.

Thanks,
David Miller Feb. 11, 2011, 7:06 a.m.
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 11 Feb 2011 17:41:38 +1100

> I think we need to do some length verifications here because for
> a malicious guest-generated packet the TCP header may not be present.

Indeed, good catch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 99461f0..7449890 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -51,6 +51,36 @@  static int ip_forward_finish(struct sk_buff *skb)
 	return dst_output(skb);
 }
 
+static bool send_frag_needed(struct sk_buff *skb, struct rtable *rt)
+{
+	unsigned int len_to_check = skb->len;
+
+	if (skb_is_gso(skb)) {
+		unsigned int gso_size = skb_shinfo(skb)->gso_size;
+		unsigned int ihl = ip_hdr(skb)->ihl * 4;
+		struct tcphdr th_stack, *th;
+
+		if (WARN_ON_ONCE(ip_hdr(skb)->protocol != IPPROTO_TCP))
+			return false;
+
+		th = skb_header_pointer(skb, ihl, sizeof(th_stack),
+					&th_stack);
+		if (!th)
+			return false;
+
+		len_to_check = gso_size + ihl + (th->doff * 4);
+	}
+
+	if (len_to_check <= dst_mtu(&rt->dst))
+		return false;
+	if (!(ip_hdr(skb)->frag_off & htons(IP_DF)))
+		return false;
+	if (skb->local_df)
+		return false;
+
+	return true;
+}
+
 int ip_forward(struct sk_buff *skb)
 {
 	struct iphdr *iph;	/* Our header */
@@ -87,8 +117,7 @@  int ip_forward(struct sk_buff *skb)
 	if (opt->is_strictroute && rt->rt_dst != rt->rt_gateway)
 		goto sr_failed;
 
-	if (unlikely(skb->len > dst_mtu(&rt->dst) && !skb_is_gso(skb) &&
-		     (ip_hdr(skb)->frag_off & htons(IP_DF))) && !skb->local_df) {
+	if (unlikely(send_frag_needed(skb, rt))) {
 		IP_INC_STATS(dev_net(rt->dst.dev), IPSTATS_MIB_FRAGFAILS);
 		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
 			  htonl(dst_mtu(&rt->dst)));