* [PATCH] xen/netfront: do not mark packets of length < MSS as GSO @ 2009-01-13 15:30 Ian Campbell 2009-01-14 4:18 ` Herbert Xu 0 siblings, 1 reply; 4+ messages in thread From: Ian Campbell @ 2009-01-13 15:30 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: Ian Campbell, Ian Campbell, Jeremy Fitzhardinge, jgarzik, netdev Linux assumes that skbs marked for GSO are longer than MSS. In particular tcp_tso_segment assumes that skb_segment will return a chain of at least 2 skbs. Therefore netfront should not pass such a packet up the stack. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: jgarzik@pobox.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index cd6184e..3777c09 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -953,6 +953,11 @@ err: else if (rx->flags & NETRXF_data_validated) skb->ip_summed = CHECKSUM_UNNECESSARY; + if (skb->data_len < skb_shinfo(skb)->gso_size) { + skb_shinfo(skb)->gso_size = 0; + skb_shinfo(skb)->gso_type = 0; + } + __skb_queue_tail(&rxq, skb); np->rx.rsp_cons = ++i; -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/netfront: do not mark packets of length < MSS as GSO 2009-01-13 15:30 [PATCH] xen/netfront: do not mark packets of length < MSS as GSO Ian Campbell @ 2009-01-14 4:18 ` Herbert Xu 2009-01-14 10:59 ` Ian Campbell 0 siblings, 1 reply; 4+ messages in thread From: Herbert Xu @ 2009-01-14 4:18 UTC (permalink / raw) To: Ian Campbell, David S. Miller; +Cc: jeremy, Ian.Campbell, jgarzik, netdev Ian Campbell <Ian.Campbell@citrix.com> wrote: > Linux assumes that skbs marked for GSO are longer than MSS. In > particular tcp_tso_segment assumes that skb_segment will return a > chain of at least 2 skbs. > > Therefore netfront should not pass such a packet up the stack. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Cc: jgarzik@pobox.com > Cc: netdev@vger.kernel.org Great catch! But we should fix this in the stack instead. gso: Ensure that the packet is long enough When we get a GSO packet from an untrusted source, we need to ensure that it is sufficiently long so that we don't end up crashing. Based on discovery and patch by Ian Campbell. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index bd6ff90..12e56ec 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2383,7 +2383,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) unsigned int seq; __be32 delta; unsigned int oldlen; - unsigned int len; + unsigned int mss; if (!pskb_may_pull(skb, sizeof(*th))) goto out; @@ -2399,10 +2399,13 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) oldlen = (u16)~skb->len; __skb_pull(skb, thlen); + mss = skb_shinfo(skb)->gso_size; + if (unlikely(skb->len <= mss)) + goto out; + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { /* Packet is from an untrusted source, reset gso_segs. */ int type = skb_shinfo(skb)->gso_type; - int mss; if (unlikely(type & ~(SKB_GSO_TCPV4 | @@ -2413,7 +2416,6 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) !(type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))) goto out; - mss = skb_shinfo(skb)->gso_size; skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); segs = NULL; @@ -2424,8 +2426,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) if (IS_ERR(segs)) goto out; - len = skb_shinfo(skb)->gso_size; - delta = htonl(oldlen + (thlen + len)); + delta = htonl(oldlen + (thlen + mss)); skb = segs; th = tcp_hdr(skb); @@ -2441,7 +2442,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) csum_fold(csum_partial(skb_transport_header(skb), thlen, skb->csum)); - seq += len; + seq += mss; skb = skb->next; th = tcp_hdr(skb); Thanks, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/netfront: do not mark packets of length < MSS as GSO 2009-01-14 4:18 ` Herbert Xu @ 2009-01-14 10:59 ` Ian Campbell 2009-01-15 4:41 ` David Miller 0 siblings, 1 reply; 4+ messages in thread From: Ian Campbell @ 2009-01-14 10:59 UTC (permalink / raw) To: Herbert Xu; +Cc: David S. Miller, jeremy, jgarzik, netdev On Wed, 2009-01-14 at 15:18 +1100, Herbert Xu wrote: > Ian Campbell <Ian.Campbell@citrix.com> wrote: > > Linux assumes that skbs marked for GSO are longer than MSS. In > > particular tcp_tso_segment assumes that skb_segment will return a > > chain of at least 2 skbs. > > > > Therefore netfront should not pass such a packet up the stack. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > > Cc: jgarzik@pobox.com > > Cc: netdev@vger.kernel.org > > Great catch! > > But we should fix this in the stack instead. > > gso: Ensure that the packet is long enough > > When we get a GSO packet from an untrusted source, we need to > ensure that it is sufficiently long so that we don't end up > crashing. > > Based on discovery and patch by Ian Campbell. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Looks good, thanks. Tested-by: Ian Campbell <ian.campbell@citrix.com> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index bd6ff90..12e56ec 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -2383,7 +2383,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) > unsigned int seq; > __be32 delta; > unsigned int oldlen; > - unsigned int len; > + unsigned int mss; > > if (!pskb_may_pull(skb, sizeof(*th))) > goto out; > @@ -2399,10 +2399,13 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) > oldlen = (u16)~skb->len; > __skb_pull(skb, thlen); > > + mss = skb_shinfo(skb)->gso_size; > + if (unlikely(skb->len <= mss)) > + goto out; > + > if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > /* Packet is from an untrusted source, reset gso_segs. */ > int type = skb_shinfo(skb)->gso_type; > - int mss; > > if (unlikely(type & > ~(SKB_GSO_TCPV4 | > @@ -2413,7 +2416,6 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) > !(type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))) > goto out; > > - mss = skb_shinfo(skb)->gso_size; > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > > segs = NULL; > @@ -2424,8 +2426,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) > if (IS_ERR(segs)) > goto out; > > - len = skb_shinfo(skb)->gso_size; > - delta = htonl(oldlen + (thlen + len)); > + delta = htonl(oldlen + (thlen + mss)); > > skb = segs; > th = tcp_hdr(skb); > @@ -2441,7 +2442,7 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb, int features) > csum_fold(csum_partial(skb_transport_header(skb), > thlen, skb->csum)); > > - seq += len; > + seq += mss; > skb = skb->next; > th = tcp_hdr(skb); > > > Thanks, ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/netfront: do not mark packets of length < MSS as GSO 2009-01-14 10:59 ` Ian Campbell @ 2009-01-15 4:41 ` David Miller 0 siblings, 0 replies; 4+ messages in thread From: David Miller @ 2009-01-15 4:41 UTC (permalink / raw) To: Ian.Campbell; +Cc: herbert, jeremy, jgarzik, netdev From: Ian Campbell <Ian.Campbell@citrix.com> Date: Wed, 14 Jan 2009 10:59:12 +0000 > On Wed, 2009-01-14 at 15:18 +1100, Herbert Xu wrote: > > gso: Ensure that the packet is long enough > > > > When we get a GSO packet from an untrusted source, we need to > > ensure that it is sufficiently long so that we don't end up > > crashing. > > > > Based on discovery and patch by Ian Campbell. > > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > Looks good, thanks. > > Tested-by: Ian Campbell <ian.campbell@citrix.com> Applied, thanks everyone. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-01-15 4:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-13 15:30 [PATCH] xen/netfront: do not mark packets of length < MSS as GSO Ian Campbell 2009-01-14 4:18 ` Herbert Xu 2009-01-14 10:59 ` Ian Campbell 2009-01-15 4:41 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).