Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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.