Patchwork net: Convert skb->csum_(start|offset) integrity BUG_ON() to WARN_ON() & drop

login
register
mail settings
Submitter Thomas Graf
Date Feb. 13, 2013, 11:40 p.m.
Message ID <20130213234021.GA21829@casper.infradead.org>
Download mbox | patch
Permalink /patch/220301/
State Rejected
Delegated to: David Miller
Headers show

Comments

Thomas Graf - Feb. 13, 2013, 11:40 p.m.
skb_checksum_help() verifies the integrity of skb->csum_start
and skb->csum_offset with BUG_ON()s.

They have been hit with IPoIB which uses a 64K MTU. If a TCP
retransmission gets partially ACKed and collapsed multiple times
it is possible for the headroom to grow beyond 64K which will
overflow the 16bit skb->csum_start.

This in turn will trigger the BUG_ON() in skb_checksum_help().
Convert these to WARN_ON() and drop the packet.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/core/dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--
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
Thomas Graf - Feb. 13, 2013, 11:48 p.m.
On 02/13/13 at 11:40pm, Thomas Graf wrote:
> They have been hit with IPoIB which uses a 64K MTU. If a TCP
> retransmission gets partially ACKed and collapsed multiple times
> it is possible for the headroom to grow beyond 64K which will
> overflow the 16bit skb->csum_start.

On the subject of fixing this, I considered:

 a) Reallocate the headroom in tcp_trim_head() if it would
    overflow. I disregarded this idea because replacing the
    old skb with the new skb on the write queue for such a
    rare situation seems overkill.

 b) No longer collapse if the new skb would result in a
    a headroom + data that exceeds 64K. This seems to be the
    most trivial fix.

 c) Increase size of csum_start or store checksum_start_offset
    differently.

Other ideas?
--
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
David Miller - Feb. 14, 2013, 12:37 a.m.
From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 13 Feb 2013 23:48:43 +0000

> On 02/13/13 at 11:40pm, Thomas Graf wrote:
>> They have been hit with IPoIB which uses a 64K MTU. If a TCP
>> retransmission gets partially ACKed and collapsed multiple times
>> it is possible for the headroom to grow beyond 64K which will
>> overflow the 16bit skb->csum_start.
> 
> On the subject of fixing this, I considered:
> 
>  a) Reallocate the headroom in tcp_trim_head() if it would
>     overflow. I disregarded this idea because replacing the
>     old skb with the new skb on the write queue for such a
>     rare situation seems overkill.
> 
>  b) No longer collapse if the new skb would result in a
>     a headroom + data that exceeds 64K. This seems to be the
>     most trivial fix.
> 
>  c) Increase size of csum_start or store checksum_start_offset
>     differently.
> 
> Other ideas?

"b" is a good idea.

Let's not paper over this, this BUG_ON() is really a BUG_ON()
meaning "FIX ME NOW" :-)
--
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
Thomas Graf - Feb. 14, 2013, 10:18 a.m.
On 02/13/13 at 07:37pm, David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 13 Feb 2013 23:48:43 +0000
[...]
> >  b) No longer collapse if the new skb would result in a
> >     a headroom + data that exceeds 64K. This seems to be the
> >     most trivial fix.
[...]
> > Other ideas?
> 
> "b" is a good idea.

OK, patch to do so being tested by original reporter.

> Let's not paper over this, this BUG_ON() is really a BUG_ON()
> meaning "FIX ME NOW" :-)

Maybe it's my general dislike of BUG_ON() in the processing
path, especially if the bug condition can be influenced remotely.
It looks absolutely doable to trigger the previously mentioned
partial acking & collapsing on purpose by a malicious receiver
even with an MTU of 1500. I believe we should avoid total DoS
in future similar situations that we don't think of yet.
--
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
David Miller - Feb. 14, 2013, 6 p.m.
From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 14 Feb 2013 10:18:53 +0000

> On 02/13/13 at 07:37pm, David Miller wrote:
> Maybe it's my general dislike of BUG_ON() in the processing
> path, especially if the bug condition can be influenced remotely.
> It looks absolutely doable to trigger the previously mentioned
> partial acking & collapsing on purpose by a malicious receiver
> even with an MTU of 1500. I believe we should avoid total DoS
> in future similar situations that we don't think of yet.

I heard that people can very effectively protect themselves from DoS's
by not using Infiniband. :-)

--
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

diff --git a/net/core/dev.c b/net/core/dev.c
index f64e439..629d22e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2047,11 +2047,14 @@  int skb_checksum_help(struct sk_buff *skb)
 	}
 
 	offset = skb_checksum_start_offset(skb);
-	BUG_ON(offset >= skb_headlen(skb));
+	if (WARN_ON(offset >= skb_headlen(skb)))
+		return -ERANGE;
+
 	csum = skb_checksum(skb, offset, skb->len - offset, 0);
 
 	offset += skb->csum_offset;
-	BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
+	if (WARN_ON(offset + sizeof(__sum16) > skb_headlen(skb)))
+		return -ERANGE;
 
 	if (skb_cloned(skb) &&
 	    !skb_clone_writable(skb, offset + sizeof(__sum16))) {