Re: [PATCH] vfio/pci: Fix a use-after-free issue

[Alex Williamson]

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