The `size'' field of Xen network wire format is uint16_t, anything
bigger than
65535 will cause overflow.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
drivers/net/xen-netfront.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 5527663..8c3d065 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct
net_device *dev)
unsigned int len = skb_headlen(skb);
unsigned long flags;
+ /*
+ * wire format of xen_netif_tx_request only supports skb->len
+ * < 64K, because size field in xen_netif_tx_request is
+ * uint16_t.
+ */
+ if (unlikely(skb->len > (uint16_t)(~0))) {
+ net_alert_ratelimited(
+ "xennet: skb->len = %d, too big for wire format\n",
+ skb->len);
+ goto drop;
+ }
+
slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
xennet_count_skb_frag_slots(skb);
if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
--
1.7.10.4
Ian Campbell
2013-Mar-18 11:42 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote:> The `size'' field of Xen network wire format is uint16_t, anything bigger than > 65535 will cause overflow. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..8c3d065 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* > + * wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t.Is there some field we can set e.g. in struct ethernet_device which would stop this from happening?> + */ > + if (unlikely(skb->len > (uint16_t)(~0))) { > + net_alert_ratelimited( > + "xennet: skb->len = %d, too big for wire format\n", > + skb->len); > + goto drop; > + } > + > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > xennet_count_skb_frag_slots(skb); > if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
Konrad Rzeszutek Wilk
2013-Mar-18 13:44 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, Mar 18, 2013 at 10:35:53AM +0000, Wei Liu wrote:> The `size'' field of Xen network wire format is uint16_t, anything bigger than > 65535 will cause overflow.Should this also copy stable@vger.kernel.org?> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..8c3d065 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* > + * wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. > + */ > + if (unlikely(skb->len > (uint16_t)(~0))) { > + net_alert_ratelimited( > + "xennet: skb->len = %d, too big for wire format\n", > + skb->len); > + goto drop; > + } > + > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > xennet_count_skb_frag_slots(skb); > if (unlikely(slots > MAX_SKB_FRAGS + 1)) { > -- > 1.7.10.4 >
David Vrabel
2013-Mar-18 13:46 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 18/03/13 10:35, Wei Liu wrote:> The `size'' field of Xen network wire format is uint16_t, anything bigger than > 65535 will cause overflow.The backend needs to be able to handle these bad packets without disconnecting the VIF -- we can''t fix all the frontend drivers. David> Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..8c3d065 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* > + * wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. > + */ > + if (unlikely(skb->len > (uint16_t)(~0))) { > + net_alert_ratelimited( > + "xennet: skb->len = %d, too big for wire format\n", > + skb->len); > + goto drop; > + } > + > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > xennet_count_skb_frag_slots(skb); > if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
Ian Campbell
2013-Mar-18 13:48 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote:> On 18/03/13 10:35, Wei Liu wrote: > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > 65535 will cause overflow. > > The backend needs to be able to handle these bad packets without > disconnecting the VIF -- we can''t fix all the frontend drivers.Agreed, although that doesn''t imply that we shouldn''t fix the frontend where we can -- such as upstream as Wei does here. Ian.> > David > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > index 5527663..8c3d065 100644 > > --- a/drivers/net/xen-netfront.c > > +++ b/drivers/net/xen-netfront.c > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > unsigned int len = skb_headlen(skb); > > unsigned long flags; > > > > + /* > > + * wire format of xen_netif_tx_request only supports skb->len > > + * < 64K, because size field in xen_netif_tx_request is > > + * uint16_t. > > + */ > > + if (unlikely(skb->len > (uint16_t)(~0))) { > > + net_alert_ratelimited( > > + "xennet: skb->len = %d, too big for wire format\n", > > + skb->len); > > + goto drop; > > + } > > + > > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > > xennet_count_skb_frag_slots(skb); > > if (unlikely(slots > MAX_SKB_FRAGS + 1)) { >
David Vrabel
2013-Mar-18 14:00 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 18/03/13 13:48, Ian Campbell wrote:> On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote: >> On 18/03/13 10:35, Wei Liu wrote: >>> The `size'' field of Xen network wire format is uint16_t, anything bigger than >>> 65535 will cause overflow. >> >> The backend needs to be able to handle these bad packets without >> disconnecting the VIF -- we can''t fix all the frontend drivers. > > Agreed, although that doesn''t imply that we shouldn''t fix the frontend > where we can -- such as upstream as Wei does here.Yes, frontends should be fixed where possible. This is what I came up with for the backend. I don''t have time to look into it further but, Wei, feel free to use it as a starting point. David diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index cd49ba9..18e2671 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -899,10 +899,11 @@ static void netbk_fatal_tx_err(struct xenvif *vif) static int netbk_count_requests(struct xenvif *vif, struct xen_netif_tx_request *first, struct xen_netif_tx_request *txp, - int work_to_do) + int work_to_do, int idx) { RING_IDX cons = vif->tx.req_cons; int frags = 0; + bool drop = false; if (!(first->flags & XEN_NETTXF_more_data)) return 0; @@ -922,10 +923,20 @@ static int netbk_count_requests(struct xenvif *vif, memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags), sizeof(*txp)); - if (txp->size > first->size) { - netdev_err(vif->dev, "Frag is bigger than frame.\n"); - netbk_fatal_tx_err(vif); - return -EIO; + + /* + * If the guest submitted a frame >= 64 KiB then + * first->size overflowed and following frags will + * appear to be larger than the frame. + * + * This cannot be a fatal error as there are buggy + * frontends that do this. + * + * Consume all the frags and drop the packet. + */ + if (!drop && txp->size > first->size) { + netdev_dbg(vif->dev, "Frag is bigger than frame.\n"); + drop = true; } first->size -= txp->size; @@ -938,6 +949,12 @@ static int netbk_count_requests(struct xenvif *vif, return -EINVAL; } } while ((txp++)->flags & XEN_NETTXF_more_data); + + if (drop) { + netbk_tx_err(vif, txp, idx + frags); + return -EIO; + } + return frags; } @@ -1327,7 +1344,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk) continue; } - ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do); + ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do, idx); if (unlikely(ret < 0)) continue;
Wei Liu
2013-Mar-18 14:19 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 14:00 +0000, David Vrabel wrote:> On 18/03/13 13:48, Ian Campbell wrote: > > On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote: > >> On 18/03/13 10:35, Wei Liu wrote: > >>> The `size'' field of Xen network wire format is uint16_t, anything bigger than > >>> 65535 will cause overflow. > >> > >> The backend needs to be able to handle these bad packets without > >> disconnecting the VIF -- we can''t fix all the frontend drivers. > > > > Agreed, although that doesn''t imply that we shouldn''t fix the frontend > > where we can -- such as upstream as Wei does here. > > Yes, frontends should be fixed where possible. > > This is what I came up with for the backend. I don''t have time to look > into it further but, Wei, feel free to use it as a starting point. >Thanks for this patch. I haven''t gone through XSA-39 discussion, this is why I didn''t come up with a fix for backend -- I need to make sure dropping packet like this won''t re-exhibit the security hole. Wei.> David > > diff --git a/drivers/net/xen-netback/netback.c > b/drivers/net/xen-netback/netback.c > index cd49ba9..18e2671 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -899,10 +899,11 @@ static void netbk_fatal_tx_err(struct xenvif *vif) > static int netbk_count_requests(struct xenvif *vif, > struct xen_netif_tx_request *first, > struct xen_netif_tx_request *txp, > - int work_to_do) > + int work_to_do, int idx) > { > RING_IDX cons = vif->tx.req_cons; > int frags = 0; > + bool drop = false; > > if (!(first->flags & XEN_NETTXF_more_data)) > return 0; > @@ -922,10 +923,20 @@ static int netbk_count_requests(struct xenvif *vif, > > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags), > sizeof(*txp)); > - if (txp->size > first->size) { > - netdev_err(vif->dev, "Frag is bigger than frame.\n"); > - netbk_fatal_tx_err(vif); > - return -EIO; > + > + /* > + * If the guest submitted a frame >= 64 KiB then > + * first->size overflowed and following frags will > + * appear to be larger than the frame. > + * > + * This cannot be a fatal error as there are buggy > + * frontends that do this. > + * > + * Consume all the frags and drop the packet. > + */ > + if (!drop && txp->size > first->size) { > + netdev_dbg(vif->dev, "Frag is bigger than frame.\n"); > + drop = true; > } > > first->size -= txp->size; > @@ -938,6 +949,12 @@ static int netbk_count_requests(struct xenvif *vif, > return -EINVAL; > } > } while ((txp++)->flags & XEN_NETTXF_more_data); > + > + if (drop) { > + netbk_tx_err(vif, txp, idx + frags); > + return -EIO; > + } > + > return frags; > } > > @@ -1327,7 +1344,7 @@ static unsigned xen_netbk_tx_build_gops(struct > xen_netbk *netbk) > continue; > } > > - ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do); > + ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do, idx); > if (unlikely(ret < 0)) > continue; >
Wei Liu
2013-Mar-18 14:40 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote:> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > 65535 will cause overflow. > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > index 5527663..8c3d065 100644 > > --- a/drivers/net/xen-netfront.c > > +++ b/drivers/net/xen-netfront.c > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > unsigned int len = skb_headlen(skb); > > unsigned long flags; > > > > + /* > > + * wire format of xen_netif_tx_request only supports skb->len > > + * < 64K, because size field in xen_netif_tx_request is > > + * uint16_t. > > Is there some field we can set e.g. in struct ethernet_device which > would stop this from happening? >struct ethernet_device? I could not find it. And for struct net_device, there is no field for this AFAICT. Wei.> > > + */ > > + if (unlikely(skb->len > (uint16_t)(~0))) { > > + net_alert_ratelimited( > > + "xennet: skb->len = %d, too big for wire format\n", > > + skb->len); > > + goto drop; > > + } > > + > > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > > xennet_count_skb_frag_slots(skb); > > if (unlikely(slots > MAX_SKB_FRAGS + 1)) { > >
Ian Campbell
2013-Mar-18 14:54 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote:> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > 65535 will cause overflow. > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > --- > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > index 5527663..8c3d065 100644 > > > --- a/drivers/net/xen-netfront.c > > > +++ b/drivers/net/xen-netfront.c > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > unsigned int len = skb_headlen(skb); > > > unsigned long flags; > > > > > > + /* > > > + * wire format of xen_netif_tx_request only supports skb->len > > > + * < 64K, because size field in xen_netif_tx_request is > > > + * uint16_t. > > > > Is there some field we can set e.g. in struct ethernet_device which > > would stop this from happening? > > > > struct ethernet_device? I could not find it. > > And for struct net_device,I meant struct net_device.> there is no field for this AFAICT.Interesting. Are hardware devices expected to cope with arbitrary sized GSO skbs then I wonder. Ian.
Wei Liu
2013-Mar-18 15:04 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote:> On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > 65535 will cause overflow. > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > --- > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > index 5527663..8c3d065 100644 > > > > --- a/drivers/net/xen-netfront.c > > > > +++ b/drivers/net/xen-netfront.c > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > unsigned int len = skb_headlen(skb); > > > > unsigned long flags; > > > > > > > > + /* > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > + * uint16_t. > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > would stop this from happening? > > > > > > > struct ethernet_device? I could not find it. > > > > And for struct net_device, > > I meant struct net_device. > > > there is no field for this AFAICT. > > Interesting. Are hardware devices expected to cope with arbitrary sized > GSO skbs then I wonder. >No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct net_device. :-) Wei.> Ian. >
Ian Campbell
2013-Mar-18 15:07 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote:> On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > 65535 will cause overflow. > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > --- > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > index 5527663..8c3d065 100644 > > > > > --- a/drivers/net/xen-netfront.c > > > > > +++ b/drivers/net/xen-netfront.c > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > unsigned int len = skb_headlen(skb); > > > > > unsigned long flags; > > > > > > > > > > + /* > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > + * uint16_t. > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > would stop this from happening? > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > And for struct net_device, > > > > I meant struct net_device. > > > > > there is no field for this AFAICT. > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > GSO skbs then I wonder. > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > net_device. :-)But aren''t we seeing skb''s bigger than that? Maybe this is just a historical bug in some older guests? Ian.
Wei Liu
2013-Mar-18 15:10 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote:> On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > > On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > > 65535 will cause overflow. > > > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > --- > > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > > index 5527663..8c3d065 100644 > > > > > > --- a/drivers/net/xen-netfront.c > > > > > > +++ b/drivers/net/xen-netfront.c > > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > > unsigned int len = skb_headlen(skb); > > > > > > unsigned long flags; > > > > > > > > > > > > + /* > > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > > + * uint16_t. > > > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > > would stop this from happening? > > > > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > > > And for struct net_device, > > > > > > I meant struct net_device. > > > > > > > there is no field for this AFAICT. > > > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > > GSO skbs then I wonder. > > > > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > > net_device. :-) > > But aren''t we seeing skb''s bigger than that? >Yes, skb->len = 65538.> Maybe this is just a historical bug in some older guests? >I saw this with latest upstream kernel. Wei.> Ian. > >
annie li
2013-Mar-19 01:35 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 2013-3-18 18:35, Wei Liu wrote:> The `size'' field of Xen network wire format is uint16_t, anything bigger than > 65535 will cause overflow. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..8c3d065 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* > + * wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. > + */ > + if (unlikely(skb->len > (uint16_t)(~0))) { > + net_alert_ratelimited( > + "xennet: skb->len = %d, too big for wire format\n", > + skb->len); > + goto drop; > + } > +Maybe it is better to do some segmentation for packets(>15536) which support segments, and drop those which do not support segment. This can also be implemented in another patch(Just like what i did for packets which requires slots larger than SKB_MAX_FRAGS). Thanks Annie> slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > xennet_count_skb_frag_slots(skb); > if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
David Vrabel
2013-Mar-19 13:40 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 18/03/13 14:19, Wei Liu wrote:> On Mon, 2013-03-18 at 14:00 +0000, David Vrabel wrote: >> On 18/03/13 13:48, Ian Campbell wrote: >>> On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote: >>>> On 18/03/13 10:35, Wei Liu wrote: >>>>> The `size'' field of Xen network wire format is uint16_t, anything bigger than >>>>> 65535 will cause overflow. >>>> >>>> The backend needs to be able to handle these bad packets without >>>> disconnecting the VIF -- we can''t fix all the frontend drivers. >>> >>> Agreed, although that doesn''t imply that we shouldn''t fix the frontend >>> where we can -- such as upstream as Wei does here. >> >> Yes, frontends should be fixed where possible. >> >> This is what I came up with for the backend. I don''t have time to look >> into it further but, Wei, feel free to use it as a starting point. >> > > Thanks for this patch. > > I haven''t gone through XSA-39 discussion, this is why I didn''t come up > with a fix for backend -- I need to make sure dropping packet like this > won''t re-exhibit the security hole.How are these overlarge packets generated? How do you reproduce the issue? David
Wei Liu
2013-Mar-19 15:23 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Tue, 2013-03-19 at 13:40 +0000, David Vrabel wrote:> On 18/03/13 14:19, Wei Liu wrote: > > On Mon, 2013-03-18 at 14:00 +0000, David Vrabel wrote: > >> On 18/03/13 13:48, Ian Campbell wrote: > >>> On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote: > >>>> On 18/03/13 10:35, Wei Liu wrote: > >>>>> The `size'' field of Xen network wire format is uint16_t, anything bigger than > >>>>> 65535 will cause overflow. > >>>> > >>>> The backend needs to be able to handle these bad packets without > >>>> disconnecting the VIF -- we can''t fix all the frontend drivers. > >>> > >>> Agreed, although that doesn''t imply that we shouldn''t fix the frontend > >>> where we can -- such as upstream as Wei does here. > >> > >> Yes, frontends should be fixed where possible. > >> > >> This is what I came up with for the backend. I don''t have time to look > >> into it further but, Wei, feel free to use it as a starting point. > >> > > > > Thanks for this patch. > > > > I haven''t gone through XSA-39 discussion, this is why I didn''t come up > > with a fix for backend -- I need to make sure dropping packet like this > > won''t re-exhibit the security hole. > > How are these overlarge packets generated? How do you reproduce the issue? >Inside a VM, ifconfig eth0 mtu 100, iperf -c XXXX . But other people seeing this could not be using the same method because nobody would set mtu to 100 in production system AFAICT. Wei.> David
Nick Pegg
2013-Mar-19 20:13 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 3/18/13 6:35 AM, Wei Liu wrote:> The `size'' field of Xen network wire format is uint16_t, anything bigger than > 65535 will cause overflow. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..8c3d065 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* > + * wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. > + */ > + if (unlikely(skb->len > (uint16_t)(~0))) { > + net_alert_ratelimited( > + "xennet: skb->len = %d, too big for wire format\n", > + skb->len); > + goto drop; > + } > + > slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) + > xennet_count_skb_frag_slots(skb); > if (unlikely(slots > MAX_SKB_FRAGS + 1)) { >I have tested this patch on a 3.7.10 DomU and have confirmed that it works for the test case that Wei came up with (set MTU to 100 on DomU, run iperf). I haven''t come across any other ways to cause this to happen, so my testing isn''t very thorough. -Nick
Ben Hutchings
2013-Mar-19 21:24 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote:> On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > > On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > > 65535 will cause overflow. > > > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > --- > > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > > index 5527663..8c3d065 100644 > > > > > > --- a/drivers/net/xen-netfront.c > > > > > > +++ b/drivers/net/xen-netfront.c > > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > > unsigned int len = skb_headlen(skb); > > > > > > unsigned long flags; > > > > > > > > > > > > + /* > > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > > + * uint16_t. > > > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > > would stop this from happening? > > > > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > > > And for struct net_device, > > > > > > I meant struct net_device. > > > > > > > there is no field for this AFAICT. > > > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > > GSO skbs then I wonder. > > > > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > > net_device. :-) > > But aren''t we seeing skb''s bigger than that? > > Maybe this is just a historical bug in some older guests?GSO_MAX_SIZE is the maximum payload length, not the maximum total length of an skb. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that''s the marketing department''s job. They asked us to note that Solarflare product names are trademarked.
Ben Hutchings
2013-Mar-19 21:28 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Tue, 2013-03-19 at 21:24 +0000, Ben Hutchings wrote:> On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote: > > On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > > > On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > > > 65535 will cause overflow. > > > > > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > > --- > > > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > > > index 5527663..8c3d065 100644 > > > > > > > --- a/drivers/net/xen-netfront.c > > > > > > > +++ b/drivers/net/xen-netfront.c > > > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > > > unsigned int len = skb_headlen(skb); > > > > > > > unsigned long flags; > > > > > > > > > > > > > > + /* > > > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > > > + * uint16_t. > > > > > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > > > would stop this from happening? > > > > > > > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > > > > > And for struct net_device, > > > > > > > > I meant struct net_device. > > > > > > > > > there is no field for this AFAICT. > > > > > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > > > GSO skbs then I wonder. > > > > > > > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > > > net_device. :-) > > > > But aren''t we seeing skb''s bigger than that? > > > > Maybe this is just a historical bug in some older guests? > > GSO_MAX_SIZE is the maximum payload length, not the maximum total length > of an skb....and it''s actually just the default value assigned to dev->gso_max_size. You''ll want to change it to your actual maximum (65535 - maximum length of headers) before registering your net devices. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that''s the marketing department''s job. They asked us to note that Solarflare product names are trademarked.
David Vrabel
2013-Mar-20 20:02 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 18/03/13 14:00, David Vrabel wrote:> On 18/03/13 13:48, Ian Campbell wrote: >> On Mon, 2013-03-18 at 13:46 +0000, David Vrabel wrote: >>> On 18/03/13 10:35, Wei Liu wrote: >>>> The `size'' field of Xen network wire format is uint16_t, anything bigger than >>>> 65535 will cause overflow. >>> >>> The backend needs to be able to handle these bad packets without >>> disconnecting the VIF -- we can''t fix all the frontend drivers. >> >> Agreed, although that doesn''t imply that we shouldn''t fix the frontend >> where we can -- such as upstream as Wei does here. > > Yes, frontends should be fixed where possible. > > This is what I came up with for the backend. I don''t have time to look > into it further but, Wei, feel free to use it as a starting point.Got some time to test this (or more correctly, something similar with XCP''s kernel) and some fixes to the suggested patch are needed. See below.> diff --git a/drivers/net/xen-netback/netback.c > b/drivers/net/xen-netback/netback.c > index cd49ba9..18e2671 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -899,10 +899,11 @@ static void netbk_fatal_tx_err(struct xenvif *vif) > static int netbk_count_requests(struct xenvif *vif, > struct xen_netif_tx_request *first, > struct xen_netif_tx_request *txp, > - int work_to_do) > + int work_to_do, int idx)idx should be of RING_IDX type.> { > RING_IDX cons = vif->tx.req_cons; > int frags = 0; > + bool drop = false; > > if (!(first->flags & XEN_NETTXF_more_data)) > return 0; > @@ -922,10 +923,20 @@ static int netbk_count_requests(struct xenvif *vif, > > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags), > sizeof(*txp)); > - if (txp->size > first->size) { > - netdev_err(vif->dev, "Frag is bigger than frame.\n"); > - netbk_fatal_tx_err(vif); > - return -EIO; > + > + /* > + * If the guest submitted a frame >= 64 KiB then > + * first->size overflowed and following frags will > + * appear to be larger than the frame. > + * > + * This cannot be a fatal error as there are buggy > + * frontends that do this. > + * > + * Consume all the frags and drop the packet. > + */ > + if (!drop && txp->size > first->size) { > + netdev_dbg(vif->dev, "Frag is bigger than frame.\n"); > + drop = true; > } > > first->size -= txp->size; > @@ -938,6 +949,12 @@ static int netbk_count_requests(struct xenvif *vif, > return -EINVAL; > } > } while ((txp++)->flags & XEN_NETTXF_more_data); > + > + if (drop) { > + netbk_tx_err(vif, txp, idx + frags);This needs to be netbk_tx_err(vif, first, idx + frags) or the guest will crash as we push a bunch of invalid responses. David> + return -EIO; > + } > + > return frags; > } > > @@ -1327,7 +1344,7 @@ static unsigned xen_netbk_tx_build_gops(struct > xen_netbk *netbk) > continue; > } > > - ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do); > + ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do, idx); > if (unlikely(ret < 0)) > continue;
Wei Liu
2013-Mar-21 13:40 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Wed, 2013-03-20 at 20:02 +0000, David Vrabel wrote:> > first->size -= txp->size; > > @@ -938,6 +949,12 @@ static int netbk_count_requests(struct xenvif *vif, > > return -EINVAL; > > } > > } while ((txp++)->flags & XEN_NETTXF_more_data); > > + > > + if (drop) { > > + netbk_tx_err(vif, txp, idx + frags); > > This needs to be netbk_tx_err(vif, first, idx + frags) or the guest will > crash as we push a bunch of invalid responses. >Can this really handle the situation when first->size overflows? In that case frag == 0, the netbk_tx_err call is in fact netbk_tx_err(vif, txp, idx). idx is the ring index of first txp, so in fact you''re only responding to the head txp, ignoring other tx requests for the same skb? Even first->size doesn''t overflow, a malicious / buggy frontend can still generate tx req that makes txp->size > first->size. In that case there could be also some trailing tx requests left un-responded. I check the code before XSA-39 fix, its logic is more or less the same, but it did work. My suspicion is that those trailing tx requests are invalidated in future loops of xen_netbk_tx_build_gops. I think the correct action is to just take first txp and loop responding until we consume the whole packet. Wei.> David > > > + return -EIO; > > + } > > + > > return frags; > > } > > > > @@ -1327,7 +1344,7 @@ static unsigned xen_netbk_tx_build_gops(struct > > xen_netbk *netbk) > > continue; > > } > > > > - ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do); > > + ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do, idx); > > if (unlikely(ret < 0)) > > continue;
David Vrabel
2013-Mar-21 14:11 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 21/03/13 13:40, Wei Liu wrote:> > > I think the correct action is to just take first txp and loop responding > until we consume the whole packet.Um. This is what the patch is doing. David
Wei Liu
2013-Mar-21 14:15 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Thu, 2013-03-21 at 14:11 +0000, David Vrabel wrote:> On 21/03/13 13:40, Wei Liu wrote: > > > > > > I think the correct action is to just take first txp and loop responding > > until we consume the whole packet. > > Um. This is what the patch is doing. >No. The idx you passed in is the index of the first txp, and idx + fras doesn''t necessary point to last tx requests of the packet. We should use XEN_NETTXF_more_data to loop through the packet. Wei.> David
Wei Liu
2013-Mar-21 14:20 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Thu, 2013-03-21 at 14:15 +0000, Wei Liu wrote:> On Thu, 2013-03-21 at 14:11 +0000, David Vrabel wrote: > > On 21/03/13 13:40, Wei Liu wrote: > > > > > > > > > I think the correct action is to just take first txp and loop responding > > > until we consume the whole packet. > > > > Um. This is what the patch is doing. > > > > No. The idx you passed in is the index of the first txp, and idx + fras > doesn''t necessary point to last tx requests of the packet. We should use > XEN_NETTXF_more_data to loop through the packet. >Sorry for the noise, this patch already loop through the whole packet. Wei.> > Wei. > > > David > >
Ian Campbell
2013-Apr-09 14:30 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Tue, 2013-03-19 at 21:28 +0000, Ben Hutchings wrote:> On Tue, 2013-03-19 at 21:24 +0000, Ben Hutchings wrote: > > On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote: > > > On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > > > > On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > > > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > > > > 65535 will cause overflow. > > > > > > > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > > > --- > > > > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > > > > index 5527663..8c3d065 100644 > > > > > > > > --- a/drivers/net/xen-netfront.c > > > > > > > > +++ b/drivers/net/xen-netfront.c > > > > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > > > > unsigned int len = skb_headlen(skb); > > > > > > > > unsigned long flags; > > > > > > > > > > > > > > > > + /* > > > > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > > > > + * uint16_t. > > > > > > > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > > > > would stop this from happening? > > > > > > > > > > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > > > > > > > And for struct net_device, > > > > > > > > > > I meant struct net_device. > > > > > > > > > > > there is no field for this AFAICT. > > > > > > > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > > > > GSO skbs then I wonder. > > > > > > > > > > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > > > > net_device. :-) > > > > > > But aren''t we seeing skb''s bigger than that? > > > > > > Maybe this is just a historical bug in some older guests? > > > > GSO_MAX_SIZE is the maximum payload length, not the maximum total length > > of an skb. > > ...and it''s actually just the default value assigned to > dev->gso_max_size. You''ll want to change it to your actual maximum > (65535 - maximum length of headers) before registering your net devices.Thanks. "maximum length of headers" might be a bit tricky to determine generically :-(. Ian.
Ben Hutchings
2013-Apr-09 14:45 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Tue, 2013-04-09 at 15:30 +0100, Ian Campbell wrote:> On Tue, 2013-03-19 at 21:28 +0000, Ben Hutchings wrote: > > On Tue, 2013-03-19 at 21:24 +0000, Ben Hutchings wrote: > > > On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote: > > > > On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > > > > > On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > > > > > > On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > > > > > > > On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > > > > > > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > > > > > > The `size'' field of Xen network wire format is uint16_t, anything bigger than > > > > > > > > > 65535 will cause overflow. > > > > > > > > > > > > > > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > > > > --- > > > > > > > > > drivers/net/xen-netfront.c | 12 ++++++++++++ > > > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > > > > > > > > index 5527663..8c3d065 100644 > > > > > > > > > --- a/drivers/net/xen-netfront.c > > > > > > > > > +++ b/drivers/net/xen-netfront.c > > > > > > > > > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > > > > > > unsigned int len = skb_headlen(skb); > > > > > > > > > unsigned long flags; > > > > > > > > > > > > > > > > > > + /* > > > > > > > > > + * wire format of xen_netif_tx_request only supports skb->len > > > > > > > > > + * < 64K, because size field in xen_netif_tx_request is > > > > > > > > > + * uint16_t. > > > > > > > > > > > > > > > > Is there some field we can set e.g. in struct ethernet_device which > > > > > > > > would stop this from happening? > > > > > > > > > > > > > > > > > > > > > > struct ethernet_device? I could not find it. > > > > > > > > > > > > > > And for struct net_device, > > > > > > > > > > > > I meant struct net_device. > > > > > > > > > > > > > there is no field for this AFAICT. > > > > > > > > > > > > Interesting. Are hardware devices expected to cope with arbitrary sized > > > > > > GSO skbs then I wonder. > > > > > > > > > > > > > > > > No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > > > > > net_device. :-) > > > > > > > > But aren''t we seeing skb''s bigger than that? > > > > > > > > Maybe this is just a historical bug in some older guests? > > > > > > GSO_MAX_SIZE is the maximum payload length, not the maximum total length > > > of an skb. > > > > ...and it''s actually just the default value assigned to > > dev->gso_max_size. You''ll want to change it to your actual maximum > > (65535 - maximum length of headers) before registering your net devices. > > Thanks. > > "maximum length of headers" might be a bit tricky to determine > generically :-(.Well you don''t need to be generic, you need to know the maximum length of headers that might appear in a TSO skb. Ethernet + VLAN tag + IPv6 + TCP + timestamp option = 90 bytes, but I''m not sure whether there can be other IP or TCP options in a TSO skb. I''d really like to get the TSO requirements clearly documented somewhere. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that''s the marketing department''s job. They asked us to note that Solarflare product names are trademarked.
Christoph Egger
2013-Apr-09 14:53 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On 09.04.13 16:45, Ben Hutchings wrote:> On Tue, 2013-04-09 at 15:30 +0100, Ian Campbell wrote: >> On Tue, 2013-03-19 at 21:28 +0000, Ben Hutchings wrote: >>> On Tue, 2013-03-19 at 21:24 +0000, Ben Hutchings wrote: >>>> On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote: >>>>> On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: >>>>>> On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: >>>>>>> On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: >>>>>>>> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: >>>>>>>>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: >>>>>>>>>> The `size'' field of Xen network wire format is uint16_t, anything bigger than >>>>>>>>>> 65535 will cause overflow. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> >>>>>>>>>> --- >>>>>>>>>> drivers/net/xen-netfront.c | 12 ++++++++++++ >>>>>>>>>> 1 file changed, 12 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >>>>>>>>>> index 5527663..8c3d065 100644 >>>>>>>>>> --- a/drivers/net/xen-netfront.c >>>>>>>>>> +++ b/drivers/net/xen-netfront.c >>>>>>>>>> @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>>>>>>>>> unsigned int len = skb_headlen(skb); >>>>>>>>>> unsigned long flags; >>>>>>>>>> >>>>>>>>>> + /* >>>>>>>>>> + * wire format of xen_netif_tx_request only supports skb->len >>>>>>>>>> + * < 64K, because size field in xen_netif_tx_request is >>>>>>>>>> + * uint16_t. >>>>>>>>> >>>>>>>>> Is there some field we can set e.g. in struct ethernet_device which >>>>>>>>> would stop this from happening? >>>>>>>>> >>>>>>>> >>>>>>>> struct ethernet_device? I could not find it. >>>>>>>> >>>>>>>> And for struct net_device, >>>>>>> >>>>>>> I meant struct net_device. >>>>>>> >>>>>>>> there is no field for this AFAICT. >>>>>>> >>>>>>> Interesting. Are hardware devices expected to cope with arbitrary sized >>>>>>> GSO skbs then I wonder. >>>>>>> >>>>>> >>>>>> No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct >>>>>> net_device. :-) >>>>> >>>>> But aren''t we seeing skb''s bigger than that? >>>>> >>>>> Maybe this is just a historical bug in some older guests? >>>> >>>> GSO_MAX_SIZE is the maximum payload length, not the maximum total length >>>> of an skb. >>> >>> ...and it''s actually just the default value assigned to >>> dev->gso_max_size. You''ll want to change it to your actual maximum >>> (65535 - maximum length of headers) before registering your net devices. >> >> Thanks. >> >> "maximum length of headers" might be a bit tricky to determine >> generically :-(. > > Well you don''t need to be generic, you need to know the maximum length > of headers that might appear in a TSO skb. > > Ethernet + VLAN tag + IPv6 + TCP + timestamp option = 90 bytes, but I''m > not sure whether there can be other IP or TCP options in a TSO skb. I''d > really like to get the TSO requirements clearly documented somewhere.What about encapsulated IPSEC, IP-in-IP-tunnels, etc. ? Christoph
Ben Hutchings
2013-Apr-09 14:59 UTC
Re: [PATCH 2/4] xen-netfront: drop skb when skb->len > 65535
On Tue, 2013-04-09 at 16:53 +0200, Christoph Egger wrote:> On 09.04.13 16:45, Ben Hutchings wrote: > > On Tue, 2013-04-09 at 15:30 +0100, Ian Campbell wrote: > >> On Tue, 2013-03-19 at 21:28 +0000, Ben Hutchings wrote: > >>> On Tue, 2013-03-19 at 21:24 +0000, Ben Hutchings wrote: > >>>> On Mon, 2013-03-18 at 15:07 +0000, Ian Campbell wrote: > >>>>> On Mon, 2013-03-18 at 15:04 +0000, Wei Liu wrote: > >>>>>> On Mon, 2013-03-18 at 14:54 +0000, Ian Campbell wrote: > >>>>>>> On Mon, 2013-03-18 at 14:40 +0000, Wei Liu wrote: > >>>>>>>> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > >>>>>>>>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > >>>>>>>>>> The `size'' field of Xen network wire format is uint16_t, anything bigger than > >>>>>>>>>> 65535 will cause overflow. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/net/xen-netfront.c | 12 ++++++++++++ > >>>>>>>>>> 1 file changed, 12 insertions(+) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > >>>>>>>>>> index 5527663..8c3d065 100644 > >>>>>>>>>> --- a/drivers/net/xen-netfront.c > >>>>>>>>>> +++ b/drivers/net/xen-netfront.c > >>>>>>>>>> @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > >>>>>>>>>> unsigned int len = skb_headlen(skb); > >>>>>>>>>> unsigned long flags; > >>>>>>>>>> > >>>>>>>>>> + /* > >>>>>>>>>> + * wire format of xen_netif_tx_request only supports skb->len > >>>>>>>>>> + * < 64K, because size field in xen_netif_tx_request is > >>>>>>>>>> + * uint16_t. > >>>>>>>>> > >>>>>>>>> Is there some field we can set e.g. in struct ethernet_device which > >>>>>>>>> would stop this from happening? > >>>>>>>>> > >>>>>>>> > >>>>>>>> struct ethernet_device? I could not find it. > >>>>>>>> > >>>>>>>> And for struct net_device, > >>>>>>> > >>>>>>> I meant struct net_device. > >>>>>>> > >>>>>>>> there is no field for this AFAICT. > >>>>>>> > >>>>>>> Interesting. Are hardware devices expected to cope with arbitrary sized > >>>>>>> GSO skbs then I wonder. > >>>>>>> > >>>>>> > >>>>>> No idea. But there is a macro called GSO_MAX_SIZE (65536) in struct > >>>>>> net_device. :-) > >>>>> > >>>>> But aren''t we seeing skb''s bigger than that? > >>>>> > >>>>> Maybe this is just a historical bug in some older guests? > >>>> > >>>> GSO_MAX_SIZE is the maximum payload length, not the maximum total length > >>>> of an skb. > >>> > >>> ...and it''s actually just the default value assigned to > >>> dev->gso_max_size. You''ll want to change it to your actual maximum > >>> (65535 - maximum length of headers) before registering your net devices. > >> > >> Thanks. > >> > >> "maximum length of headers" might be a bit tricky to determine > >> generically :-(. > > > > Well you don''t need to be generic, you need to know the maximum length > > of headers that might appear in a TSO skb. > > > > Ethernet + VLAN tag + IPv6 + TCP + timestamp option = 90 bytes, but I''m > > not sure whether there can be other IP or TCP options in a TSO skb. I''d > > really like to get the TSO requirements clearly documented somewhere. > > What about encapsulated IPSEC, IP-in-IP-tunnels, etc. ?xen-netfront doesn''t offload GSO for those, unless I''m much mistaken. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that''s the marketing department''s job. They asked us to note that Solarflare product names are trademarked.