On Tue, 16 May 2023 10:02:24 +0000
"Duan, Zhenzhong" <zhenzhong.d...@intel.com> wrote:
> >-----Original Message-----
> >From: Cédric Le Goater <c...@redhat.com>
> >Sent: Tuesday, May 16, 2023 4:58 PM
> >To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; qemu-
> >de...@nongnu.org
> >Cc: minwoo...@samsung.com; alex.william...@redhat.com; Peng, Chao P
> ><chao.p.p...@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.d...@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
