Messages in this thread | | | Subject | Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show | From | Boris Ostrovsky <> | Date | Fri, 6 Dec 2019 15:15:44 -0500 |
| |
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> >>> >>> Reading /sys/bus/pci/drivers/pciback/quirks while unbinding can >>> result >>> in dereferencing a NULL pointer. Instead, skip printing information >>> about the dangling quirk. >>> >>> Reported-by: Conny Seidel <consei@amazon.de> >>> Signed-off-by: Uwe Dannowski <uwed@amazon.de> >>> Signed-off-by: Stefan Nuernberger <snu@amazon.com> >>> >>> Cc: xen-devel@lists.xenproject.org >>> Cc: stable@vger.kernel.org >>> --- >>> drivers/xen/xen-pciback/pci_stub.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen- >>> pciback/pci_stub.c >>> index 097410a7cdb7..da725e474294 100644 >>> --- a/drivers/xen/xen-pciback/pci_stub.c >>> +++ b/drivers/xen/xen-pciback/pci_stub.c >>> @@ -1346,6 +1346,8 @@ static ssize_t quirks_show(struct >>> device_driver *drv, char *buf) >>> quirk->devid.subdevice); >>> >>> dev_data = pci_get_drvdata(quirk->pdev); >>> + if (!dev_data) >>> + continue; >>> >>> 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).
> 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).
-boris
| |