[PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

Andrey Drobyshev posted 3 patches 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260825172051.435372-1-andrey.drobyshev@virtuozzo.com
hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
hw/display/trace-events |   1 +
2 files changed, 97 insertions(+), 36 deletions(-)
[PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Andrey Drobyshev 1 month ago
We've experienced crashes when spice-server aborts on migration target
when processing an invalid/stale guest address pointer.  While those are
hard to reproduce, it seems they can be fairly easily avoided by simply
skipping commands with invalid addresses in qxl_post_load.

Andrey Drobyshev (3):
  hw/display/qxl: factor out qxl_guest_phys2virt()
  hw/display/qxl: validate replayed commands in qxl_post_load
  hw/display/qxl: trace skipped stale loadvm commands

 hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
 hw/display/trace-events |   1 +
 2 files changed, 97 insertions(+), 36 deletions(-)

-- 
2.47.1
Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Michael Tokarev 1 week, 5 days ago
On 8/25/26 20:20, Andrey Drobyshev wrote:
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.
> 
> Andrey Drobyshev (3):
>    hw/display/qxl: factor out qxl_guest_phys2virt()
>    hw/display/qxl: validate replayed commands in qxl_post_load
>    hw/display/qxl: trace skipped stale loadvm commands
> 
>   hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>   hw/display/trace-events |   1 +
>   2 files changed, 97 insertions(+), 36 deletions(-)
Hi!

Is this a qemu-stable material?  Or there's no need to bother?

Thanks,

/mjt
Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Andrey Drobyshev 1 week, 5 days ago
On 9/14/26 4:57 PM, Michael Tokarev wrote:
> On 8/25/26 20:20, Andrey Drobyshev wrote:
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
>>
>> Andrey Drobyshev (3):
>>    hw/display/qxl: factor out qxl_guest_phys2virt()
>>    hw/display/qxl: validate replayed commands in qxl_post_load
>>    hw/display/qxl: trace skipped stale loadvm commands
>>
>>   hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>>   hw/display/trace-events |   1 +
>>   2 files changed, 97 insertions(+), 36 deletions(-)
> Hi!
> 
> Is this a qemu-stable material?  Or there's no need to bother?
> 
> Thanks,
> 
> /mjt

IMHO it's not necessarily a qemu-stable material as the crash itself is
hard to reproduce and I haven't seen other people complaining about it.
Still, feel free to pull it.

Andrey
Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Marc-André Lureau 2 weeks ago
On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.
>
> Andrey Drobyshev (3):
>   hw/display/qxl: factor out qxl_guest_phys2virt()
>   hw/display/qxl: validate replayed commands in qxl_post_load
>   hw/display/qxl: trace skipped stale loadvm commands
>
>  hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>  hw/display/trace-events |   1 +
>  2 files changed, 97 insertions(+), 36 deletions(-)

series lgtm:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>
> --
> 2.47.1
>
>
Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Marc-André Lureau 1 month ago
Hi Andrey

On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.

I would rather try to find the root cause, rather than applying a
post-load workaround. It may be related to this issue
https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
there is a lack of resource tracking, when a guest release
slot/cursors etc, the track commands may not be correctly updated - or
preventing resource release. Unfortunately, spice/qxl is no longer
maintained - we should clearly mark this device as unsecure imho.

> Andrey Drobyshev (3):
>   hw/display/qxl: factor out qxl_guest_phys2virt()
>   hw/display/qxl: validate replayed commands in qxl_post_load
>   hw/display/qxl: trace skipped stale loadvm commands
>
>  hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>  hw/display/trace-events |   1 +
>  2 files changed, 97 insertions(+), 36 deletions(-)
>
> --
> 2.47.1
>
>
Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Denis V. Lunev 1 month ago
On 8/26/26 10:10, Marc-André Lureau wrote:
> Hi Andrey
>
> On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
> I would rather try to find the root cause, rather than applying a
> post-load workaround. It may be related to this issue
> https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
> there is a lack of resource tracking, when a guest release
> slot/cursors etc, the track commands may not be correctly updated - or
> preventing resource release. Unfortunately, spice/qxl is no longer
> maintained - we should clearly mark this device as unsecure imho.

Unfortunately we do not have access to this item in gitlab.
Can you grant it at least to den@openvz.org?

Den

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load
Posted by Andrey Drobyshev 1 month ago
On 8/26/26 11:10 AM, Marc-André Lureau wrote:
> Hi Andrey
> 
> On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>>
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
> 
> I would rather try to find the root cause, rather than applying a
> post-load workaround. It may be related to this issue
> https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
> there is a lack of resource tracking, when a guest release
> slot/cursors etc, the track commands may not be correctly updated - or
> preventing resource release. Unfortunately, spice/qxl is no longer
> maintained - we should clearly mark this device as unsecure imho.
> 
>> [...]
I agree on all points. AFAIU this is a use-after-free of a tracked guest
pointer.  I'm not sure whether we can reliably track whenever guest
frees a buffer, so we drop it from the cmd list.  Still, I'd argue that
my 2nd patch here adds some hardening:

1. Currently we consume guest-controlled bytes which reach
memslot_get_virt() in spice-server, which aborts on the migration
target.  So there's a potential for guest->host DoS.
2. Dropping invalid commands is cheap, as we just don't replay cursor
movements on the target - instead wait until guest sets it again.  IMHO
this kind of hardening would make sense even if we did remedy the root
cause.

Andrey