It's possible for a device to be assigned to a domain but have no
vpci structure if vpci_process_pending() failed and called
vpci_remove_device() as a result.  The unconditional accesses done by
vpci_{read,write}() and vpci_remove_device() to pdev->vpci would
then trigger a NULL pointer dereference.

Add checks for pdev->vpci presence in the affected functions.

Fixes: 9c244fdef7 ('vpci: add header handlers')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/vpci/vpci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 3467c0de86..647f7af679 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -37,7 +37,7 @@ extern vpci_register_init_t *const __end_vpci_array[];
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
-    if ( !has_vpci(pdev->domain) )
+    if ( !has_vpci(pdev->domain) || !pdev->vpci )
         return;
 
     spin_lock(&pdev->vpci->lock);
@@ -326,7 +326,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
 
     /* Find the PCI dev matching the address. */
     pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev )
+    if ( !pdev || !pdev->vpci )
         return vpci_read_hw(sbdf, reg, size);
 
     spin_lock(&pdev->vpci->lock);
@@ -436,7 +436,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
      * Passthrough everything that's not trapped.
      */
     pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev )
+    if ( !pdev || !pdev->vpci )
     {
         vpci_write_hw(sbdf, reg, size, data);
         return;
-- 
2.37.3

On 20.10.2022 11:46, Roger Pau Monne wrote:
> It's possible for a device to be assigned to a domain but have no
> vpci structure if vpci_process_pending() failed and called
> vpci_remove_device() as a result.  The unconditional accesses done by
> vpci_{read,write}() and vpci_remove_device() to pdev->vpci would
> then trigger a NULL pointer dereference.
> 
> Add checks for pdev->vpci presence in the affected functions.
> 
> Fixes: 9c244fdef7 ('vpci: add header handlers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I wonder though whether these changes are enough. Is
vpci_process_pending() immune to a pdev losing its ->vpci?

Furthermore msix_find() iterates over d->arch.hvm.msix_tables, which
looks to only ever be added to. Doesn't this list need pruning by
vpci_remove_device()? I've noticed this only because of looking at
derefs of ->vpci in msix.c - I don't think I can easily see that all
of those derefs are once again immune to a pdev losing its ->vpci.

Jan

On Mon, Oct 24, 2022 at 01:04:01PM +0200, Jan Beulich wrote:
> On 20.10.2022 11:46, Roger Pau Monne wrote:
> > It's possible for a device to be assigned to a domain but have no
> > vpci structure if vpci_process_pending() failed and called
> > vpci_remove_device() as a result.  The unconditional accesses done by
> > vpci_{read,write}() and vpci_remove_device() to pdev->vpci would
> > then trigger a NULL pointer dereference.
> > 
> > Add checks for pdev->vpci presence in the affected functions.
> > 
> > Fixes: 9c244fdef7 ('vpci: add header handlers')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I wonder though whether these changes are enough. Is
> vpci_process_pending() immune to a pdev losing its ->vpci?

I think this is safe so far because the only place where
vpci_remove_device() gets called that doesn't also deassign the device
from the domain is vpci_process_pending(), and in that error path it
also clears any pending work.  Since the device no longer has ->vpci
handlers  no further calls to vpci_process_pending() can happen.

> Furthermore msix_find() iterates over d->arch.hvm.msix_tables, which
> looks to only ever be added to. Doesn't this list need pruning by
> vpci_remove_device()? I've noticed this only because of looking at
> derefs of ->vpci in msix.c - I don't think I can easily see that all
> of those derefs are once again immune to a pdev losing its ->vpci.

I think you are correct, we are missing a
list_del(&pdev->vpci->msix->next) in vpci_remove_device().  We will
however have locking issues with this, as msix_find() doesn't take any
locks, neither do it's callers.  I guess this will be fixed as part of
the lager add vPCI locking series.  Will add another patch to the
series with the MSIX table removal.

Thanks, Roger.

On 24.10.2022 18:01, Roger Pau Monné wrote:
> On Mon, Oct 24, 2022 at 01:04:01PM +0200, Jan Beulich wrote:
>> Furthermore msix_find() iterates over d->arch.hvm.msix_tables, which
>> looks to only ever be added to. Doesn't this list need pruning by
>> vpci_remove_device()? I've noticed this only because of looking at
>> derefs of ->vpci in msix.c - I don't think I can easily see that all
>> of those derefs are once again immune to a pdev losing its ->vpci.
> 
> I think you are correct, we are missing a
> list_del(&pdev->vpci->msix->next) in vpci_remove_device().  We will
> however have locking issues with this, as msix_find() doesn't take any
> locks, neither do it's callers.  I guess this will be fixed as part of
> the lager add vPCI locking series.  Will add another patch to the
> series with the MSIX table removal.

But the locking issue the isn't new then: List insertion can also disturb
msix_find(), can't it?

Jan