* [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-06 13:48 ` Stefan Nuernberger
0 siblings, 0 replies; 12+ 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] 12+ messages in thread
* [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-06 13:48 ` Stefan Nuernberger
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Nuernberger @ 2019-12-06 13:48 UTC (permalink / raw)
To: linux-kernel
Cc: Juergen Gross, Stefan Nuernberger, Conny Seidel, Ross Lagerwall,
stable, xen-devel, Boris Ostrovsky, Uwe Dannowski
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
2019-12-06 13:48 ` [Xen-devel] " Stefan Nuernberger
@ 2019-12-06 15:11 ` Boris Ostrovsky
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-06 15:11 ` Boris Ostrovsky
0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2019-12-06 15:11 UTC (permalink / raw)
To: Stefan Nuernberger, linux-kernel
Cc: Juergen Gross, stable, Ross Lagerwall, Conny Seidel, xen-devel,
Uwe Dannowski
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
2019-12-06 15:11 ` [Xen-devel] " Boris Ostrovsky
@ 2019-12-06 18:09 ` Nuernberger, Stefan
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-06 18:09 ` Nuernberger, Stefan
0 siblings, 0 replies; 12+ messages in thread
From: Nuernberger, Stefan @ 2019-12-06 18:09 UTC (permalink / raw)
To: boris.ostrovsky, Nuernberger, Stefan, linux-kernel
Cc: jgross, stable, ross.lagerwall, Seidel, Conny, xen-devel, Dannowski, Uwe
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
2019-12-06 18:09 ` [Xen-devel] " Nuernberger, Stefan
@ 2019-12-06 20:15 ` Boris Ostrovsky
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-06 20:15 ` Boris Ostrovsky
0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2019-12-06 20:15 UTC (permalink / raw)
To: Nuernberger, Stefan, linux-kernel
Cc: jgross, stable, ross.lagerwall, Seidel, Conny, xen-devel, Dannowski, Uwe
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
2019-12-06 20:15 ` [Xen-devel] " Boris Ostrovsky
@ 2019-12-09 18:16 ` Nuernberger, Stefan
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-09 18:16 ` Nuernberger, Stefan
0 siblings, 0 replies; 12+ messages in thread
From: Nuernberger, Stefan @ 2019-12-09 18:16 UTC (permalink / raw)
To: boris.ostrovsky, linux-kernel
Cc: jgross, stable, ross.lagerwall, Seidel, Conny, xen-devel, 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
2019-12-09 18:16 ` [Xen-devel] " Nuernberger, Stefan
@ 2019-12-09 21:21 ` Boris Ostrovsky
-1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [Xen-devel] [PATCH] xen/pciback: Prevent NULL pointer dereference in quirks_show
@ 2019-12-09 21:21 ` Boris Ostrovsky
0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2019-12-09 21:21 UTC (permalink / raw)
To: Nuernberger, Stefan, linux-kernel
Cc: jgross, stable, ross.lagerwall, Seidel, Conny, xen-devel, 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-12-09 21:19 UTC | newest]
Thread overview: 12+ 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 13:48 ` [Xen-devel] " Stefan Nuernberger
2019-12-06 15:11 ` Boris Ostrovsky
2019-12-06 15:11 ` [Xen-devel] " Boris Ostrovsky
2019-12-06 18:09 ` Nuernberger, Stefan
2019-12-06 18:09 ` [Xen-devel] " Nuernberger, Stefan
2019-12-06 20:15 ` Boris Ostrovsky
2019-12-06 20:15 ` [Xen-devel] " Boris Ostrovsky
2019-12-09 18:16 ` Nuernberger, Stefan
2019-12-09 18:16 ` [Xen-devel] " Nuernberger, Stefan
2019-12-09 21:21 ` Boris Ostrovsky
2019-12-09 21:21 ` [Xen-devel] " Boris Ostrovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.