diff mbox series

[5/8] xen/netfront: read response from backend only once

Message ID 20210513100302.22027-6-jgross@suse.com
State New
Headers show
Series xen: harden frontends against malicious backends | expand

Commit Message

Jürgen Groß May 13, 2021, 10:02 a.m. UTC 
In order to avoid problems in case the backend is modifying a response
on the ring page while the frontend has already seen it, just read the
response into a local buffer in one go and then operate on that buffer
only.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/net/xen-netfront.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

Comments

Jan Beulich May 17, 2021, 2:20 p.m. UTC | #1 
On 13.05.2021 12:02, Juergen Gross wrote:
> In order to avoid problems in case the backend is modifying a response

> on the ring page while the frontend has already seen it, just read the

> response into a local buffer in one go and then operate on that buffer

> only.

> 

> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Jan Beulich <jbeulich@suse.com>

with one remark:

> @@ -830,24 +830,22 @@ static int xennet_get_extras(struct netfront_queue *queue,

>  			break;

>  		}

>  

> -		extra = (struct xen_netif_extra_info *)

> -			RING_GET_RESPONSE(&queue->rx, ++cons);

> +		RING_COPY_RESPONSE(&queue->rx, ++cons, &extra);

>  

> -		if (unlikely(!extra->type ||

> -			     extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {

> +		if (unlikely(!extra.type ||

> +			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {

>  			if (net_ratelimit())

>  				dev_warn(dev, "Invalid extra type: %d\n",

> -					extra->type);

> +					extra.type);

>  			err = -EINVAL;

>  		} else {

> -			memcpy(&extras[extra->type - 1], extra,

> -			       sizeof(*extra));

> +			memcpy(&extras[extra.type - 1], &extra, sizeof(extra));


Maybe take the opportunity and switch to (type safe) structure
assignment?

Jan
Jürgen Groß May 17, 2021, 2:24 p.m. UTC | #2 
On 17.05.21 16:20, Jan Beulich wrote:
> On 13.05.2021 12:02, Juergen Gross wrote:

>> In order to avoid problems in case the backend is modifying a response

>> on the ring page while the frontend has already seen it, just read the

>> response into a local buffer in one go and then operate on that buffer

>> only.

>>

>> Signed-off-by: Juergen Gross <jgross@suse.com>

> 

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

> with one remark:

> 

>> @@ -830,24 +830,22 @@ static int xennet_get_extras(struct netfront_queue *queue,

>>   			break;

>>   		}

>>   

>> -		extra = (struct xen_netif_extra_info *)

>> -			RING_GET_RESPONSE(&queue->rx, ++cons);

>> +		RING_COPY_RESPONSE(&queue->rx, ++cons, &extra);

>>   

>> -		if (unlikely(!extra->type ||

>> -			     extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {

>> +		if (unlikely(!extra.type ||

>> +			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {

>>   			if (net_ratelimit())

>>   				dev_warn(dev, "Invalid extra type: %d\n",

>> -					extra->type);

>> +					extra.type);

>>   			err = -EINVAL;

>>   		} else {

>> -			memcpy(&extras[extra->type - 1], extra,

>> -			       sizeof(*extra));

>> +			memcpy(&extras[extra.type - 1], &extra, sizeof(extra));

> 

> Maybe take the opportunity and switch to (type safe) structure

> assignment?


Yes, good idea.


Juergen
diff mbox series

Patch

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 44275908d61a..f91e41ece554 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -399,13 +399,13 @@  static void xennet_tx_buf_gc(struct netfront_queue *queue)
 		rmb(); /* Ensure we see responses up to 'rp'. */
 
 		for (cons = queue->tx.rsp_cons; cons != prod; cons++) {
-			struct xen_netif_tx_response *txrsp;
+			struct xen_netif_tx_response txrsp;
 
-			txrsp = RING_GET_RESPONSE(&queue->tx, cons);
-			if (txrsp->status == XEN_NETIF_RSP_NULL)
+			RING_COPY_RESPONSE(&queue->tx, cons, &txrsp);
+			if (txrsp.status == XEN_NETIF_RSP_NULL)
 				continue;
 
-			id  = txrsp->id;
+			id  = txrsp.id;
 			skb = queue->tx_skbs[id].skb;
 			if (unlikely(gnttab_query_foreign_access(
 				queue->grant_tx_ref[id]) != 0)) {
@@ -814,7 +814,7 @@  static int xennet_get_extras(struct netfront_queue *queue,
 			     RING_IDX rp)
 
 {
-	struct xen_netif_extra_info *extra;
+	struct xen_netif_extra_info extra;
 	struct device *dev = &queue->info->netdev->dev;
 	RING_IDX cons = queue->rx.rsp_cons;
 	int err = 0;
@@ -830,24 +830,22 @@  static int xennet_get_extras(struct netfront_queue *queue,
 			break;
 		}
 
-		extra = (struct xen_netif_extra_info *)
-			RING_GET_RESPONSE(&queue->rx, ++cons);
+		RING_COPY_RESPONSE(&queue->rx, ++cons, &extra);
 
-		if (unlikely(!extra->type ||
-			     extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
+		if (unlikely(!extra.type ||
+			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
 			if (net_ratelimit())
 				dev_warn(dev, "Invalid extra type: %d\n",
-					extra->type);
+					extra.type);
 			err = -EINVAL;
 		} else {
-			memcpy(&extras[extra->type - 1], extra,
-			       sizeof(*extra));
+			memcpy(&extras[extra.type - 1], &extra, sizeof(extra));
 		}
 
 		skb = xennet_get_rx_skb(queue, cons);
 		ref = xennet_get_rx_ref(queue, cons);
 		xennet_move_rx_slot(queue, skb, ref);
-	} while (extra->flags & XEN_NETIF_EXTRA_FLAG_MORE);
+	} while (extra.flags & XEN_NETIF_EXTRA_FLAG_MORE);
 
 	queue->rx.rsp_cons = cons;
 	return err;
@@ -905,7 +903,7 @@  static int xennet_get_responses(struct netfront_queue *queue,
 				struct sk_buff_head *list,
 				bool *need_xdp_flush)
 {
-	struct xen_netif_rx_response *rx = &rinfo->rx;
+	struct xen_netif_rx_response *rx = &rinfo->rx, rx_local;
 	int max = XEN_NETIF_NR_SLOTS_MIN + (rx->status <= RX_COPY_THRESHOLD);
 	RING_IDX cons = queue->rx.rsp_cons;
 	struct sk_buff *skb = xennet_get_rx_skb(queue, cons);
@@ -989,7 +987,8 @@  static int xennet_get_responses(struct netfront_queue *queue,
 			break;
 		}
 
-		rx = RING_GET_RESPONSE(&queue->rx, cons + slots);
+		RING_COPY_RESPONSE(&queue->rx, cons + slots, &rx_local);
+		rx = &rx_local;
 		skb = xennet_get_rx_skb(queue, cons + slots);
 		ref = xennet_get_rx_ref(queue, cons + slots);
 		slots++;
@@ -1044,10 +1043,11 @@  static int xennet_fill_frags(struct netfront_queue *queue,
 	struct sk_buff *nskb;
 
 	while ((nskb = __skb_dequeue(list))) {
-		struct xen_netif_rx_response *rx =
-			RING_GET_RESPONSE(&queue->rx, ++cons);
+		struct xen_netif_rx_response rx;
 		skb_frag_t *nfrag = &skb_shinfo(nskb)->frags[0];
 
+		RING_COPY_RESPONSE(&queue->rx, ++cons, &rx);
+
 		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS) {
 			unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
 
@@ -1062,7 +1062,7 @@  static int xennet_fill_frags(struct netfront_queue *queue,
 
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
 				skb_frag_page(nfrag),
-				rx->offset, rx->status, PAGE_SIZE);
+				rx.offset, rx.status, PAGE_SIZE);
 
 		skb_shinfo(nskb)->nr_frags = 0;
 		kfree_skb(nskb);
@@ -1161,7 +1161,7 @@  static int xennet_poll(struct napi_struct *napi, int budget)
 	i = queue->rx.rsp_cons;
 	work_done = 0;
 	while ((i != rp) && (work_done < budget)) {
-		memcpy(rx, RING_GET_RESPONSE(&queue->rx, i), sizeof(*rx));
+		RING_COPY_RESPONSE(&queue->rx, i, rx);
 		memset(extras, 0, sizeof(rinfo.extras));
 
 		err = xennet_get_responses(queue, &rinfo, rp, &tmpq,