Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [Xen-devel] [PATCH v4 2/2] xen/blkback: rework connect_ring() to
 avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront
To:     =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <roger.pau@citrix.com>
References: <1546839359-5478-1-git-send-email-dongli.zhang@oracle.com>
 <1546839359-5478-2-git-send-email-dongli.zhang@oracle.com>
 <20190107120107.euf7mrq7gk6bmibz@mac>
 <35c7d495-2d6c-5fec-abf8-c5aef55cf866@oracle.com>
 <56819579-def2-b045-f414-4de45188fe2e@oracle.com>
 <20190107152708.z4mecdm2apfxz2rk@mac>
Cc:     Paul.Durrant@citrix.com, axboe@kernel.dk, konrad.wilk@oracle.com,
        linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        xen-devel@lists.xenproject.org
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <d347524f-20e1-6ce4-70f5-95b699374bb2@oracle.com>
Date:   Tue, 8 Jan 2019 17:52:20 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <20190107152708.z4mecdm2apfxz2rk@mac>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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