[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs |
Date: |
Wed, 14 Feb 2024 10:54:23 -0500 |
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.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;
> > > + }
> >
> >
> > 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
- [PATCH v4 0/9] hw/pci: SR-IOV related fixes and improvements, Akihiko Odaki, 2024/02/14
- [PATCH v4 1/9] hw/pci: Use -1 as a default value for rombar, Akihiko Odaki, 2024/02/14
- [PATCH v4 2/9] hw/pci: Determine if rombar is explicitly enabled, Akihiko Odaki, 2024/02/14
- [PATCH v4 3/9] vfio: Avoid inspecting option QDict for rombar, Akihiko Odaki, 2024/02/14
- [PATCH v4 4/9] hw/qdev: Remove opts member, Akihiko Odaki, 2024/02/14
- [PATCH v4 5/9] pcie_sriov: Validate NumVFs, Akihiko Odaki, 2024/02/14
- Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs, Michael Tokarev, 2024/02/14
[PATCH v4 6/9] pcie_sriov: Reuse SR-IOV VF device instances, Akihiko Odaki, 2024/02/14
[PATCH v4 7/9] pcie_sriov: Release VFs failed to realize, Akihiko Odaki, 2024/02/14