[PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring()

[PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring()

[Dongli Zhang]

As 'be->blkif' is used for many times in connect_ring(), the stack variable
'blkif' is added to substitute 'be-blkif'.

Suggested-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/block/xen-blkback/xenbus.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index a4bc74e..a4aadac 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -1023,6 +1023,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 static int connect_ring(struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;
+	struct xen_blkif *blkif = be->blkif;
 	unsigned int pers_grants;
 	char protocol[64] = "";
 	int err, i;
@@ -1033,25 +1034,25 @@ static int connect_ring(struct backend_info *be)
 
 	pr_debug("%s %s\n", __func__, dev->otherend);
 
-	be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
+	blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
 	err = xenbus_scanf(XBT_NIL, dev->otherend, "protocol",
 			   "%63s", protocol);
 	if (err <= 0)
 		strcpy(protocol, "unspecified, assuming default");
 	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
-		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+		blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
 	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
-		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+		blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
 	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
-		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+		blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
 	else {
 		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
 		return -ENOSYS;
 	}
 	pers_grants = xenbus_read_unsigned(dev->otherend, "feature-persistent",
 					   0);
-	be->blkif->vbd.feature_gnt_persistent = pers_grants;
-	be->blkif->vbd.overflow_max_grants = 0;
+	blkif->vbd.feature_gnt_persistent = pers_grants;
+	blkif->vbd.overflow_max_grants = 0;
 
 	/*
 	 * Read the number of hardware queues from frontend.
@@ -1067,16 +1068,16 @@ static int connect_ring(struct backend_info *be)
 				requested_num_queues, xenblk_max_queues);
 		return -ENOSYS;
 	}
-	be->blkif->nr_rings = requested_num_queues;
-	if (xen_blkif_alloc_rings(be->blkif))
+	blkif->nr_rings = requested_num_queues;
+	if (xen_blkif_alloc_rings(blkif))
 		return -ENOMEM;
 
 	pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
-		 be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
+		 blkif->nr_rings, blkif->blk_protocol, protocol,
 		 pers_grants ? "persistent grants" : "");
 
-	if (be->blkif->nr_rings == 1)
-		return read_per_ring_refs(&be->blkif->rings[0], dev->otherend);
+	if (blkif->nr_rings == 1)
+		return read_per_ring_refs(&blkif->rings[0], dev->otherend);
 	else {
 		xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
 		xspath = kmalloc(xspathsize, GFP_KERNEL);
@@ -1085,10 +1086,10 @@ static int connect_ring(struct backend_info *be)
 			return -ENOMEM;
 		}
 
-		for (i = 0; i < be->blkif->nr_rings; i++) {
+		for (i = 0; i < blkif->nr_rings; i++) {
 			memset(xspath, 0, xspathsize);
 			snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend, i);
-			err = read_per_ring_refs(&be->blkif->rings[i], xspath);
+			err = read_per_ring_refs(&blkif->rings[i], xspath);
 			if (err) {
 				kfree(xspath);
 				return err;
-- 
2.7.4

[PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Dongli Zhang]

The xenstore 'ring-page-order' is used globally for each blkback queue and
therefore should be read from xenstore only once. However, it is obtained
in read_per_ring_refs() which might be called multiple times during the
initialization of each blkback queue.

If the blkfront is malicious and the 'ring-page-order' is set in different
value by blkfront every time before blkback reads it, this may end up at
the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
xen_blkif_disconnect() when frontend is destroyed.

This patch reworks connect_ring() to read xenstore 'ring-page-order' only
once.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v1:
  * change the order of xenstore read in read_per_ring_refs
  * use xenbus_read_unsigned() in connect_ring()

Changed since v2:
  * simplify the condition check as "(err != 1 && nr_grefs > 1)"
  * avoid setting err as -EINVAL to remove extra one line of code

Changed since v3:
  * exit at the beginning if !nr_grefs
  * change the if statements to avoid test (err != 1) twice
  * initialize a 'blkif' stack variable (refer to PATCH 1/2)

 drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index a4aadac..a2acbc9 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 	int err, i, j;
 	struct xen_blkif *blkif = ring->blkif;
 	struct xenbus_device *dev = blkif->be->dev;
-	unsigned int ring_page_order, nr_grefs, evtchn;
+	unsigned int nr_grefs, evtchn;
 
 	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
 			  &evtchn);
@@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
 		return err;
 	}
 
-	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
-			  &ring_page_order);
-	if (err != 1) {
-		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
+	nr_grefs = blkif->nr_ring_pages;
+
+	if (unlikely(!nr_grefs))
+		return -EINVAL;
+
+	for (i = 0; i < nr_grefs; i++) {
+		char ring_ref_name[RINGREF_NAME_LEN];
+
+		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
+		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
+				   "%u", &ring_ref[i]);
+
 		if (err != 1) {
-			err = -EINVAL;
-			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
-			return err;
-		}
-		nr_grefs = 1;
-	} else {
-		unsigned int i;
-
-		if (ring_page_order > xen_blkif_max_ring_order) {
-			err = -EINVAL;
-			xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
-					 dir, ring_page_order,
-					 xen_blkif_max_ring_order);
-			return err;
+			if (nr_grefs == 1)
+				break;
+
+			xenbus_dev_fatal(dev, err, "reading %s/%s",
+					 dir, ring_ref_name);
+			return -EINVAL;
 		}
+	}
 
-		nr_grefs = 1 << ring_page_order;
-		for (i = 0; i < nr_grefs; i++) {
-			char ring_ref_name[RINGREF_NAME_LEN];
-
-			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
-			err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
-					   "%u", &ring_ref[i]);
-			if (err != 1) {
-				err = -EINVAL;
-				xenbus_dev_fatal(dev, err, "reading %s/%s",
-						 dir, ring_ref_name);
-				return err;
-			}
+	if (err != 1) {
+		WARN_ON(nr_grefs != 1);
+
+		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
+				   &ring_ref[0]);
+		if (err != 1) {
+			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
+			return -EINVAL;
 		}
 	}
-	blkif->nr_ring_pages = nr_grefs;
 
 	for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
 		req = kzalloc(sizeof(*req), GFP_KERNEL);
@@ -1031,6 +1026,7 @@ static int connect_ring(struct backend_info *be)
 	size_t xspathsize;
 	const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-NNN" */
 	unsigned int requested_num_queues = 0;
+	unsigned int ring_page_order;
 
 	pr_debug("%s %s\n", __func__, dev->otherend);
 
@@ -1076,6 +1072,20 @@ static int connect_ring(struct backend_info *be)
 		 blkif->nr_rings, blkif->blk_protocol, protocol,
 		 pers_grants ? "persistent grants" : "");
 
+	ring_page_order = xenbus_read_unsigned(dev->otherend,
+					       "ring-page-order", 0);
+
+	if (ring_page_order > xen_blkif_max_ring_order) {
+		err = -EINVAL;
+		xenbus_dev_fatal(dev, err,
+				 "requested ring page order %d exceed max:%d",
+				 ring_page_order,
+				 xen_blkif_max_ring_order);
+		return err;
+	}
+
+	blkif->nr_ring_pages = 1 << ring_page_order;
+
 	if (blkif->nr_rings == 1)
 		return read_per_ring_refs(&blkif->rings[0], dev->otherend);
 	else {
-- 
2.7.4

RE: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring()

[Paul Durrant]

> -----Original Message-----
> From: Dongli Zhang [mailto:dongli.zhang@oracle.com]
> Sent: 07 January 2019 05:36
> To: xen-devel@lists.xenproject.org; linux-block@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: konrad.wilk@oracle.com; Roger Pau Monne <roger.pau@citrix.com>;
> axboe@kernel.dk; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in
> connect_ring()
> 
> As 'be->blkif' is used for many times in connect_ring(), the stack
> variable
> 'blkif' is added to substitute 'be-blkif'.
> 
> Suggested-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

That looks better :-)

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  drivers/block/xen-blkback/xenbus.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> blkback/xenbus.c
> index a4bc74e..a4aadac 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -1023,6 +1023,7 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
>  static int connect_ring(struct backend_info *be)
>  {
>  	struct xenbus_device *dev = be->dev;
> +	struct xen_blkif *blkif = be->blkif;
>  	unsigned int pers_grants;
>  	char protocol[64] = "";
>  	int err, i;
> @@ -1033,25 +1034,25 @@ static int connect_ring(struct backend_info *be)
> 
>  	pr_debug("%s %s\n", __func__, dev->otherend);
> 
> -	be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
> +	blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
>  	err = xenbus_scanf(XBT_NIL, dev->otherend, "protocol",
>  			   "%63s", protocol);
>  	if (err <= 0)
>  		strcpy(protocol, "unspecified, assuming default");
>  	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
> -		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
> +		blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
>  	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
> -		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
> +		blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
>  	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
> -		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
> +		blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
>  	else {
>  		xenbus_dev_fatal(dev, err, "unknown fe protocol %s",
> protocol);
>  		return -ENOSYS;
>  	}
>  	pers_grants = xenbus_read_unsigned(dev->otherend, "feature-
> persistent",
>  					   0);
> -	be->blkif->vbd.feature_gnt_persistent = pers_grants;
> -	be->blkif->vbd.overflow_max_grants = 0;
> +	blkif->vbd.feature_gnt_persistent = pers_grants;
> +	blkif->vbd.overflow_max_grants = 0;
> 
>  	/*
>  	 * Read the number of hardware queues from frontend.
> @@ -1067,16 +1068,16 @@ static int connect_ring(struct backend_info *be)
>  				requested_num_queues, xenblk_max_queues);
>  		return -ENOSYS;
>  	}
> -	be->blkif->nr_rings = requested_num_queues;
> -	if (xen_blkif_alloc_rings(be->blkif))
> +	blkif->nr_rings = requested_num_queues;
> +	if (xen_blkif_alloc_rings(blkif))
>  		return -ENOMEM;
> 
>  	pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
> -		 be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
> +		 blkif->nr_rings, blkif->blk_protocol, protocol,
>  		 pers_grants ? "persistent grants" : "");
> 
> -	if (be->blkif->nr_rings == 1)
> -		return read_per_ring_refs(&be->blkif->rings[0], dev-
> >otherend);
> +	if (blkif->nr_rings == 1)
> +		return read_per_ring_refs(&blkif->rings[0], dev->otherend);
>  	else {
>  		xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
>  		xspath = kmalloc(xspathsize, GFP_KERNEL);
> @@ -1085,10 +1086,10 @@ static int connect_ring(struct backend_info *be)
>  			return -ENOMEM;
>  		}
> 
> -		for (i = 0; i < be->blkif->nr_rings; i++) {
> +		for (i = 0; i < blkif->nr_rings; i++) {
>  			memset(xspath, 0, xspathsize);
>  			snprintf(xspath, xspathsize, "%s/queue-%u", dev-
> >otherend, i);
> -			err = read_per_ring_refs(&be->blkif->rings[i], xspath);
> +			err = read_per_ring_refs(&blkif->rings[i], xspath);
>  			if (err) {
>  				kfree(xspath);
>  				return err;
> --
> 2.7.4

RE: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Paul Durrant]

> -----Original Message-----
> From: Dongli Zhang [mailto:dongli.zhang@oracle.com]
> Sent: 07 January 2019 05:36
> To: xen-devel@lists.xenproject.org; linux-block@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: konrad.wilk@oracle.com; Roger Pau Monne <roger.pau@citrix.com>;
> axboe@kernel.dk; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid
> inconsistent xenstore 'ring-page-order' set by malicious blkfront
> 
> The xenstore 'ring-page-order' is used globally for each blkback queue and
> therefore should be read from xenstore only once. However, it is obtained
> in read_per_ring_refs() which might be called multiple times during the
> initialization of each blkback queue.
> 
> If the blkfront is malicious and the 'ring-page-order' is set in different
> value by blkfront every time before blkback reads it, this may end up at
> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> xen_blkif_disconnect() when frontend is destroyed.
> 
> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> once.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> Changed since v1:
>   * change the order of xenstore read in read_per_ring_refs
>   * use xenbus_read_unsigned() in connect_ring()
> 
> Changed since v2:
>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>   * avoid setting err as -EINVAL to remove extra one line of code
> 
> Changed since v3:
>   * exit at the beginning if !nr_grefs
>   * change the if statements to avoid test (err != 1) twice
>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> 
>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++------------
> -----
>  1 file changed, 43 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> blkback/xenbus.c
> index a4aadac..a2acbc9 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
>  	int err, i, j;
>  	struct xen_blkif *blkif = ring->blkif;
>  	struct xenbus_device *dev = blkif->be->dev;
> -	unsigned int ring_page_order, nr_grefs, evtchn;
> +	unsigned int nr_grefs, evtchn;
> 
>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>  			  &evtchn);
> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
>  		return err;
>  	}
> 
> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> -			  &ring_page_order);
> -	if (err != 1) {
> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> &ring_ref[0]);
> +	nr_grefs = blkif->nr_ring_pages;
> +
> +	if (unlikely(!nr_grefs))
> +		return -EINVAL;
> +
> +	for (i = 0; i < nr_grefs; i++) {
> +		char ring_ref_name[RINGREF_NAME_LEN];
> +
> +		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> +		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> +				   "%u", &ring_ref[i]);
> +
>  		if (err != 1) {
> -			err = -EINVAL;
> -			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> -			return err;
> -		}
> -		nr_grefs = 1;
> -	} else {
> -		unsigned int i;
> -
> -		if (ring_page_order > xen_blkif_max_ring_order) {
> -			err = -EINVAL;
> -			xenbus_dev_fatal(dev, err, "%s/request %d ring page
> order exceed max:%d",
> -					 dir, ring_page_order,
> -					 xen_blkif_max_ring_order);
> -			return err;
> +			if (nr_grefs == 1)
> +				break;
> +
> +			xenbus_dev_fatal(dev, err, "reading %s/%s",
> +					 dir, ring_ref_name);

This patch looks much better, but I guess you don't want to be using 'err' in the above call as it will still be set to whatever xenbus_scanf() returned. Probably neatest to just leave the "err = -EINVAL" and "return err" alone.

> +			return -EINVAL;
>  		}
> +	}
> 
> -		nr_grefs = 1 << ring_page_order;
> -		for (i = 0; i < nr_grefs; i++) {
> -			char ring_ref_name[RINGREF_NAME_LEN];
> -
> -			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u",
> i);
> -			err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> -					   "%u", &ring_ref[i]);
> -			if (err != 1) {
> -				err = -EINVAL;
> -				xenbus_dev_fatal(dev, err, "reading %s/%s",
> -						 dir, ring_ref_name);
> -				return err;
> -			}
> +	if (err != 1) {
> +		WARN_ON(nr_grefs != 1);
> +
> +		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> +				   &ring_ref[0]);
> +		if (err != 1) {
> +			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);

Same here. Set err to -EINVAL above the call to xenbus_dev_fatal() and return it below...

> +			return -EINVAL;
>  		}
>  	}
> -	blkif->nr_ring_pages = nr_grefs;
> 
>  	for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
>  		req = kzalloc(sizeof(*req), GFP_KERNEL);
> @@ -1031,6 +1026,7 @@ static int connect_ring(struct backend_info *be)
>  	size_t xspathsize;
>  	const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-
> NNN" */
>  	unsigned int requested_num_queues = 0;
> +	unsigned int ring_page_order;
> 
>  	pr_debug("%s %s\n", __func__, dev->otherend);
> 
> @@ -1076,6 +1072,20 @@ static int connect_ring(struct backend_info *be)
>  		 blkif->nr_rings, blkif->blk_protocol, protocol,
>  		 pers_grants ? "persistent grants" : "");
> 
> +	ring_page_order = xenbus_read_unsigned(dev->otherend,
> +					       "ring-page-order", 0);
> +
> +	if (ring_page_order > xen_blkif_max_ring_order) {
> +		err = -EINVAL;
> +		xenbus_dev_fatal(dev, err,
> +				 "requested ring page order %d exceed max:%d",
> +				 ring_page_order,
> +				 xen_blkif_max_ring_order);
> +		return err;

... just like here :-)

  Paul

> +	}
> +
> +	blkif->nr_ring_pages = 1 << ring_page_order;
> +
>  	if (blkif->nr_rings == 1)
>  		return read_per_ring_refs(&blkif->rings[0], dev->otherend);
>  	else {
> --
> 2.7.4

Re: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring()

[Roger Pau Monné]

On Mon, Jan 07, 2019 at 01:35:58PM +0800, Dongli Zhang wrote:
> As 'be->blkif' is used for many times in connect_ring(), the stack variable
> 'blkif' is added to substitute 'be-blkif'.
> 
> Suggested-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Re: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Roger Pau Monné]

On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> The xenstore 'ring-page-order' is used globally for each blkback queue and
> therefore should be read from xenstore only once. However, it is obtained
> in read_per_ring_refs() which might be called multiple times during the
> initialization of each blkback queue.
> 
> If the blkfront is malicious and the 'ring-page-order' is set in different
> value by blkfront every time before blkback reads it, this may end up at
> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> xen_blkif_disconnect() when frontend is destroyed.
> 
> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> once.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> Changed since v1:
>   * change the order of xenstore read in read_per_ring_refs
>   * use xenbus_read_unsigned() in connect_ring()
> 
> Changed since v2:
>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>   * avoid setting err as -EINVAL to remove extra one line of code
> 
> Changed since v3:
>   * exit at the beginning if !nr_grefs
>   * change the if statements to avoid test (err != 1) twice
>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> 
>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index a4aadac..a2acbc9 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>  	int err, i, j;
>  	struct xen_blkif *blkif = ring->blkif;
>  	struct xenbus_device *dev = blkif->be->dev;
> -	unsigned int ring_page_order, nr_grefs, evtchn;
> +	unsigned int nr_grefs, evtchn;
>  
>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>  			  &evtchn);
> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>  		return err;
>  	}
>  
> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> -			  &ring_page_order);
> -	if (err != 1) {
> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> +	nr_grefs = blkif->nr_ring_pages;
> +
> +	if (unlikely(!nr_grefs))
> +		return -EINVAL;

Is this even possible? AFAICT read_per_ring_refs will always be called
with blkif->nr_ring_pages != 0?

If so, I would consider turning this into a BUG_ON/WARN_ON.

> +
> +	for (i = 0; i < nr_grefs; i++) {
> +		char ring_ref_name[RINGREF_NAME_LEN];
> +
> +		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> +		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> +				   "%u", &ring_ref[i]);
> +
>  		if (err != 1) {
> -			err = -EINVAL;
> -			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> -			return err;
> -		}
> -		nr_grefs = 1;
> -	} else {
> -		unsigned int i;
> -
> -		if (ring_page_order > xen_blkif_max_ring_order) {
> -			err = -EINVAL;
> -			xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
> -					 dir, ring_page_order,
> -					 xen_blkif_max_ring_order);
> -			return err;
> +			if (nr_grefs == 1)
> +				break;
> +

You need to either set err to EINVAL before calling xenbus_dev_fatal,
or call xenbus_dev_fatal with EINVAL as the second parameter.

> +			xenbus_dev_fatal(dev, err, "reading %s/%s",
> +					 dir, ring_ref_name);
> +			return -EINVAL;
>  		}
> +	}
>  
> -		nr_grefs = 1 << ring_page_order;
> -		for (i = 0; i < nr_grefs; i++) {
> -			char ring_ref_name[RINGREF_NAME_LEN];
> -
> -			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> -			err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> -					   "%u", &ring_ref[i]);
> -			if (err != 1) {
> -				err = -EINVAL;
> -				xenbus_dev_fatal(dev, err, "reading %s/%s",
> -						 dir, ring_ref_name);
> -				return err;
> -			}
> +	if (err != 1) {
> +		WARN_ON(nr_grefs != 1);
> +
> +		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> +				   &ring_ref[0]);
> +		if (err != 1) {
> +			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);

Second parameter should be EINVAL, or err should be set to EINVAL
before calling xenbus_dev_fatal.

Thanks, Roger.

Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Dongli Zhang]

On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>> therefore should be read from xenstore only once. However, it is obtained
>> in read_per_ring_refs() which might be called multiple times during the
>> initialization of each blkback queue.
>>
>> If the blkfront is malicious and the 'ring-page-order' is set in different
>> value by blkfront every time before blkback reads it, this may end up at
>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>> xen_blkif_disconnect() when frontend is destroyed.
>>
>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>> once.
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>> Changed since v1:
>>   * change the order of xenstore read in read_per_ring_refs
>>   * use xenbus_read_unsigned() in connect_ring()
>>
>> Changed since v2:
>>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>>   * avoid setting err as -EINVAL to remove extra one line of code
>>
>> Changed since v3:
>>   * exit at the beginning if !nr_grefs
>>   * change the if statements to avoid test (err != 1) twice
>>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>
>>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>>  1 file changed, 43 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index a4aadac..a2acbc9 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>  	int err, i, j;
>>  	struct xen_blkif *blkif = ring->blkif;
>>  	struct xenbus_device *dev = blkif->be->dev;
>> -	unsigned int ring_page_order, nr_grefs, evtchn;
>> +	unsigned int nr_grefs, evtchn;
>>  
>>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>>  			  &evtchn);
>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>  		return err;
>>  	}
>>  
>> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>> -			  &ring_page_order);
>> -	if (err != 1) {
>> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>> +	nr_grefs = blkif->nr_ring_pages;
>> +
>> +	if (unlikely(!nr_grefs))
>> +		return -EINVAL;
> 
> Is this even possible? AFAICT read_per_ring_refs will always be called
> with blkif->nr_ring_pages != 0?
> 
> If so, I would consider turning this into a BUG_ON/WARN_ON.

It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.

I would turn it into WARN_ON if it is fine with both Paul and you.

I prefer WARN_ON because it would remind the developers in the future that
read_per_ring_refs() should be used only when blkif->nr_ring_pages != 0.

> 
>> +
>> +	for (i = 0; i < nr_grefs; i++) {
>> +		char ring_ref_name[RINGREF_NAME_LEN];
>> +
>> +		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>> +		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>> +				   "%u", &ring_ref[i]);
>> +
>>  		if (err != 1) {
>> -			err = -EINVAL;
>> -			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>> -			return err;
>> -		}
>> -		nr_grefs = 1;
>> -	} else {
>> -		unsigned int i;
>> -
>> -		if (ring_page_order > xen_blkif_max_ring_order) {
>> -			err = -EINVAL;
>> -			xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
>> -					 dir, ring_page_order,
>> -					 xen_blkif_max_ring_order);
>> -			return err;
>> +			if (nr_grefs == 1)
>> +				break;
>> +
> 
> You need to either set err to EINVAL before calling xenbus_dev_fatal,
> or call xenbus_dev_fatal with EINVAL as the second parameter.
> 
>> +			xenbus_dev_fatal(dev, err, "reading %s/%s",
>> +					 dir, ring_ref_name);
>> +			return -EINVAL;
>>  		}
>> +	}
>>  
>> -		nr_grefs = 1 << ring_page_order;
>> -		for (i = 0; i < nr_grefs; i++) {
>> -			char ring_ref_name[RINGREF_NAME_LEN];
>> -
>> -			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>> -			err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>> -					   "%u", &ring_ref[i]);
>> -			if (err != 1) {
>> -				err = -EINVAL;
>> -				xenbus_dev_fatal(dev, err, "reading %s/%s",
>> -						 dir, ring_ref_name);
>> -				return err;
>> -			}
>> +	if (err != 1) {
>> +		WARN_ON(nr_grefs != 1);
>> +
>> +		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
>> +				   &ring_ref[0]);
>> +		if (err != 1) {
>> +			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> 
> Second parameter should be EINVAL, or err should be set to EINVAL
> before calling xenbus_dev_fatal.
> 
> Thanks, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

Dongli Zhang

Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Dongli Zhang]

On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> 
> 
> On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>>> therefore should be read from xenstore only once. However, it is obtained
>>> in read_per_ring_refs() which might be called multiple times during the
>>> initialization of each blkback queue.
>>>
>>> If the blkfront is malicious and the 'ring-page-order' is set in different
>>> value by blkfront every time before blkback reads it, this may end up at
>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>>> xen_blkif_disconnect() when frontend is destroyed.
>>>
>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>>> once.
>>>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>> ---
>>> Changed since v1:
>>>   * change the order of xenstore read in read_per_ring_refs
>>>   * use xenbus_read_unsigned() in connect_ring()
>>>
>>> Changed since v2:
>>>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>>>   * avoid setting err as -EINVAL to remove extra one line of code
>>>
>>> Changed since v3:
>>>   * exit at the beginning if !nr_grefs
>>>   * change the if statements to avoid test (err != 1) twice
>>>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>>
>>>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>>>  1 file changed, 43 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>> index a4aadac..a2acbc9 100644
>>> --- a/drivers/block/xen-blkback/xenbus.c
>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>  	int err, i, j;
>>>  	struct xen_blkif *blkif = ring->blkif;
>>>  	struct xenbus_device *dev = blkif->be->dev;
>>> -	unsigned int ring_page_order, nr_grefs, evtchn;
>>> +	unsigned int nr_grefs, evtchn;
>>>  
>>>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>>>  			  &evtchn);
>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>  		return err;
>>>  	}
>>>  
>>> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>>> -			  &ring_page_order);
>>> -	if (err != 1) {
>>> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>>> +	nr_grefs = blkif->nr_ring_pages;
>>> +
>>> +	if (unlikely(!nr_grefs))
>>> +		return -EINVAL;
>>
>> Is this even possible? AFAICT read_per_ring_refs will always be called
>> with blkif->nr_ring_pages != 0?
>>
>> If so, I would consider turning this into a BUG_ON/WARN_ON.
> 
> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
> 
> I would turn it into WARN_ON if it is fine with both Paul and you.

To clarify, I would use WARN_ON() before exit with -EINVAL (when
blkif->nr_ring_pages is 0).

Dongli Zhang

> 
> I prefer WARN_ON because it would remind the developers in the future that
> read_per_ring_refs() should be used only when blkif->nr_ring_pages != 0.
> 
>>
>>> +
>>> +	for (i = 0; i < nr_grefs; i++) {
>>> +		char ring_ref_name[RINGREF_NAME_LEN];
>>> +
>>> +		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>>> +		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>>> +				   "%u", &ring_ref[i]);
>>> +
>>>  		if (err != 1) {
>>> -			err = -EINVAL;
>>> -			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>>> -			return err;
>>> -		}
>>> -		nr_grefs = 1;
>>> -	} else {
>>> -		unsigned int i;
>>> -
>>> -		if (ring_page_order > xen_blkif_max_ring_order) {
>>> -			err = -EINVAL;
>>> -			xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
>>> -					 dir, ring_page_order,
>>> -					 xen_blkif_max_ring_order);
>>> -			return err;
>>> +			if (nr_grefs == 1)
>>> +				break;
>>> +
>>
>> You need to either set err to EINVAL before calling xenbus_dev_fatal,
>> or call xenbus_dev_fatal with EINVAL as the second parameter.
>>
>>> +			xenbus_dev_fatal(dev, err, "reading %s/%s",
>>> +					 dir, ring_ref_name);
>>> +			return -EINVAL;
>>>  		}
>>> +	}
>>>  
>>> -		nr_grefs = 1 << ring_page_order;
>>> -		for (i = 0; i < nr_grefs; i++) {
>>> -			char ring_ref_name[RINGREF_NAME_LEN];
>>> -
>>> -			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>>> -			err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>>> -					   "%u", &ring_ref[i]);
>>> -			if (err != 1) {
>>> -				err = -EINVAL;
>>> -				xenbus_dev_fatal(dev, err, "reading %s/%s",
>>> -						 dir, ring_ref_name);
>>> -				return err;
>>> -			}
>>> +	if (err != 1) {
>>> +		WARN_ON(nr_grefs != 1);
>>> +
>>> +		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
>>> +				   &ring_ref[0]);
>>> +		if (err != 1) {
>>> +			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>>
>> Second parameter should be EINVAL, or err should be set to EINVAL
>> before calling xenbus_dev_fatal.
>>
>> Thanks, Roger.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xenproject.org
>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>>
> 
> Dongli Zhang
> 

Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Roger Pau Monné]

On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
> 
> 
> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> > 
> > 
> > On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> >> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> >>> The xenstore 'ring-page-order' is used globally for each blkback queue and
> >>> therefore should be read from xenstore only once. However, it is obtained
> >>> in read_per_ring_refs() which might be called multiple times during the
> >>> initialization of each blkback queue.
> >>>
> >>> If the blkfront is malicious and the 'ring-page-order' is set in different
> >>> value by blkfront every time before blkback reads it, this may end up at
> >>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> >>> xen_blkif_disconnect() when frontend is destroyed.
> >>>
> >>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> >>> once.
> >>>
> >>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> >>> ---
> >>> Changed since v1:
> >>>   * change the order of xenstore read in read_per_ring_refs
> >>>   * use xenbus_read_unsigned() in connect_ring()
> >>>
> >>> Changed since v2:
> >>>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> >>>   * avoid setting err as -EINVAL to remove extra one line of code
> >>>
> >>> Changed since v3:
> >>>   * exit at the beginning if !nr_grefs
> >>>   * change the if statements to avoid test (err != 1) twice
> >>>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> >>>
> >>>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
> >>>  1 file changed, 43 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >>> index a4aadac..a2acbc9 100644
> >>> --- a/drivers/block/xen-blkback/xenbus.c
> >>> +++ b/drivers/block/xen-blkback/xenbus.c
> >>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>  	int err, i, j;
> >>>  	struct xen_blkif *blkif = ring->blkif;
> >>>  	struct xenbus_device *dev = blkif->be->dev;
> >>> -	unsigned int ring_page_order, nr_grefs, evtchn;
> >>> +	unsigned int nr_grefs, evtchn;
> >>>  
> >>>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> >>>  			  &evtchn);
> >>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>  		return err;
> >>>  	}
> >>>  
> >>> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> >>> -			  &ring_page_order);
> >>> -	if (err != 1) {
> >>> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> >>> +	nr_grefs = blkif->nr_ring_pages;
> >>> +
> >>> +	if (unlikely(!nr_grefs))
> >>> +		return -EINVAL;
> >>
> >> Is this even possible? AFAICT read_per_ring_refs will always be called
> >> with blkif->nr_ring_pages != 0?
> >>
> >> If so, I would consider turning this into a BUG_ON/WARN_ON.
> > 
> > It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
> > 
> > I would turn it into WARN_ON if it is fine with both Paul and you.
> 
> To clarify, I would use WARN_ON() before exit with -EINVAL (when
> blkif->nr_ring_pages is 0).

Given that this function will never be called with nr_ring_pages == 0
I would be fine with just using a BUG_ON, getting here with
nr_ring_pages == 0 would imply memory corruption or some other severe
issue has happened, and there's no possible recovery.

If you want to instead keep the return, please use plain WARN instead
of WARN_ON.

Thanks, Roger.

Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Dongli Zhang]

Hi Roger,

On 01/07/2019 11:27 PM, Roger Pau Monné wrote:
> On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
>>
>>
>> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
>>>
>>>
>>> On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
>>>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>>>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>>>>> therefore should be read from xenstore only once. However, it is obtained
>>>>> in read_per_ring_refs() which might be called multiple times during the
>>>>> initialization of each blkback queue.
>>>>>
>>>>> If the blkfront is malicious and the 'ring-page-order' is set in different
>>>>> value by blkfront every time before blkback reads it, this may end up at
>>>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>>>>> xen_blkif_disconnect() when frontend is destroyed.
>>>>>
>>>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>>>>> once.
>>>>>
>>>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>>>> ---
>>>>> Changed since v1:
>>>>>   * change the order of xenstore read in read_per_ring_refs
>>>>>   * use xenbus_read_unsigned() in connect_ring()
>>>>>
>>>>> Changed since v2:
>>>>>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>>>>>   * avoid setting err as -EINVAL to remove extra one line of code
>>>>>
>>>>> Changed since v3:
>>>>>   * exit at the beginning if !nr_grefs
>>>>>   * change the if statements to avoid test (err != 1) twice
>>>>>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>>>>
>>>>>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>>>>>  1 file changed, 43 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>>>> index a4aadac..a2acbc9 100644
>>>>> --- a/drivers/block/xen-blkback/xenbus.c
>>>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>>>  	int err, i, j;
>>>>>  	struct xen_blkif *blkif = ring->blkif;
>>>>>  	struct xenbus_device *dev = blkif->be->dev;
>>>>> -	unsigned int ring_page_order, nr_grefs, evtchn;
>>>>> +	unsigned int nr_grefs, evtchn;
>>>>>  
>>>>>  	err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>>>>>  			  &evtchn);
>>>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>>>  		return err;
>>>>>  	}
>>>>>  
>>>>> -	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>>>>> -			  &ring_page_order);
>>>>> -	if (err != 1) {
>>>>> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>>>>> +	nr_grefs = blkif->nr_ring_pages;
>>>>> +
>>>>> +	if (unlikely(!nr_grefs))
>>>>> +		return -EINVAL;
>>>>
>>>> Is this even possible? AFAICT read_per_ring_refs will always be called
>>>> with blkif->nr_ring_pages != 0?
>>>>
>>>> If so, I would consider turning this into a BUG_ON/WARN_ON.
>>>
>>> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
>>>
>>> I would turn it into WARN_ON if it is fine with both Paul and you.
>>
>> To clarify, I would use WARN_ON() before exit with -EINVAL (when
>> blkif->nr_ring_pages is 0).
> 
> Given that this function will never be called with nr_ring_pages == 0
> I would be fine with just using a BUG_ON, getting here with
> nr_ring_pages == 0 would imply memory corruption or some other severe
> issue has happened, and there's no possible recovery.
> 
> If you want to instead keep the return, please use plain WARN instead
> of WARN_ON.
> 
> Thanks, Roger.
> 

Is there any reason using WARN than WARN_ON? Because of the message printed by
WARN? something like below?

 941         if (unlikely(!nr_grefs)) {
 942                 WARN(!nr_grefs,
 943                      "read_per_ring_refs() should be called with
blkif->nr_ring_pages != 0");
 944                 return -EINVAL;
 945         }

Thank you very much.

Dongli Zhang

Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront

[Roger Pau Monné]

On Tue, Jan 8, 2019 at 10:53 AM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
> Hi Roger,
>
> On 01/07/2019 11:27 PM, Roger Pau Monné wrote:
> > On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
> >>
> >>
> >> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> >>>
> >>>
> >>> On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> >>>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> >>>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
> >>>>> therefore should be read from xenstore only once. However, it is obtained
> >>>>> in read_per_ring_refs() which might be called multiple times during the
> >>>>> initialization of each blkback queue.
> >>>>>
> >>>>> If the blkfront is malicious and the 'ring-page-order' is set in different
> >>>>> value by blkfront every time before blkback reads it, this may end up at
> >>>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> >>>>> xen_blkif_disconnect() when frontend is destroyed.
> >>>>>
> >>>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> >>>>> once.
> >>>>>
> >>>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> >>>>> ---
> >>>>> Changed since v1:
> >>>>>   * change the order of xenstore read in read_per_ring_refs
> >>>>>   * use xenbus_read_unsigned() in connect_ring()
> >>>>>
> >>>>> Changed since v2:
> >>>>>   * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> >>>>>   * avoid setting err as -EINVAL to remove extra one line of code
> >>>>>
> >>>>> Changed since v3:
> >>>>>   * exit at the beginning if !nr_grefs
> >>>>>   * change the if statements to avoid test (err != 1) twice
> >>>>>   * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> >>>>>
> >>>>>  drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
> >>>>>  1 file changed, 43 insertions(+), 33 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >>>>> index a4aadac..a2acbc9 100644
> >>>>> --- a/drivers/block/xen-blkback/xenbus.c
> >>>>> +++ b/drivers/block/xen-blkback/xenbus.c
> >>>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>>>   int err, i, j;
> >>>>>   struct xen_blkif *blkif = ring->blkif;
> >>>>>   struct xenbus_device *dev = blkif->be->dev;
> >>>>> - unsigned int ring_page_order, nr_grefs, evtchn;
> >>>>> + unsigned int nr_grefs, evtchn;
> >>>>>
> >>>>>   err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> >>>>>                     &evtchn);
> >>>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>>>           return err;
> >>>>>   }
> >>>>>
> >>>>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> >>>>> -                   &ring_page_order);
> >>>>> - if (err != 1) {
> >>>>> -         err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> >>>>> + nr_grefs = blkif->nr_ring_pages;
> >>>>> +
> >>>>> + if (unlikely(!nr_grefs))
> >>>>> +         return -EINVAL;
> >>>>
> >>>> Is this even possible? AFAICT read_per_ring_refs will always be called
> >>>> with blkif->nr_ring_pages != 0?
> >>>>
> >>>> If so, I would consider turning this into a BUG_ON/WARN_ON.
> >>>
> >>> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
> >>>
> >>> I would turn it into WARN_ON if it is fine with both Paul and you.
> >>
> >> To clarify, I would use WARN_ON() before exit with -EINVAL (when
> >> blkif->nr_ring_pages is 0).
> >
> > Given that this function will never be called with nr_ring_pages == 0
> > I would be fine with just using a BUG_ON, getting here with
> > nr_ring_pages == 0 would imply memory corruption or some other severe
> > issue has happened, and there's no possible recovery.
> >
> > If you want to instead keep the return, please use plain WARN instead
> > of WARN_ON.
> >
> > Thanks, Roger.
> >
>
> Is there any reason using WARN than WARN_ON? Because of the message printed by
> WARN? something like below?

Oh, so WARN also takes a condition, I was expecting WARN to not take
any parameters. Just use WARN_ON(true); then, there's no need to
re-evaluate !nr_grefs.