* [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() @ 2019-01-07 5:35 Dongli Zhang 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Dongli Zhang @ 2019-01-07 5:35 UTC (permalink / raw) To: xen-devel, linux-block, linux-kernel Cc: konrad.wilk, roger.pau, axboe, Paul.Durrant 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 5:35 [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Dongli Zhang @ 2019-01-07 5:35 ` Dongli Zhang 2019-01-07 9:18 ` Paul Durrant 2019-01-07 12:01 ` Roger Pau Monné 2019-01-07 9:11 ` [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Paul Durrant 2019-01-07 11:53 ` Roger Pau Monné 2 siblings, 2 replies; 11+ messages in thread From: Dongli Zhang @ 2019-01-07 5:35 UTC (permalink / raw) To: xen-devel, linux-block, linux-kernel Cc: konrad.wilk, roger.pau, axboe, Paul.Durrant 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang @ 2019-01-07 9:18 ` Paul Durrant 2019-01-07 12:01 ` Roger Pau Monné 1 sibling, 0 replies; 11+ messages in thread From: Paul Durrant @ 2019-01-07 9:18 UTC (permalink / raw) To: 'Dongli Zhang', xen-devel, linux-block, linux-kernel Cc: konrad.wilk, Roger Pau Monne, axboe > -----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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang 2019-01-07 9:18 ` Paul Durrant @ 2019-01-07 12:01 ` Roger Pau Monné 2019-01-07 14:05 ` [Xen-devel] " Dongli Zhang 1 sibling, 1 reply; 11+ messages in thread From: Roger Pau Monné @ 2019-01-07 12:01 UTC (permalink / raw) To: Dongli Zhang Cc: xen-devel, linux-block, linux-kernel, konrad.wilk, axboe, Paul.Durrant 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 12:01 ` Roger Pau Monné @ 2019-01-07 14:05 ` Dongli Zhang 2019-01-07 14:07 ` Dongli Zhang 0 siblings, 1 reply; 11+ messages in thread From: Dongli Zhang @ 2019-01-07 14:05 UTC (permalink / raw) To: Roger Pau Monné, Paul.Durrant Cc: axboe, konrad.wilk, linux-kernel, linux-block, xen-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 14:05 ` [Xen-devel] " Dongli Zhang @ 2019-01-07 14:07 ` Dongli Zhang 2019-01-07 15:27 ` Roger Pau Monné 0 siblings, 1 reply; 11+ messages in thread From: Dongli Zhang @ 2019-01-07 14:07 UTC (permalink / raw) To: Roger Pau Monné, Paul.Durrant Cc: axboe, konrad.wilk, linux-kernel, linux-block, xen-devel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 14:07 ` Dongli Zhang @ 2019-01-07 15:27 ` Roger Pau Monné 2019-01-08 9:52 ` Dongli Zhang 0 siblings, 1 reply; 11+ messages in thread From: Roger Pau Monné @ 2019-01-07 15:27 UTC (permalink / raw) To: Dongli Zhang Cc: Paul.Durrant, axboe, konrad.wilk, linux-kernel, linux-block, xen-devel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-07 15:27 ` Roger Pau Monné @ 2019-01-08 9:52 ` Dongli Zhang 2019-01-11 14:57 ` Roger Pau Monné 0 siblings, 1 reply; 11+ messages in thread From: Dongli Zhang @ 2019-01-08 9:52 UTC (permalink / raw) To: Roger Pau Monné Cc: Paul.Durrant, axboe, konrad.wilk, linux-kernel, linux-block, xen-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront 2019-01-08 9:52 ` Dongli Zhang @ 2019-01-11 14:57 ` Roger Pau Monné 0 siblings, 0 replies; 11+ messages in thread From: Roger Pau Monné @ 2019-01-11 14:57 UTC (permalink / raw) To: Dongli Zhang Cc: Roger Pau Monné, axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, Paul Durrant, xen-devel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() 2019-01-07 5:35 [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Dongli Zhang 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang @ 2019-01-07 9:11 ` Paul Durrant 2019-01-07 11:53 ` Roger Pau Monné 2 siblings, 0 replies; 11+ messages in thread From: Paul Durrant @ 2019-01-07 9:11 UTC (permalink / raw) To: 'Dongli Zhang', xen-devel, linux-block, linux-kernel Cc: konrad.wilk, Roger Pau Monne, axboe > -----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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() 2019-01-07 5:35 [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Dongli Zhang 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang 2019-01-07 9:11 ` [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Paul Durrant @ 2019-01-07 11:53 ` Roger Pau Monné 2 siblings, 0 replies; 11+ messages in thread From: Roger Pau Monné @ 2019-01-07 11:53 UTC (permalink / raw) To: Dongli Zhang Cc: xen-devel, linux-block, linux-kernel, konrad.wilk, axboe, Paul.Durrant 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-01-11 14:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-07 5:35 [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Dongli Zhang 2019-01-07 5:35 ` [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront Dongli Zhang 2019-01-07 9:18 ` Paul Durrant 2019-01-07 12:01 ` Roger Pau Monné 2019-01-07 14:05 ` [Xen-devel] " Dongli Zhang 2019-01-07 14:07 ` Dongli Zhang 2019-01-07 15:27 ` Roger Pau Monné 2019-01-08 9:52 ` Dongli Zhang 2019-01-11 14:57 ` Roger Pau Monné 2019-01-07 9:11 ` [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in connect_ring() Paul Durrant 2019-01-07 11:53 ` Roger Pau Monné
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).