| Message ID | 20130213234021.GA21829@casper.infradead.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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))) {
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