| Message ID | 20240212-reuse-v3-5-8017b689ce7f@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
On Mon, Feb 12, 2024 at 07:20:33PM +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.odaki@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; > + } Indeed: The results are undefined if NumVFs is set to a value greater than TotalVFs. However I note that hw/nvme/ctrl.c will still poke at NumVFs. Since it's undefined, I propose a simpler hack and just force it to PCI_SRIOV_TOTAL_VF. This way everyone can just assume it's ok. > > dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs); > > > -- > 2.43.0
On 2024/02/13 19:59, Michael S. Tsirkin wrote: > On Mon, Feb 12, 2024 at 07:20:33PM +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.odaki@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; >> + } > > Indeed: > The results are undefined if NumVFs is set to a value greater than TotalVFs. > > However I note that hw/nvme/ctrl.c will still poke at NumVFs. > > Since it's undefined, I propose a simpler hack and just force it > to PCI_SRIOV_TOTAL_VF. This way everyone can just assume it's ok. It is still not OK to poke at NumVFs as the guest may set a different number anytime though it's undefined if NumVFs is set while VFs are enabled. I think hw/nvme/ctrl.c should be changed to look at exp.sriov_pf.num_vfs, which holds the committed NumVFs value.
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; + } dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
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.odaki@daynix.com> --- hw/pci/pcie_sriov.c | 3 +++ 1 file changed, 3 insertions(+)