Re: [PATCH v3 5/7] pcie_sriov: Validate NumVFs

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"Cédric Le Goater" <clg@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Sriram Yagnaraman" <sriram.yagnaraman@est.tech>,
	"Jason Wang" <jasowang@redhat.com>,
	"Keith Busch" <kbusch@kernel.org>,
	"Klaus Jensen" <its@irrelevant.dk>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH v3 5/7] pcie_sriov: Validate NumVFs
Date: Tue, 13 Feb 2024 05:59:01 -0500	[thread overview]
Message-ID: <20240213055345-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20240212-reuse-v3-5-8017b689ce7f@daynix.com>

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