diff mbox

[6/6] xen-netback: don't disconnect frontend when seeing oversize frame

Message ID 1364209702-12437-7-git-send-email-wei.liu2@citrix.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Wei Liu March 25, 2013, 11:08 a.m. UTC 
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>
---
 drivers/net/xen-netback/netback.c |   32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

Comments

David Vrabel March 25, 2013, 11:47 a.m. UTC | #1 
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
Wei Liu March 25, 2013, noon UTC | #2 
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
Jan Beulich March 25, 2013, 12:53 p.m. UTC | #3 
>>> 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
Wei Liu March 25, 2013, 1:52 p.m. UTC | #4 
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
David Miller March 25, 2013, 4:21 p.m. UTC | #5 
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
Konrad Rzeszutek Wilk March 25, 2013, 4:58 p.m. UTC | #6 
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
William Dauchy March 27, 2013, 10:03 a.m. UTC | #7 
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 mbox

Patch

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;