| Message ID | 1546839359-5478-2-git-send-email-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] xen/blkback: add stack variable 'blkif' in connect_ring() | expand |
> -----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
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.
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
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 >
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.
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
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.
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 {
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(-)