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

[Roger Pau MonnÃ]

On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
> 
> 
> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> > 
> > 
> > On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> >> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> >>> The xenstore 'ring-page-order' is used globally for each blkback queue and
> >>> therefore should be read from xenstore only once. However, it is obtained
> >>> in read_per_ring_refs() which might be called multiple times during the
> >>> initialization of each blkback queue.
> >>>
> >>> If the blkfront is malicious and the 'ring-page-order' is set in different
> >>> value by blkfront every time before blkback reads it, this may end up at
> >>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> >>> xen_blkif_disconnect() when frontend is destroyed.
> >>>
> >>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> >>> once.
> >>>
> >>> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
> >>> ---
> >>> 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.