| Message ID | 20221020094649.28667-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | (v)pci: fixes related to memory decoding handling | expand |
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
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;
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(-)