PCI_SRIOV_* are offsets into the SR-IOV capability, not into the PCI
config space. pcie_sriov_pf_exit() erroneously takes them as the latter,
which makes it read PCI_HEADER_TYPE and PCI_BIST when it tries to read
PCI_SRIOV_TOTAL_VF.

In many cases we're lucky enough that the PCI config space will be 0
there, so we just skip the whole for loop, but this isn't guaranteed.
For example, setting the multifunction bit on the PF and then doing a
'device_del' on it will get a larger number and cause a segfault.

Fix this and access the real PCI_SRIOV_* fields in the capability.

Cc: qemu-stable@nongnu.org
Fixes: 19e55471d4e8 ('pcie_sriov: Allow user to create SR-IOV device')
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/pci/pcie_sriov.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index c4f88f09757..d467284cbda 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -195,14 +195,17 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
 
 void pcie_sriov_pf_exit(PCIDevice *dev)
 {
+    uint8_t *cfg;
+
     if (dev->exp.sriov_cap == 0) {
         return;
     }
+    cfg = dev->config + dev->exp.sriov_cap;
 
     if (dev->exp.sriov_pf.vf_user_created) {
         uint16_t ven_id = pci_get_word(dev->config + PCI_VENDOR_ID);
-        uint16_t total_vfs = pci_get_word(dev->config + PCI_SRIOV_TOTAL_VF);
-        uint16_t vf_dev_id = pci_get_word(dev->config + PCI_SRIOV_VF_DID);
+        uint16_t total_vfs = pci_get_word(cfg + PCI_SRIOV_TOTAL_VF);
+        uint16_t vf_dev_id = pci_get_word(cfg + PCI_SRIOV_VF_DID);
 
         unregister_vfs(dev);
 
@@ -213,8 +216,6 @@ void pcie_sriov_pf_exit(PCIDevice *dev)
             pci_config_set_device_id(dev->exp.sriov_pf.vf[i]->config, vf_dev_id);
         }
     } else {
-        uint8_t *cfg = dev->config + dev->exp.sriov_cap;
-
         unparent_vfs(dev, pci_get_word(cfg + PCI_SRIOV_TOTAL_VF));
     }
 }
-- 
2.52.0

On Fri, Dec 05, 2025 at 03:57:18PM +0100, Kevin Wolf wrote:
> PCI_SRIOV_* are offsets into the SR-IOV capability, not into the PCI
> config space. pcie_sriov_pf_exit() erroneously takes them as the latter,
> which makes it read PCI_HEADER_TYPE and PCI_BIST when it tries to read
> PCI_SRIOV_TOTAL_VF.
> 
> In many cases we're lucky enough that the PCI config space will be 0
> there, so we just skip the whole for loop, but this isn't guaranteed.
> For example, setting the multifunction bit on the PF and then doing a
> 'device_del' on it will get a larger number and cause a segfault.
> 
> Fix this and access the real PCI_SRIOV_* fields in the capability.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 19e55471d4e8 ('pcie_sriov: Allow user to create SR-IOV device')
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Thanks for the patch! something small to improve:

> ---
>  hw/pci/pcie_sriov.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index c4f88f09757..d467284cbda 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -195,14 +195,17 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
>  
>  void pcie_sriov_pf_exit(PCIDevice *dev)
>  {
> +    uint8_t *cfg;
> +
>      if (dev->exp.sriov_cap == 0) {
>          return;
>      }
> +    cfg = dev->config + dev->exp.sriov_cap;

initialize cfg at the point of declaration maybe? I think it would
be clearer.

>  
>      if (dev->exp.sriov_pf.vf_user_created) {
>          uint16_t ven_id = pci_get_word(dev->config + PCI_VENDOR_ID);
> -        uint16_t total_vfs = pci_get_word(dev->config + PCI_SRIOV_TOTAL_VF);
> -        uint16_t vf_dev_id = pci_get_word(dev->config + PCI_SRIOV_VF_DID);
> +        uint16_t total_vfs = pci_get_word(cfg + PCI_SRIOV_TOTAL_VF);
> +        uint16_t vf_dev_id = pci_get_word(cfg + PCI_SRIOV_VF_DID);
>  
>          unregister_vfs(dev);
>  
> @@ -213,8 +216,6 @@ void pcie_sriov_pf_exit(PCIDevice *dev)
>              pci_config_set_device_id(dev->exp.sriov_pf.vf[i]->config, vf_dev_id);
>          }
>      } else {
> -        uint8_t *cfg = dev->config + dev->exp.sriov_cap;
> -
>          unparent_vfs(dev, pci_get_word(cfg + PCI_SRIOV_TOTAL_VF));
>      }
>  }
> -- 
> 2.52.0

Am 05.12.2025 um 16:03 hat Michael S. Tsirkin geschrieben:
> On Fri, Dec 05, 2025 at 03:57:18PM +0100, Kevin Wolf wrote:
> > PCI_SRIOV_* are offsets into the SR-IOV capability, not into the PCI
> > config space. pcie_sriov_pf_exit() erroneously takes them as the latter,
> > which makes it read PCI_HEADER_TYPE and PCI_BIST when it tries to read
> > PCI_SRIOV_TOTAL_VF.
> > 
> > In many cases we're lucky enough that the PCI config space will be 0
> > there, so we just skip the whole for loop, but this isn't guaranteed.
> > For example, setting the multifunction bit on the PF and then doing a
> > 'device_del' on it will get a larger number and cause a segfault.
> > 
> > Fix this and access the real PCI_SRIOV_* fields in the capability.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: 19e55471d4e8 ('pcie_sriov: Allow user to create SR-IOV device')
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Thanks for the patch! something small to improve:
> 
> > ---
> >  hw/pci/pcie_sriov.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > index c4f88f09757..d467284cbda 100644
> > --- a/hw/pci/pcie_sriov.c
> > +++ b/hw/pci/pcie_sriov.c
> > @@ -195,14 +195,17 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
> >  
> >  void pcie_sriov_pf_exit(PCIDevice *dev)
> >  {
> > +    uint8_t *cfg;
> > +
> >      if (dev->exp.sriov_cap == 0) {
> >          return;
> >      }
> > +    cfg = dev->config + dev->exp.sriov_cap;
> 
> initialize cfg at the point of declaration maybe? I think it would
> be clearer.

That's what I had first, then changed it to make it clearer that the
pointer is only guaranteed to be valid after the dev->exp.sriov_cap
check. But either way works for me. Let me know if I should send a v2
that puts it back on the top.

Kevin