* [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).