We should free the duplicated variant of vbasedev->name plus uuid
rather than vbasedev->name itself.

Fixes: 2dca1b37a7 ("vfio/pci: add support for VF toke")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bf27a3990564..d2593681e000 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2998,7 +2998,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     }
 
     ret = vfio_get_device(group, name, vbasedev, errp);
-    g_free(name);
+    if (name != vbasedev->name) {
+        g_free(name);
+    }
     if (ret) {
         vfio_put_group(group);
         goto error;
-- 
2.34.1

On 5/16/23 05:43, Zhenzhong Duan wrote:
> We should free the duplicated variant of vbasedev->name plus uuid
> rather than vbasedev->name itself.
> 
> Fixes: 2dca1b37a7 ("vfio/pci: add support for VF toke")

"toke" -> "token"

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/vfio/pci.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index bf27a3990564..d2593681e000 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2998,7 +2998,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       }
>   
>       ret = vfio_get_device(group, name, vbasedev, errp);
> -    g_free(name);
> +    if (name != vbasedev->name) {


yes. I wonder if we shouldn't use the same test with which 'name' was
allocated instead :

     if (!qemu_uuid_is_null(&vdev->vf_token)) {

Thanks,

C.


> +        g_free(name);
> +    }
>       if (ret) {
>           vfio_put_group(group);
>           goto error;

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, May 16, 2023 4:58 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: minwoo.im@samsung.com; alex.williamson@redhat.com; Peng, Chao P
><chao.p.peng@intel.com>
>Subject: Re: [PATCH] vfio/pci: Fix a use-after-free issue
>
>On 5/16/23 05:43, Zhenzhong Duan wrote:
>> We should free the duplicated variant of vbasedev->name plus uuid
>> rather than vbasedev->name itself.
>>
>> Fixes: 2dca1b37a7 ("vfio/pci: add support for VF toke")
>
>"toke" -> "token"
Will fix, thanks

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>> bf27a3990564..d2593681e000 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2998,7 +2998,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>       }
>>
>>       ret = vfio_get_device(group, name, vbasedev, errp);
>> -    g_free(name);
>> +    if (name != vbasedev->name) {
>
>
>yes. I wonder if we shouldn't use the same test with which 'name' was
>allocated instead :
>
>     if (!qemu_uuid_is_null(&vdev->vf_token)) {

I think they are same effect and " if (name != vbasedev->name) {" is a bit
more optimal. If you prefer " if (!qemu_uuid_is_null(&vdev->vf_token)) {",
let me know and I'll update in v2.

Thanks
Zhenzhong

On Tue, 16 May 2023 10:02:24 +0000
"Duan, Zhenzhong" <zhenzhong.duan@intel.com> wrote:

> >-----Original Message-----
> >From: Cédric Le Goater <clg@redhat.com>
> >Sent: Tuesday, May 16, 2023 4:58 PM
> >To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
> >devel@nongnu.org
> >Cc: minwoo.im@samsung.com; alex.williamson@redhat.com; Peng, Chao P
> ><chao.p.peng@intel.com>
> >Subject: Re: [PATCH] vfio/pci: Fix a use-after-free issue
> >
> >On 5/16/23 05:43, Zhenzhong Duan wrote:  
> >> We should free the duplicated variant of vbasedev->name plus uuid
> >> rather than vbasedev->name itself.
> >>
> >> Fixes: 2dca1b37a7 ("vfio/pci: add support for VF toke")  
> >
> >"toke" -> "token"  
> Will fix, thanks
> 
> >  
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >> ---
> >>   hw/vfio/pci.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> >> bf27a3990564..d2593681e000 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -2998,7 +2998,9 @@ static void vfio_realize(PCIDevice *pdev, Error  
> >**errp)  
> >>       }
> >>
> >>       ret = vfio_get_device(group, name, vbasedev, errp);
> >> -    g_free(name);
> >> +    if (name != vbasedev->name) {  
> >
> >
> >yes. I wonder if we shouldn't use the same test with which 'name' was
> >allocated instead :
> >
> >     if (!qemu_uuid_is_null(&vdev->vf_token)) {  
> 
> I think they are same effect and " if (name != vbasedev->name) {" is a bit
> more optimal. If you prefer " if (!qemu_uuid_is_null(&vdev->vf_token)) {",
> let me know and I'll update in v2.

My preference would be that both paths allocate name so that we don't
need to conditionalize the free.  For example:

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bf27a3990564..73874a94de12 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2994,7 +2994,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         qemu_uuid_unparse(&vdev->vf_token, uuid);
         name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
     } else {
-        name = vbasedev->name;
+        name = g_strdup(vbasedev->name);
     }
 
     ret = vfio_get_device(group, name, vbasedev, errp);

Thanks,
Alex