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: [PATCH] xen/pciback: Prevent NULL pointer dereference in
 quirks_show
To:     "Nuernberger, Stefan" <snu@amazon.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     "Seidel, Conny" <consei@amazon.de>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "jgross@suse.com" <jgross@suse.com>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        "ross.lagerwall@citrix.com" <ross.lagerwall@citrix.com>,
        "Dannowski, Uwe" <uwed@amazon.de>
References: <20191206134804.4537-1-snu@amazon.com>
 <9917a357-12f6-107f-e08d-33e464036317@oracle.com>
 <1575655787.7257.42.camel@amazon.de>
 <4bc83b82-427f-2215-3161-5776867675a1@oracle.com>
 <1575915416.21160.49.camel@amazon.de>
From:   Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <7e20daf7-e351-205d-183d-5861e0839c66@oracle.com>
Date:   Mon, 9 Dec 2019 16:21:33 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <1575915416.21160.49.camel@amazon.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 12/9/19 1:16 PM, Nuernberger, Stefan wrote:
> On Fri, 2019-12-06 at 15:15 -0500, Boris Ostrovsky wrote:
>> On 12/6/19 1:09 PM, Nuernberger, Stefan wrote:
>>> On Fri, 2019-12-06 at 10:11 -0500, Boris Ostrovsky wrote:
>>>> On 12/6/19 8:48 AM, Stefan Nuernberger wrote:
>>>>> From: Uwe Dannowski <uwed@amazon.de>
>>>>>   
>>>>>   		list_for_each_entry(cfg_entry, &dev_data-
>>>>>> config_fields, list) {
>>>> Couldn't you have the same race here?
>>> Not quite the same, but it might not be entirely safe yet. The
>>> 'quirks_show' takes the 'device_ids_lock' and races with unbind /
>>> 'pcistub_device_release' "which takes device_lock mutex". So this
>>> might
>>> now be a UAF read access instead of a NULL pointer dereference.
>> Yes, that's what I meant (although I don't see much difference in
>> this
>> context).
> Well, the NULL ptr access causes an instant kernel panic whereas we
> have not attributed crashes to the possible UAF read until now.
>
>>>   We have
>>> not observed adversarial effects in our testing (compared to the
>>> obvious issues with NULL pointer) but that's not a guarantee of
>>> course.
>>>
>>> So should quirks_show actually be protected by pcistub_devices_lock
>>> instead as are other functions that access dev_data? Does it need
>>> both
>>> locks in that case?
>> device_ids_lock protects device_ids list, which is not what you are
>> trying to access, so that doesn't look like right lock to hold. And
>> AFAICT pcistub_devices_lock is not held when device data is cleared
>> in
>> pcistub_device_release() (which I think is where we are racing).
> Indeed. The xen_pcibk_quirks list does not have a separate lock to
> protect it. It's either modified under 'pcistub_devices_lock', from
> pcistub_remove(), or iterated over with the 'device_ids_lock' held in
> quirks_show(). Also the quirks list is amended from
>    pcistub_init_device()
>      -> xen_pcibk_config_init_dev()
>        -> xen_pcibk_config_quirks_init()
> without holding any lock at all. In fact the
> pcistub_init_devices_late() and pcistub_seize() functions deliberately
> release the pcistub_devices_lock before calling pcistub_init_device().
> This looks broken.


Indeed.


>
> The race is between
>    pcistub_remove()
>      -> pcistub_device_put()
>        -> pcistub_device_release()
> on one side and the quirks_show() on the other side. The problematic
> quirk is freed from the xen_pcibk_quirks list in pcistub_remove() early
> on under pcistub_devices_lock before the associated dev_data is freed
> eventually. So switching from device_ids_lock to pcistub_devices_lock
> in quirks_show() could be sufficient to always have valid dev_data for
> all quirks in the list.


Yes, that should do it. (I missed xen_pcibk_config_quirk_release() call, 
which is why I wasn't sure pcistub_devices_lock is held where necessary).


>
> There is also pcistub_put_pci_dev() possibly in the race, called from
> xen_pcibk_remove_device(), or xen_pcibk_xenbus_remove(), or
> pcistub_remove(). The pcistub_remove() call site is safe when we switch
> to pcistub_devices_lock (same reasoning as above). For the others I
> currently do not see when the quirks are ever freed?


I wonder whether we should call xen_pcibk_config_quirk_release() from 
pcistub_device_release() under pcistub_devices_lock.


-boris