Message ID | 1364209702-12437-7-git-send-email-wei.liu2@citrix.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 25/03/13 11:08, Wei Liu wrote: > Some buggy frontends may generate frames larger than 64 KiB. We should > aggresively consume all slots and drop the packet instead of disconnecting the > frontend. The following is the changeset description I wrote internally. It's a bit more descriptive. Apologies for not sending out a proper patch in the first place. "Some frontend drivers are sending packets >= 64 KiB in length. This length overflows the length field in the first frag making the following frags have an invalid length ("Frag is bigger than frame"). Turn this error back into a non-fatal error by dropping the packet. To avoid having the following frags having fatal errors, consume all frags in the packet. This does not reopen the security hole as if the packet as an invalid number of frags it will still hit this fatal error case." David -- 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 Mon, Mar 25, 2013 at 11:47:17AM +0000, David Vrabel wrote: > On 25/03/13 11:08, Wei Liu wrote: > > Some buggy frontends may generate frames larger than 64 KiB. We should > > aggresively consume all slots and drop the packet instead of disconnecting the > > frontend. > > The following is the changeset description I wrote internally. It's a > bit more descriptive. > > Apologies for not sending out a proper patch in the first place. > > "Some frontend drivers are sending packets >= 64 KiB in length. This > length overflows the length field in the first frag making the > following frags have an invalid length ("Frag is bigger than frame"). > > Turn this error back into a non-fatal error by dropping the packet. > To avoid having the following frags having fatal errors, consume all > frags in the packet. > > This does not reopen the security hole as if the packet as an invalid > number of frags it will still hit this fatal error case." > Thanks. Overall this looks good. I will need to change 'frags' to 'slots' though. Wei. > David -- 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 25.03.13 at 12:08, Wei Liu <wei.liu2@citrix.com> wrote: > @@ -947,10 +949,21 @@ static int netbk_count_requests(struct xenvif *vif, > > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots), > sizeof(*txp)); > - if (txp->size > first->size) { > - netdev_err(vif->dev, "Packet 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 slots will > + * appear to be larger than the frame. > + * > + * This cannot be fatal error as there are buggy > + * frontends that do this. > + * > + * Consume all slots and drop the packet. > + */ > + if (!drop && txp->size > first->size) { > + if (net_ratelimit()) > + netdev_dbg(vif->dev, > + "Packet is bigger than frame.\n"); > + drop = true; So this deals with one half of the problem, but shouldn't we also revert the disconnect when slots would exceed MAX_SKB_FRAGS (or max_skb_slots after patch 5)? Afaict you could trivially extend this patch to also cover that case... Jan -- 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 Mon, Mar 25, 2013 at 12:53:57PM +0000, Jan Beulich wrote: > >>> On 25.03.13 at 12:08, Wei Liu <wei.liu2@citrix.com> wrote: > > @@ -947,10 +949,21 @@ static int netbk_count_requests(struct xenvif *vif, > > > > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots), > > sizeof(*txp)); > > - if (txp->size > first->size) { > > - netdev_err(vif->dev, "Packet 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 slots will > > + * appear to be larger than the frame. > > + * > > + * This cannot be fatal error as there are buggy > > + * frontends that do this. > > + * > > + * Consume all slots and drop the packet. > > + */ > > + if (!drop && txp->size > first->size) { > > + if (net_ratelimit()) > > + netdev_dbg(vif->dev, > > + "Packet is bigger than frame.\n"); > > + drop = true; > > So this deals with one half of the problem, but shouldn't we also > revert the disconnect when slots would exceed MAX_SKB_FRAGS > (or max_skb_slots after patch 5)? Afaict you could trivially extend > this patch to also cover that case... > I don't think we should do that. IMO a guest using too many slots should be considered malicious and disconnected. Wei. > Jan -- 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: Wei Liu <wei.liu2@citrix.com> Date: Mon, 25 Mar 2013 11:08:22 +0000 > Some buggy frontends may generate frames larger than 64 KiB. We should > aggresively consume all slots and drop the packet instead of disconnecting the > frontend. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> Similarly, resubmit with the more descriptive commit message David posted and also integrate any other feedback you've received. -- 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 Mon, Mar 25, 2013 at 11:08:22AM +0000, Wei Liu wrote: > Some buggy frontends may generate frames larger than 64 KiB. We should > aggresively consume all slots and drop the packet instead of disconnecting the > frontend. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> CC stable@vger.kernel.org ? > --- > drivers/net/xen-netback/netback.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index a634dc5..1971623 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -924,10 +924,12 @@ 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, > + RING_IDX first_idx) > { > RING_IDX cons = vif->tx.req_cons; > int slots = 0; > + bool drop = false; > > if (!(first->flags & XEN_NETTXF_more_data)) > return 0; > @@ -947,10 +949,21 @@ static int netbk_count_requests(struct xenvif *vif, > > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots), > sizeof(*txp)); > - if (txp->size > first->size) { > - netdev_err(vif->dev, "Packet 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 slots will > + * appear to be larger than the frame. > + * > + * This cannot be fatal error as there are buggy > + * frontends that do this. > + * > + * Consume all slots and drop the packet. > + */ > + if (!drop && txp->size > first->size) { > + if (net_ratelimit()) > + netdev_dbg(vif->dev, > + "Packet is bigger than frame.\n"); > + drop = true; > } > > first->size -= txp->size; > @@ -963,6 +976,12 @@ static int netbk_count_requests(struct xenvif *vif, > return -EINVAL; > } > } while ((txp++)->flags & XEN_NETTXF_more_data); > + > + if (drop) { > + netbk_tx_err(vif, first, first_idx + slots); > + return -EIO; > + } > + > return slots; > } > > @@ -1429,7 +1448,8 @@ 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; > > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > -- 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 Mon, Mar 25, 2013 at 5:58 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> CC stable@vger.kernel.org ?
I agree on that. I got several issues with the previous patch on a 3.4.x
Regards,
--
William
--
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/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index a634dc5..1971623 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -924,10 +924,12 @@ 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, + RING_IDX first_idx) { RING_IDX cons = vif->tx.req_cons; int slots = 0; + bool drop = false; if (!(first->flags & XEN_NETTXF_more_data)) return 0; @@ -947,10 +949,21 @@ static int netbk_count_requests(struct xenvif *vif, memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots), sizeof(*txp)); - if (txp->size > first->size) { - netdev_err(vif->dev, "Packet 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 slots will + * appear to be larger than the frame. + * + * This cannot be fatal error as there are buggy + * frontends that do this. + * + * Consume all slots and drop the packet. + */ + if (!drop && txp->size > first->size) { + if (net_ratelimit()) + netdev_dbg(vif->dev, + "Packet is bigger than frame.\n"); + drop = true; } first->size -= txp->size; @@ -963,6 +976,12 @@ static int netbk_count_requests(struct xenvif *vif, return -EINVAL; } } while ((txp++)->flags & XEN_NETTXF_more_data); + + if (drop) { + netbk_tx_err(vif, first, first_idx + slots); + return -EIO; + } + return slots; } @@ -1429,7 +1448,8 @@ 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;