| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4b5b3d4fb52c421d9be3f204b5695cc2@AMSPEX02CL03.citrite.net>
Date: Fri, 7 Dec 2018 15:15:37 +0000
From: Paul Durrant <Paul.Durrant@...rix.com>
To: 'Dongli Zhang' <dongli.zhang@...cle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>
CC: "axboe@...nel.dk" <axboe@...nel.dk>,
Roger Pau Monne <roger.pau@...rix.com>,
"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>
Subject: RE: [Xen-devel] [PATCH 1/1] xen/blkback: rework connect_ring() to
avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront
> -----Original Message-----
> From: Dongli Zhang [mailto:dongli.zhang@...cle.com]
> Sent: 07 December 2018 15:10
> To: Paul Durrant <Paul.Durrant@...rix.com>; linux-kernel@...r.kernel.org;
> xen-devel@...ts.xenproject.org; linux-block@...r.kernel.org
> Cc: axboe@...nel.dk; Roger Pau Monne <roger.pau@...rix.com>;
> konrad.wilk@...cle.com
> Subject: Re: [Xen-devel] [PATCH 1/1] xen/blkback: rework connect_ring() to
> avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront
>
> Hi Paul,
>
> On 12/07/2018 05:39 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@...ts.xenproject.org] On
> Behalf
> >> Of Dongli Zhang
> >> Sent: 07 December 2018 04:18
> >> To: linux-kernel@...r.kernel.org; xen-devel@...ts.xenproject.org;
> linux-
> >> block@...r.kernel.org
> >> Cc: axboe@...nel.dk; Roger Pau Monne <roger.pau@...rix.com>;
> >> konrad.wilk@...cle.com
> >> Subject: [Xen-devel] [PATCH 1/1] 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.
> >
> > That is certainly sub-optimal.
> >
> >>
> >> 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.
> >
> > I can't actually see what useful function blkif->nr_ring_pages actually
> performs any more. Perhaps you could actually get rid of it?
>
> How about we keep it? Other than reading from xenstore, it is the only
> place for
> us to know the value from 'ring-page-order'.
>
> This helps calculate the initialized number of elements on all
> xen_blkif_ring->pending_free lists. That's how "WARN_ON(i !=
> (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" is used to double
> check if
> there is no leak of elements reclaimed from all xen_blkif_ring-
> >pending_free.
>
> It helps vmcore analysis as well. Given blkif->nr_ring_pages, we would be
> able
> to double check if the number of ring buffer slots are correct.
>
> I could not see any drawback leaving blkif->nr_ring_pagesin the code.
No, there's no drawback apart from space, but apart from that cross-check and, as you say, core analysis it seems to have little value.
Paul
>
> Dongli Zhang
>
> >
> >>
> >> This patch reworks connect_ring() to read xenstore 'ring-page-order'
> only
> >> once.
> >
> > That is certainly a good thing :-)
> >
> > Paul
> >
> >>
> >> Signed-off-by: Dongli Zhang <dongli.zhang@...cle.com>
> >> ---
> >> drivers/block/xen-blkback/xenbus.c | 49 ++++++++++++++++++++++++------
> ---
> >> -----
> >> 1 file changed, 31 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> >> blkback/xenbus.c
> >> index a4bc74e..4a8ce20 100644
> >> --- a/drivers/block/xen-blkback/xenbus.c
> >> +++ b/drivers/block/xen-blkback/xenbus.c
> >> @@ -919,14 +919,15 @@ static void connect(struct backend_info *be)
> >> /*
> >> * Each ring may have multi pages, depends on "ring-page-order".
> >> */
> >> -static int read_per_ring_refs(struct xen_blkif_ring *ring, const char
> >> *dir)
> >> +static int read_per_ring_refs(struct xen_blkif_ring *ring, const char
> >> *dir,
> >> + bool use_ring_page_order)
> >> {
> >> unsigned int ring_ref[XENBUS_MAX_RING_GRANTS];
> >> struct pending_req *req, *n;
> >> 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,28 +937,18 @@ 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) {
> >> + nr_grefs = blkif->nr_ring_pages;
> >> +
> >> + if (!use_ring_page_order) {
> >> err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> >> &ring_ref[0]);
> >> 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;
> >> - }
> >> -
> >> - nr_grefs = 1 << ring_page_order;
> >> for (i = 0; i < nr_grefs; i++) {
> >> char ring_ref_name[RINGREF_NAME_LEN];
> >>
> >> @@ -972,7 +963,6 @@ static int read_per_ring_refs(struct xen_blkif_ring
> >> *ring, const char *dir)
> >> }
> >> }
> >> }
> >> - blkif->nr_ring_pages = nr_grefs;
> >>
> >> for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
> >> req = kzalloc(sizeof(*req), GFP_KERNEL);
> >> @@ -1030,6 +1020,8 @@ 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;
> >> + bool use_ring_page_order = false;
> >> + unsigned int ring_page_order;
> >>
> >> pr_debug("%s %s\n", __func__, dev->otherend);
> >>
> >> @@ -1075,8 +1067,28 @@ static int connect_ring(struct backend_info *be)
> >> be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
> >> pers_grants ? "persistent grants" : "");
> >>
> >> + err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> >> + &ring_page_order);
> >> +
> >> + if (err != 1) {
> >> + be->blkif->nr_ring_pages = 1;
> >> + } else {
> >> + 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;
> >> + }
> >> +
> >> + use_ring_page_order = true;
> >> + be->blkif->nr_ring_pages = 1 << ring_page_order;
> >> + }
> >> +
> >> if (be->blkif->nr_rings == 1)
> >> - return read_per_ring_refs(&be->blkif->rings[0], dev-
> >>> otherend);
> >> + return read_per_ring_refs(&be->blkif->rings[0], dev->otherend,
> >> + use_ring_page_order);
> >> else {
> >> xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
> >> xspath = kmalloc(xspathsize, GFP_KERNEL);
> >> @@ -1088,7 +1100,8 @@ static int connect_ring(struct backend_info *be)
> >> for (i = 0; i < be->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(&be->blkif->rings[i], xspath,
> >> + use_ring_page_order);
> >> if (err) {
> >> kfree(xspath);
> >> return err;
> >> --
> >> 2.7.4
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@...ts.xenproject.org
> >> https://lists.xenproject.org/mailman/listinfo/xen-devel
Powered by blists - more mailing lists