On Wed, Feb 14, 2024 at 11:49:52PM +0900, Akihiko Odaki wrote: > On 2024/02/14 15:52, Michael S. Tsirkin wrote: > > On Wed, Feb 14, 2024 at 02:13:43PM +0900, Akihiko Odaki wrote: > > > The guest may write NumVFs greater than TotalVFs and that can lead > > > to buffer overflow in VF implementations. > > > > > > Fixes: 7c0fa8dff811 ("pcie: Add support for Single Root I/O > > > Virtualization (SR/IOV)") > > > Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> > > > --- > > > hw/pci/pcie_sriov.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c > > > index a1fe65f5d801..da209b7f47fd 100644 > > > --- a/hw/pci/pcie_sriov.c > > > +++ b/hw/pci/pcie_sriov.c > > > @@ -176,6 +176,9 @@ static void register_vfs(PCIDevice *dev) > > > assert(sriov_cap > 0); > > > num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF); > > > + if (num_vfs > pci_get_word(dev->config + sriov_cap + > > > PCI_SRIOV_TOTAL_VF)) { > > > + return; > > > + } > > > > > > yes but with your change PCI_SRIOV_NUM_VF no longer reflects the > > number of registered VFs and that might lead to uninitialized > > data read which is not better :(. > > > > How about just forcing the PCI_SRIOV_NUM_VF register to be > > below PCI_SRIOV_TOTAL_VF at all times? > > PCI_SRIOV_NUM_VF is already divergent from the number of registered VFs. It > may have a number greater than the current registered VFs before setting VF > Enable. > > The guest may also change PCI_SRIOV_NUM_VF while VF Enable is set; the > behavior is undefined in such a case but we still accept such a write. A > value written in such a case won't be reflected to the actual number of > enabled VFs.
OK then let's add a comment near num_vfs explaining all this and saying only to use this value. I also would prefer to have this if just where we set num_vfs. And maybe after all do set num_vfs to PCI_SRIOV_TOTAL_VF? Less of a chance of breaking something I feel... -- MST