1. Patchew
  2. QEMU
  3. [PATCH v4 0/9] hw/pci: SR-IOV related fixes and improvements
  4. View patch

[PATCH v4 5/9] pcie_sriov: Validate NumVFs

Akihiko Odaki posted 9 patches 9 months, 2 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Akihiko Odaki <akihiko.odaki@daynix.com>, Sriram Yagnaraman <sriram.yagnaraman@est.tech>, Jason Wang <jasowang@redhat.com>, Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
There is a newer version of this series
[PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Akihiko Odaki 9 months, 2 weeks ago 
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;
+    }
 
     dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
 

-- 
2.43.0
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Michael Tokarev 9 months, 2 weeks ago 
14.02.2024 08:13, Akihiko Odaki wrote:
> The guest may write NumVFs greater than TotalVFs and that can lead
> to buffer overflow in VF implementations.

This seems to be stable-worthy (Cc'd), and maybe even CVE-worthy?

Thanks,

/mjt

> 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;
> +    }
>   
>       dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>   
>
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Akihiko Odaki 9 months, 2 weeks ago 
On 2024/02/14 17:58, Michael Tokarev wrote:
> 14.02.2024 08:13, Akihiko Odaki wrote:
>> The guest may write NumVFs greater than TotalVFs and that can lead
>> to buffer overflow in VF implementations.
> 
> This seems to be stable-worthy (Cc'd), and maybe even CVE-worthy?

Perhaps so. The scope of the bug is limited to emulated SR-IOV devices, 
and I think nobody use them except for development, but it may be still 
nice to have a CVE.

Can anyone help assign a CVE? I don't know the procedure.

Regards,
Akihiko Odaki

> 
> Thanks,
> 
> /mjt
> 
>> 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;
>> +    }
>>       dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>>
>
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Michael Tokarev 9 months, 2 weeks ago 
14.02.2024 17:54, Akihiko Odaki wrote:
> On 2024/02/14 17:58, Michael Tokarev wrote:
>> 14.02.2024 08:13, Akihiko Odaki wrote:
>>> The guest may write NumVFs greater than TotalVFs and that can lead
>>> to buffer overflow in VF implementations.
>>
>> This seems to be stable-worthy (Cc'd), and maybe even CVE-worthy?
> 
> Perhaps so. The scope of the bug is limited to emulated SR-IOV devices, and I think nobody use them except for development, but it may be still nice 
> to have a CVE.
> 
> Can anyone help assign a CVE? I don't know the procedure.

Heh. Usually I ask exactly the opposite question: how to avoid assigning
a CVE# for a non-issue which they most likely think is a serious security
bug?  We've plenty of these in qemu, collecting dust for years...  For
example, for things like some actions by privileged guest process (or kernel)
which leads to qemu dying with assertion failure, which, on a real HW, will
cause hardware lockup.

Nope, I don't remember how to request a CVE ;)

/mjt
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Michael S. Tsirkin 9 months, 2 weeks ago 
On Wed, Feb 14, 2024 at 06:53:43PM +0300, Michael Tokarev wrote:
> Nope, I don't remember how to request a CVE ;)

https://www.qemu.org/contribute/security-process/
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Akihiko Odaki 9 months, 1 week ago 
On 2024/02/15 0:55, Michael S. Tsirkin wrote:
> On Wed, Feb 14, 2024 at 06:53:43PM +0300, Michael Tokarev wrote:
>> Nope, I don't remember how to request a CVE ;)
> 
> https://www.qemu.org/contribute/security-process/

Thanks, I requested CVEs with the form.

QEMU doesn't have any list of features without security guarantee so I 
think it's "kind" to have CVEs for any features though I would certainly 
add igb and NVMe's SR-IOV feature to such a list if there is.

It should be harmless to have extra CVEs at least. Well, some may think 
all CVEs serious but that's not my fault.
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Michael S. Tsirkin 9 months, 2 weeks ago 
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?

>      dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>  
> 
> -- 
> 2.43.0
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Akihiko Odaki 9 months, 2 weeks ago 
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.
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Michael S. Tsirkin 9 months, 2 weeks ago 
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
Re: [PATCH v4 5/9] pcie_sriov: Validate NumVFs
Posted by Akihiko Odaki 9 months, 2 weeks ago 
On 2024/02/15 0:54, Michael S. Tsirkin wrote:
> 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...

I don't think we need a comment for this. In general, it should be the 
last thing to do to read the PCI configuration space, which is writable 
by the guest, without proper validation. And such a validation should 
happen in the internal of the capability implementation.

I think the core of the problem in nvme is that it decided to take a 
look into SR-IOV configurations. It should have never looked at 
PCI_SRIOV_CTRL or PCI_SRIOV_NUM_VF.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.