* [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show @ 2019-12-06 13:48 Stefan Nuernberger 2019-12-06 15:11 ` Boris Ostrovsky 0 siblings, 1 reply; 6+ messages in thread From: Stefan Nuernberger @ 2019-12-06 13:48 UTC (permalink / raw) To: linux-kernel Cc: Boris Ostrovsky, Juergen Gross, Ross Lagerwall, Uwe Dannowski, Conny Seidel, Stefan Nuernberger, xen-devel, stable 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) { field = cfg_entry->field; -- 2.23.0 Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show 2019-12-06 13:48 [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show Stefan Nuernberger @ 2019-12-06 15:11 ` Boris Ostrovsky 2019-12-06 18:09 ` Nuernberger, Stefan 0 siblings, 1 reply; 6+ messages in thread From: Boris Ostrovsky @ 2019-12-06 15:11 UTC (permalink / raw) To: Stefan Nuernberger, linux-kernel Cc: Juergen Gross, Ross Lagerwall, Uwe Dannowski, Conny Seidel, xen-devel, stable 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? -boris > field = cfg_entry->field; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show 2019-12-06 15:11 ` Boris Ostrovsky @ 2019-12-06 18:09 ` Nuernberger, Stefan 2019-12-06 20:15 ` Boris Ostrovsky 0 siblings, 1 reply; 6+ messages in thread From: Nuernberger, Stefan @ 2019-12-06 18:09 UTC (permalink / raw) To: boris.ostrovsky, Nuernberger, Stefan, linux-kernel Cc: xen-devel, Seidel, Conny, jgross, ross.lagerwall, Dannowski, Uwe, stable 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. 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? -Stefan > > -boris > > > > > field = cfg_entry->field; Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show 2019-12-06 18:09 ` Nuernberger, Stefan @ 2019-12-06 20:15 ` Boris Ostrovsky 2019-12-09 18:16 ` Nuernberger, Stefan 0 siblings, 1 reply; 6+ messages in thread From: Boris Ostrovsky @ 2019-12-06 20:15 UTC (permalink / raw) To: Nuernberger, Stefan, linux-kernel Cc: xen-devel, Seidel, Conny, jgross, ross.lagerwall, Dannowski, Uwe, stable 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show 2019-12-06 20:15 ` Boris Ostrovsky @ 2019-12-09 18:16 ` Nuernberger, Stefan 2019-12-09 21:21 ` Boris Ostrovsky 0 siblings, 1 reply; 6+ messages in thread From: Nuernberger, Stefan @ 2019-12-09 18:16 UTC (permalink / raw) To: boris.ostrovsky, linux-kernel Cc: Seidel, Conny, stable, jgross, xen-devel, ross.lagerwall, Dannowski, Uwe 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. 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. 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? - Stefan Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show 2019-12-09 18:16 ` Nuernberger, Stefan @ 2019-12-09 21:21 ` Boris Ostrovsky 0 siblings, 0 replies; 6+ messages in thread From: Boris Ostrovsky @ 2019-12-09 21:21 UTC (permalink / raw) To: Nuernberger, Stefan, linux-kernel Cc: Seidel, Conny, stable, jgross, xen-devel, ross.lagerwall, Dannowski, Uwe 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-09 21:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 13:48 [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show Stefan Nuernberger 2019-12-06 15:11 ` Boris Ostrovsky 2019-12-06 18:09 ` Nuernberger, Stefan 2019-12-06 20:15 ` Boris Ostrovsky 2019-12-09 18:16 ` Nuernberger, Stefan 2019-12-09 21:21 ` Boris Ostrovsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).