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

[Dongli Zhang <dongli.zhang@oracle.com>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel