Fix use-after-free and make format overflow more difficult

[PATCH 2/5] vfio/pci: Grow buffer in vfio_pci_host_match()

Posted by Akihiko Odaki 2 weeks, 2 days ago

Ensure the buffer in vfio_pci_host_match() will not overflow even when
an invalid addr parameter is provided.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c73447272141..3338c4d7b528 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2673,7 +2673,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 
 bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-    char tmp[13];
+    char tmp[36];
 
     sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
             addr->bus, addr->slot, addr->function);

-- 
2.52.0

Re: [PATCH 2/5] vfio/pci: Grow buffer in vfio_pci_host_match()

Posted by Cédric Le Goater 2 weeks, 2 days ago

On 1/25/26 07:42, Akihiko Odaki wrote:
> Ensure the buffer in vfio_pci_host_match() will not overflow even when
> an invalid addr parameter is provided.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/vfio/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c73447272141..3338c4d7b528 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2673,7 +2673,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>   
>   bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>   {
> -    char tmp[13];
> +    char tmp[36];
>   
>       sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
>               addr->bus, addr->slot, addr->function);
> 

Using a g_autofree variable seems possible too :

     g_autofree char *tmp =
         g_strdup_printf("%04x:%02x:%02x.%1x", addr->domain, addr->bus,
                         addr->slot, addr->function);

vfio_pci_host_match() could be moved under container-legacy.c, with
an assert on addr too.

Thanks,

C.

Re: [PATCH 2/5] vfio/pci: Grow buffer in vfio_pci_host_match()

Posted by Akihiko Odaki 2 weeks, 1 day ago

On 2026/01/25 19:22, Cédric Le Goater wrote:
> On 1/25/26 07:42, Akihiko Odaki wrote:
>> Ensure the buffer in vfio_pci_host_match() will not overflow even when
>> an invalid addr parameter is provided.
>>
>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> ---
>>   hw/vfio/pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c73447272141..3338c4d7b528 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2673,7 +2673,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>>   bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>>   {
>> -    char tmp[13];
>> +    char tmp[36];
>>       sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
>>               addr->bus, addr->slot, addr->function);
>>
> 
> Using a g_autofree variable seems possible too :
> 
>      g_autofree char *tmp =
>          g_strdup_printf("%04x:%02x:%02x.%1x", addr->domain, addr->bus,
>                          addr->slot, addr->function);
> 
> vfio_pci_host_match() could be moved under container-legacy.c, with
> an assert on addr too.

I have another idea for improvement: replace slot and function with 
devfn. It will limit the range of slot and function, and is better 
aligned with Linux UAPI (c.f., vfio_legacy_pci_hot_reset()).

In any case, I intend to limit the scope of this series to fix the 
overflow. Further improvements like moving the code to 
container-legacy.c may be done later.

Regards,
Akihiko Odaki

Re: [PATCH 2/5] vfio/pci: Grow buffer in vfio_pci_host_match()

Posted by Cédric Le Goater 2 weeks, 1 day ago

On 1/26/26 03:37, Akihiko Odaki wrote:
> On 2026/01/25 19:22, Cédric Le Goater wrote:
>> On 1/25/26 07:42, Akihiko Odaki wrote:
>>> Ensure the buffer in vfio_pci_host_match() will not overflow even when
>>> an invalid addr parameter is provided.
>>>
>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> ---
>>>   hw/vfio/pci.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index c73447272141..3338c4d7b528 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2673,7 +2673,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>>>   bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>>>   {
>>> -    char tmp[13];
>>> +    char tmp[36];
>>>       sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
>>>               addr->bus, addr->slot, addr->function);
>>>
>>
>> Using a g_autofree variable seems possible too :
>>
>>      g_autofree char *tmp =
>>          g_strdup_printf("%04x:%02x:%02x.%1x", addr->domain, addr->bus,
>>                          addr->slot, addr->function);
>>
>> vfio_pci_host_match() could be moved under container-legacy.c, with
>> an assert on addr too.
> 
> I have another idea for improvement: replace slot and function with devfn. It will limit the range of slot and function, and is better aligned with Linux UAPI (c.f., vfio_legacy_pci_hot_reset()).
> 
> In any case, I intend to limit the scope of this series to fix the overflow. Further improvements like moving the code to container-legacy.c may be done later.

Sure.

Thanks,

C.