* [PATCH] vfio: fix use-after-free in display @ 2020-07-13 12:45 Gerd Hoffmann 2020-07-13 12:51 ` Philippe Mathieu-Daudé 2020-07-13 20:53 ` Alex Williamson 0 siblings, 2 replies; 6+ messages in thread From: Gerd Hoffmann @ 2020-07-13 12:45 UTC (permalink / raw) To: qemu-devel; +Cc: Alex Williamson, Gerd Hoffmann Calling ramfb_display_update() might replace the DisplaySurface with the boot display, which in turn will free the currently active DisplaySurface. So clear our DisplaySurface pinter (dpy->region.surface pointer) to (a) avoid use-after-free and (b) force replacing the boot display with the real display when switching back. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/vfio/display.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/vfio/display.c b/hw/vfio/display.c index a57a22674d62..342054193b3c 100644 --- a/hw/vfio/display.c +++ b/hw/vfio/display.c @@ -405,6 +405,7 @@ static void vfio_display_region_update(void *opaque) if (!plane.drm_format || !plane.size) { if (dpy->ramfb) { ramfb_display_update(dpy->con, dpy->ramfb); + dpy->region.surface = NULL; } return; } -- 2.18.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: fix use-after-free in display 2020-07-13 12:45 [PATCH] vfio: fix use-after-free in display Gerd Hoffmann @ 2020-07-13 12:51 ` Philippe Mathieu-Daudé 2020-07-13 14:00 ` Gerd Hoffmann 2020-07-13 20:53 ` Alex Williamson 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-13 12:51 UTC (permalink / raw) To: Gerd Hoffmann, qemu-devel; +Cc: Alex Williamson On 7/13/20 2:45 PM, Gerd Hoffmann wrote: > Calling ramfb_display_update() might replace the DisplaySurface with the > boot display, which in turn will free the currently active > DisplaySurface. > > So clear our DisplaySurface pinter (dpy->region.surface pointer) to (a) > avoid use-after-free and (b) force replacing the boot display with the > real display when switching back. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/vfio/display.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c > index a57a22674d62..342054193b3c 100644 > --- a/hw/vfio/display.c > +++ b/hw/vfio/display.c > @@ -405,6 +405,7 @@ static void vfio_display_region_update(void *opaque) > if (!plane.drm_format || !plane.size) { > if (dpy->ramfb) { > ramfb_display_update(dpy->con, dpy->ramfb); > + dpy->region.surface = NULL; > } > return; > } > More generic fix: -- >8 -- --- a/ui/console.c +++ b/ui/console.c @@ -1580,10 +1580,10 @@ void dpy_gfx_replace_surface(QemuConsole *con, DisplaySurface *surface) { DisplayState *s = con->ds; - DisplaySurface *old_surface = con->surface; + QemuConsole *old_con = con; DisplayChangeListener *dcl; - assert(old_surface != surface || surface == NULL); + assert(con->surface != surface || surface == NULL); con->surface = surface; QLIST_FOREACH(dcl, &s->listeners, next) { @@ -1594,7 +1594,8 @@ void dpy_gfx_replace_surface(QemuConsole *con, dcl->ops->dpy_gfx_switch(dcl, surface); } } - qemu_free_displaysurface(old_surface); + qemu_free_displaysurface(old_con->surface); + old_con->surface = NULL; } --- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: fix use-after-free in display 2020-07-13 12:51 ` Philippe Mathieu-Daudé @ 2020-07-13 14:00 ` Gerd Hoffmann 2020-07-13 14:09 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 6+ messages in thread From: Gerd Hoffmann @ 2020-07-13 14:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Alex Williamson, qemu-devel On Mon, Jul 13, 2020 at 02:51:05PM +0200, Philippe Mathieu-Daudé wrote: > On 7/13/20 2:45 PM, Gerd Hoffmann wrote: > > Calling ramfb_display_update() might replace the DisplaySurface with the > > boot display, which in turn will free the currently active > > DisplaySurface. > > > > So clear our DisplaySurface pinter (dpy->region.surface pointer) to (a) > > avoid use-after-free and (b) force replacing the boot display with the > > real display when switching back. > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > hw/vfio/display.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c > > index a57a22674d62..342054193b3c 100644 > > --- a/hw/vfio/display.c > > +++ b/hw/vfio/display.c > > @@ -405,6 +405,7 @@ static void vfio_display_region_update(void *opaque) > > if (!plane.drm_format || !plane.size) { > > if (dpy->ramfb) { > > ramfb_display_update(dpy->con, dpy->ramfb); > > + dpy->region.surface = NULL; > > } > > return; > > } > > > > More generic fix: > > -- >8 -- > --- a/ui/console.c > +++ b/ui/console.c > @@ -1580,10 +1580,10 @@ void dpy_gfx_replace_surface(QemuConsole *con, > DisplaySurface *surface) > { > DisplayState *s = con->ds; > - DisplaySurface *old_surface = con->surface; > + QemuConsole *old_con = con; > DisplayChangeListener *dcl; > > - assert(old_surface != surface || surface == NULL); > + assert(con->surface != surface || surface == NULL); > > con->surface = surface; > QLIST_FOREACH(dcl, &s->listeners, next) { > @@ -1594,7 +1594,8 @@ void dpy_gfx_replace_surface(QemuConsole *con, > dcl->ops->dpy_gfx_switch(dcl, surface); > } > } > - qemu_free_displaysurface(old_surface); > + qemu_free_displaysurface(old_con->surface); > + old_con->surface = NULL; No. That doesn't clear VFIODisplay->region.surface, but it sets QemuConsole->surface to NULL no matter what got passed to dpy_gfx_replace_surface(). Guesswork based just on the patch chunk doesn't always work, sometimes you have to consult the source code to see what the patch actually does ;) take care, Gerd ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: fix use-after-free in display 2020-07-13 14:00 ` Gerd Hoffmann @ 2020-07-13 14:09 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-13 14:09 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: Alex Williamson, qemu-devel On 7/13/20 4:00 PM, Gerd Hoffmann wrote: > On Mon, Jul 13, 2020 at 02:51:05PM +0200, Philippe Mathieu-Daudé wrote: >> On 7/13/20 2:45 PM, Gerd Hoffmann wrote: >>> Calling ramfb_display_update() might replace the DisplaySurface with the >>> boot display, which in turn will free the currently active >>> DisplaySurface. >>> >>> So clear our DisplaySurface pinter (dpy->region.surface pointer) to (a) >>> avoid use-after-free and (b) force replacing the boot display with the >>> real display when switching back. >>> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >>> --- >>> hw/vfio/display.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c >>> index a57a22674d62..342054193b3c 100644 >>> --- a/hw/vfio/display.c >>> +++ b/hw/vfio/display.c >>> @@ -405,6 +405,7 @@ static void vfio_display_region_update(void *opaque) >>> if (!plane.drm_format || !plane.size) { >>> if (dpy->ramfb) { >>> ramfb_display_update(dpy->con, dpy->ramfb); >>> + dpy->region.surface = NULL; >>> } >>> return; >>> } >>> >> >> More generic fix: >> >> -- >8 -- >> --- a/ui/console.c >> +++ b/ui/console.c >> @@ -1580,10 +1580,10 @@ void dpy_gfx_replace_surface(QemuConsole *con, >> DisplaySurface *surface) >> { >> DisplayState *s = con->ds; >> - DisplaySurface *old_surface = con->surface; >> + QemuConsole *old_con = con; >> DisplayChangeListener *dcl; >> >> - assert(old_surface != surface || surface == NULL); >> + assert(con->surface != surface || surface == NULL); >> >> con->surface = surface; >> QLIST_FOREACH(dcl, &s->listeners, next) { >> @@ -1594,7 +1594,8 @@ void dpy_gfx_replace_surface(QemuConsole *con, >> dcl->ops->dpy_gfx_switch(dcl, surface); >> } >> } >> - qemu_free_displaysurface(old_surface); >> + qemu_free_displaysurface(old_con->surface); >> + old_con->surface = NULL; > > No. > > That doesn't clear VFIODisplay->region.surface, but it sets > QemuConsole->surface to NULL no matter what got passed to > dpy_gfx_replace_surface(). > > Guesswork based just on the patch chunk doesn't always work, > sometimes you have to consult the source code to see what the > patch actually does ;) I'm certainly not a source of trust producing perfect code :) I appreciate being reviewed on snippets, so I can learn from my mistakes and improve. I looked at the code (ui/console.c was not in the previous context) but I missed to look at the other call sites for dpy_gfx_replace_surface() and how it is used. Thanks, Phil. > > take care, > Gerd > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: fix use-after-free in display 2020-07-13 12:45 [PATCH] vfio: fix use-after-free in display Gerd Hoffmann 2020-07-13 12:51 ` Philippe Mathieu-Daudé @ 2020-07-13 20:53 ` Alex Williamson 2020-07-15 10:40 ` Gerd Hoffmann 1 sibling, 1 reply; 6+ messages in thread From: Alex Williamson @ 2020-07-13 20:53 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel On Mon, 13 Jul 2020 14:45:20 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Calling ramfb_display_update() might replace the DisplaySurface with the > boot display, which in turn will free the currently active > DisplaySurface. > > So clear our DisplaySurface pinter (dpy->region.surface pointer) to (a) ^o > avoid use-after-free and (b) force replacing the boot display with the > real display when switching back. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/vfio/display.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c > index a57a22674d62..342054193b3c 100644 > --- a/hw/vfio/display.c > +++ b/hw/vfio/display.c > @@ -405,6 +405,7 @@ static void vfio_display_region_update(void *opaque) > if (!plane.drm_format || !plane.size) { > if (dpy->ramfb) { > ramfb_display_update(dpy->con, dpy->ramfb); > + dpy->region.surface = NULL; > } > return; > } Tricky, but I think I follow that dpy->region.surface is only ever allocated to replace dpy->con->surface, so when ramfb_display_update() then replaces and frees dpy->con->surface with dpy->ramfb->ds, that's where the object point to by dpy->region.surface was freed. Right? If so, looks ok to me. If you're constructing a pull request, I'll give you an: Acked-by: Alex Williamson <alex.williamson@redhat.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> If you need me to send a pull, let me know. Thanks, Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vfio: fix use-after-free in display 2020-07-13 20:53 ` Alex Williamson @ 2020-07-15 10:40 ` Gerd Hoffmann 0 siblings, 0 replies; 6+ messages in thread From: Gerd Hoffmann @ 2020-07-15 10:40 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel Hi, > Tricky, but I think I follow that dpy->region.surface is only ever > allocated to replace dpy->con->surface, so when ramfb_display_update() > then replaces and frees dpy->con->surface with dpy->ramfb->ds, that's > where the object point to by dpy->region.surface was freed. Right? Correct. > If so, looks ok to me. If you're constructing a pull request, I'll > give you an: > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > If you need me to send a pull, let me know. I'll go create a pull req, I have one or two other patches pending anyway. take care, Gerd ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-15 10:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-13 12:45 [PATCH] vfio: fix use-after-free in display Gerd Hoffmann 2020-07-13 12:51 ` Philippe Mathieu-Daudé 2020-07-13 14:00 ` Gerd Hoffmann 2020-07-13 14:09 ` Philippe Mathieu-Daudé 2020-07-13 20:53 ` Alex Williamson 2020-07-15 10:40 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).