| Message ID | 20230516034357.61276-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Fix a use-after-free issue | expand |
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
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;
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(-)