thr3ads.net - Xen devel - [Xen-devel] [NET] back: Copy tx_ring data before verification [Oct 2006]

If this information is useful, please help other people find it:
Share via:

Herbert Xu

2006-Oct-11 09:46 UTC

[Xen-devel] [NET] back: Copy tx_ring data before verification

Hi Keir:

This patch fixes a potential security problem should the
frontend attempt to change the tx_ring entries under us.

[NET] back: Copy tx_ring data before verification

We need to make a copy of data from tx_ring before
verifying them as otherwise what we end up using may
be different from what was verified.

Signed-off-by: Herbert Xu herbert@gondor.apana.org.au>

Cheers,
-- 
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
--
diff -r f7d65fb7299b linux-2.6-xen-sparse/drivers/xen/netback/netback.c
--- a/linux-2.6-xen-sparse/drivers/xen/netback/netback.c	Tue Oct 10 22:04:01
2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/netback/netback.c	Wed Oct 11 17:11:57
2006 +0800
@@ -875,10 +875,9 @@ static void netbk_tx_err(netif_t *netif,
 	netif_put(netif);
 }
 
-static int netbk_count_requests(netif_t *netif, netif_tx_request_t *txp,
-				int work_to_do)
-{
-	netif_tx_request_t *first = txp;
+static int netbk_count_requests(netif_t *netif, netif_tx_request_t *first,
+				netif_tx_request_t *txp, int work_to_do)
+{
 	RING_IDX cons = netif->tx.req_cons;
 	int frags = 0;
 
@@ -888,7 +887,13 @@ static int netbk_count_requests(netif_t 
 			return -frags;
 		}
 
-		txp = RING_GET_REQUEST(&netif->tx, cons + frags);
+		if (unlikely(frags >= MAX_SKB_FRAGS)) {
+			DPRINTK("Too many frags\n");
+			return -frags;
+		}
+
+		memcpy(txp, RING_GET_REQUEST(&netif->tx, cons + frags),
+		       sizeof(*txp));
 		if (txp->size > first->size) {
 			DPRINTK("Frags galore\n");
 			return -frags;
@@ -902,6 +907,8 @@ static int netbk_count_requests(netif_t 
 				txp->offset, txp->size);
 			return -frags;
 		}
+
+		txp++;
 	}
 
 	return frags;
@@ -909,20 +916,18 @@ static int netbk_count_requests(netif_t 
 
 static gnttab_map_grant_ref_t *netbk_get_requests(netif_t *netif,
 						  struct sk_buff *skb,
+						  netif_tx_request_t *txp,
 						  gnttab_map_grant_ref_t *mop)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	skb_frag_t *frags = shinfo->frags;
-	netif_tx_request_t *txp;
 	unsigned long pending_idx = *((u16 *)skb->data);
-	RING_IDX cons = netif->tx.req_cons;
 	int i, start;
 
 	/* Skip first skb fragment if it is on same page as header fragment. */
 	start = ((unsigned long)shinfo->frags[0].page == pending_idx);
 
-	for (i = start; i < shinfo->nr_frags; i++) {
-		txp = RING_GET_REQUEST(&netif->tx, cons++);
+	for (i = start; i < shinfo->nr_frags; i++, txp++) {
 		pending_idx = pending_ring[MASK_PEND_IDX(pending_cons++)];
 
 		gnttab_set_map_op(mop++, idx_to_kaddr(pending_idx),
@@ -1036,7 +1041,7 @@ int netbk_get_extras(netif_t *netif, str
 int netbk_get_extras(netif_t *netif, struct netif_extra_info *extras,
 		     int work_to_do)
 {
-	struct netif_extra_info *extra;
+	struct netif_extra_info extra;
 	RING_IDX cons = netif->tx.req_cons;
 
 	do {
@@ -1045,18 +1050,18 @@ int netbk_get_extras(netif_t *netif, str
 			return -EBADR;
 		}
 
-		extra = (struct netif_extra_info *)
-			RING_GET_REQUEST(&netif->tx, cons);
-		if (unlikely(!extra->type ||
-			     extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
+		memcpy(&extra, RING_GET_REQUEST(&netif->tx, cons),
+		       sizeof(extra));
+		if (unlikely(!extra.type ||
+			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
 			netif->tx.req_cons = ++cons;
-			DPRINTK("Invalid extra type: %d\n", extra->type);
+			DPRINTK("Invalid extra type: %d\n", extra.type);
 			return -EINVAL;
 		}
 
-		memcpy(&extras[extra->type - 1], extra, sizeof(*extra));
+		memcpy(&extras[extra.type - 1], &extra, sizeof(extra));
 		netif->tx.req_cons = ++cons;
-	} while (extra->flags & XEN_NETIF_EXTRA_FLAG_MORE);
+	} while (extra.flags & XEN_NETIF_EXTRA_FLAG_MORE);
 
 	return work_to_do;
 }
@@ -1091,6 +1096,7 @@ static void net_tx_action(unsigned long 
 	struct sk_buff *skb;
 	netif_t *netif;
 	netif_tx_request_t txreq;
+	netif_tx_request_t txfrags[MAX_SKB_FRAGS];
 	struct netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX - 1];
 	u16 pending_idx;
 	RING_IDX i;
@@ -1167,18 +1173,12 @@ static void net_tx_action(unsigned long 
 			}
 		}
 
-		ret = netbk_count_requests(netif, &txreq, work_to_do);
+		ret = netbk_count_requests(netif, &txreq, txfrags, work_to_do);
 		if (unlikely(ret < 0)) {
 			netbk_tx_err(netif, &txreq, i - ret);
 			continue;
 		}
 		i += ret;
-
-		if (unlikely(ret > MAX_SKB_FRAGS)) {
-			DPRINTK("Too many frags\n");
-			netbk_tx_err(netif, &txreq, i);
-			continue;
-		}
 
 		if (unlikely(txreq.size < ETH_HLEN)) {
 			DPRINTK("Bad packet size: %d\n", txreq.size);
@@ -1248,7 +1248,7 @@ static void net_tx_action(unsigned long 
 
 		pending_cons++;
 
-		mop = netbk_get_requests(netif, skb, mop);
+		mop = netbk_get_requests(netif, skb, txfrags, mop);
 
 		netif->tx.req_cons = i;
 		netif_schedule_work(netif);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-Oct-12 13:54 UTC

head link

Re: [Xen-devel] [NET] back: Copy tx_ring data before verification

On 11/10/06 10:46, "Herbert Xu" <herbert@gondor.apana.org.au>
wrote:
> This patch fixes a potential security problem should the
> frontend attempt to change the tx_ring entries under us.
> 
> [NET] back: Copy tx_ring data before verification
> 
> We need to make a copy of data from tx_ring before
> verifying them as otherwise what we end up using may
> be different from what was verified.
I would very much like to take this patch, but right now it does not work!

# scp bigfile remotehost:/tmp/.
Disconnecting: Bad packet length 1349676916.
Write failed: Connection reset by peer
lost connection

This is with your patch applied on xen-unstable 11748:af1aa35265eb. The scp
is happening from a PV guest running Centos4 and no special networking
options (e.g., *not* using the rx-copy path, although the bug is
reproducible either way). TSO etc are all enabled (since they are defaults).

 -- Keir

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Herbert Xu

2006-Oct-12 15:02 UTC

head link

Re: [Xen-devel] [NET] back: Copy tx_ring data before verification

On Thu, Oct 12, 2006 at 02:54:14PM +0100, Keir Fraser
wrote:>
> I would very much like to take this patch, but right now it does not work!
How embarrassing! This version should work better:

[NET] back: Copy tx_ring data before verification

We need to make a copy of data from tx_ring before
verifying them as otherwise what we end up using may
be different from what was verified.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
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
--
diff -r f7d65fb7299b linux-2.6-xen-sparse/drivers/xen/netback/netback.c
--- a/linux-2.6-xen-sparse/drivers/xen/netback/netback.c	Tue Oct 10 22:04:01
2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/netback/netback.c	Thu Oct 12 23:00:24
2006 +0800
@@ -875,20 +875,28 @@ static void netbk_tx_err(netif_t *netif,
 	netif_put(netif);
 }
 
-static int netbk_count_requests(netif_t *netif, netif_tx_request_t *txp,
-				int work_to_do)
-{
-	netif_tx_request_t *first = txp;
+static int netbk_count_requests(netif_t *netif, netif_tx_request_t *first,
+				netif_tx_request_t *txp, int work_to_do)
+{
 	RING_IDX cons = netif->tx.req_cons;
 	int frags = 0;
 
-	while (txp->flags & NETTXF_more_data) {
+	if (!(first->flags & NETTXF_more_data))
+		return 0;
+
+	do {
 		if (frags >= work_to_do) {
 			DPRINTK("Need more frags\n");
 			return -frags;
 		}
 
-		txp = RING_GET_REQUEST(&netif->tx, cons + frags);
+		if (unlikely(frags >= MAX_SKB_FRAGS)) {
+			DPRINTK("Too many frags\n");
+			return -frags;
+		}
+
+		memcpy(txp, RING_GET_REQUEST(&netif->tx, cons + frags),
+		       sizeof(*txp));
 		if (txp->size > first->size) {
 			DPRINTK("Frags galore\n");
 			return -frags;
@@ -902,27 +910,25 @@ static int netbk_count_requests(netif_t 
 				txp->offset, txp->size);
 			return -frags;
 		}
-	}
+	} while ((txp++)->flags & NETTXF_more_data);
 
 	return frags;
 }
 
 static gnttab_map_grant_ref_t *netbk_get_requests(netif_t *netif,
 						  struct sk_buff *skb,
+						  netif_tx_request_t *txp,
 						  gnttab_map_grant_ref_t *mop)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	skb_frag_t *frags = shinfo->frags;
-	netif_tx_request_t *txp;
 	unsigned long pending_idx = *((u16 *)skb->data);
-	RING_IDX cons = netif->tx.req_cons;
 	int i, start;
 
 	/* Skip first skb fragment if it is on same page as header fragment. */
 	start = ((unsigned long)shinfo->frags[0].page == pending_idx);
 
-	for (i = start; i < shinfo->nr_frags; i++) {
-		txp = RING_GET_REQUEST(&netif->tx, cons++);
+	for (i = start; i < shinfo->nr_frags; i++, txp++) {
 		pending_idx = pending_ring[MASK_PEND_IDX(pending_cons++)];
 
 		gnttab_set_map_op(mop++, idx_to_kaddr(pending_idx),
@@ -1036,7 +1042,7 @@ int netbk_get_extras(netif_t *netif, str
 int netbk_get_extras(netif_t *netif, struct netif_extra_info *extras,
 		     int work_to_do)
 {
-	struct netif_extra_info *extra;
+	struct netif_extra_info extra;
 	RING_IDX cons = netif->tx.req_cons;
 
 	do {
@@ -1045,18 +1051,18 @@ int netbk_get_extras(netif_t *netif, str
 			return -EBADR;
 		}
 
-		extra = (struct netif_extra_info *)
-			RING_GET_REQUEST(&netif->tx, cons);
-		if (unlikely(!extra->type ||
-			     extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
+		memcpy(&extra, RING_GET_REQUEST(&netif->tx, cons),
+		       sizeof(extra));
+		if (unlikely(!extra.type ||
+			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
 			netif->tx.req_cons = ++cons;
-			DPRINTK("Invalid extra type: %d\n", extra->type);
+			DPRINTK("Invalid extra type: %d\n", extra.type);
 			return -EINVAL;
 		}
 
-		memcpy(&extras[extra->type - 1], extra, sizeof(*extra));
+		memcpy(&extras[extra.type - 1], &extra, sizeof(extra));
 		netif->tx.req_cons = ++cons;
-	} while (extra->flags & XEN_NETIF_EXTRA_FLAG_MORE);
+	} while (extra.flags & XEN_NETIF_EXTRA_FLAG_MORE);
 
 	return work_to_do;
 }
@@ -1091,6 +1097,7 @@ static void net_tx_action(unsigned long 
 	struct sk_buff *skb;
 	netif_t *netif;
 	netif_tx_request_t txreq;
+	netif_tx_request_t txfrags[MAX_SKB_FRAGS];
 	struct netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX - 1];
 	u16 pending_idx;
 	RING_IDX i;
@@ -1167,18 +1174,12 @@ static void net_tx_action(unsigned long 
 			}
 		}
 
-		ret = netbk_count_requests(netif, &txreq, work_to_do);
+		ret = netbk_count_requests(netif, &txreq, txfrags, work_to_do);
 		if (unlikely(ret < 0)) {
 			netbk_tx_err(netif, &txreq, i - ret);
 			continue;
 		}
 		i += ret;
-
-		if (unlikely(ret > MAX_SKB_FRAGS)) {
-			DPRINTK("Too many frags\n");
-			netbk_tx_err(netif, &txreq, i);
-			continue;
-		}
 
 		if (unlikely(txreq.size < ETH_HLEN)) {
 			DPRINTK("Bad packet size: %d\n", txreq.size);
@@ -1248,7 +1249,7 @@ static void net_tx_action(unsigned long 
 
 		pending_cons++;
 
-		mop = netbk_get_requests(netif, skb, mop);
+		mop = netbk_get_requests(netif, skb, txfrags, mop);
 
 		netif->tx.req_cons = i;
 		netif_schedule_work(netif);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Oct 2006 - [NET] back: Copy tx_ring data before verification

[Xen-devel] [NET] back: Copy tx_ring data before verification

Re: [Xen-devel] [NET] back: Copy tx_ring data before verification

Re: [Xen-devel] [NET] back: Copy tx_ring data before verification