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

[Dongli Zhang <dongli.zhang@xxxxxxxxxx>]

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.

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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

Dongli Zhang

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