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
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; } ---
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
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 >
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
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
© 2016 - 2024 Red Hat, Inc.