Re: [PULL 1/3] vfio/pci: add support for VF token

["Cédric Le Goater" <clegoate@redhat.com>]

On 10/20/23 15:32, Peter Maydell wrote:
> On Tue, 9 May 2023 at 23:01, Alex Williamson <alex.williamson@redhat.com> wrote:
>>
>> From: Minwoo Im <minwoo.im@samsung.com>
>>
>> VF token was introduced [1] to kernel vfio-pci along with SR-IOV
>> support [2].  This patch adds support VF token among PF and VF(s). To
>> passthu PCIe VF to a VM, kernel >= v5.7 needs this.
>>
>> It can be configured with UUID like:
>>
>>    -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...
>>
>> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
>> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/
>>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi; Coverity points out that this change introduces a buffer
> overrun (CID 1522913). I dunno why it's taken it so long
> to notice...
> 
>> ---
>>   hw/vfio/pci.c | 13 ++++++++++++-
>>   hw/vfio/pci.h |  1 +
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ec9a854361ac..cf27f28936cb 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>       int groupid;
>>       int i, ret;
>>       bool is_mdev;
>> +    char uuid[UUID_FMT_LEN];
> 
> We define the array uuid[] as UUID_FMT_LEN bytes long...
> 
>> +    char *name;
>>
>>       if (!vbasedev->sysfsdev) {
>>           if (!(~vdev->host.domain || ~vdev->host.bus ||
>> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>           goto error;
>>       }
>>
>> -    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
>> +    if (!qemu_uuid_is_null(&vdev->vf_token)) {
>> +        qemu_uuid_unparse(&vdev->vf_token, uuid);
> 
> ...but qemu_uuid_unparse() writes UUID_FMT_LEN + 1 bytes,
> including a trailing NUL.
> 
> Every other use of UUID_FMT_LEN to declare an array
> uses "UUID_FMT_LEN + 1" to avoid this.

We also have :

     char uuidstr[37];

in vdi_header_print() and other places like test-uuid.


> (In fact, every use of UUID_FMT_LEN at all uses "+ 1",
> which suggests that perhaps defining it differently (and
> perhaps with a different name) would reduce the risk of
> this particular bug...)

libuuid defines :

   #define UUID_STR_LEN	37

QEMU could do the same ?

Thanks,

C.

> 
> thanks
> -- PMM
>