[Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Max Reitz]

There are some 2D resource formats that can be used through virtio-gpu,
but which are not supported by SDL2 when used for a scanout; these are
all alpha-channel formats and also XBGR (RGBX in non-BE pixman).

Add these formats in the switch converting pixman to SDL format
constants so a guest cannot crash the VM by triggering the
g_assert_not_reached() with an unsupported format.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 ui/sdl2-2d.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c
index 85484407be..c9df72636a 100644
--- a/ui/sdl2-2d.c
+++ b/ui/sdl2-2d.c
@@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl,
     case PIXMAN_r5g6b5:
         format = SDL_PIXELFORMAT_RGB565;
         break;
+    case PIXMAN_a8r8g8b8:
     case PIXMAN_x8r8g8b8:
         format = SDL_PIXELFORMAT_ARGB8888;
         break;
+    case PIXMAN_a8b8g8r8:
+    case PIXMAN_x8b8g8r8:
+        format = SDL_PIXELFORMAT_ABGR8888;
+        break;
+    case PIXMAN_r8g8b8a8:
     case PIXMAN_r8g8b8x8:
         format = SDL_PIXELFORMAT_RGBA8888;
         break;
     case PIXMAN_b8g8r8x8:
         format = SDL_PIXELFORMAT_BGRX8888;
         break;
+    case PIXMAN_b8g8r8a8:
+        format = SDL_PIXELFORMAT_BGRA8888;
+        break;
     default:
         g_assert_not_reached();
     }
-- 
2.17.1

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On 08.10.18 20:50, Max Reitz wrote:
> There are some 2D resource formats that can be used through virtio-gpu,
> but which are not supported by SDL2 when used for a scanout; these are
> all alpha-channel formats and also XBGR (RGBX in non-BE pixman).

Oops, it's the other way round.  The virtio-gpu format is RGBX, and the
non-BE pixman (and SDL) constant is XBGR.

Max

> Add these formats in the switch converting pixman to SDL format
> constants so a guest cannot crash the VM by triggering the
> g_assert_not_reached() with an unsupported format.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  ui/sdl2-2d.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c
> index 85484407be..c9df72636a 100644
> --- a/ui/sdl2-2d.c
> +++ b/ui/sdl2-2d.c
> @@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl,
>      case PIXMAN_r5g6b5:
>          format = SDL_PIXELFORMAT_RGB565;
>          break;
> +    case PIXMAN_a8r8g8b8:
>      case PIXMAN_x8r8g8b8:
>          format = SDL_PIXELFORMAT_ARGB8888;
>          break;
> +    case PIXMAN_a8b8g8r8:
> +    case PIXMAN_x8b8g8r8:
> +        format = SDL_PIXELFORMAT_ABGR8888;
> +        break;
> +    case PIXMAN_r8g8b8a8:
>      case PIXMAN_r8g8b8x8:
>          format = SDL_PIXELFORMAT_RGBA8888;
>          break;
>      case PIXMAN_b8g8r8x8:
>          format = SDL_PIXELFORMAT_BGRX8888;
>          break;
> +    case PIXMAN_b8g8r8a8:
> +        format = SDL_PIXELFORMAT_BGRA8888;
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Gerd Hoffmann]

On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
> There are some 2D resource formats that can be used through virtio-gpu,

Ahem, not really.  XRGB is the only one which works in practice, and
virtio-gpu kms driver will stop advertising anything else soon (patches
should land upstream with the next merge window).

> Add these formats in the switch converting pixman to SDL format
> constants so a guest cannot crash the VM by triggering the
> g_assert_not_reached() with an unsupported format.

Do you have a reproducer for that?

There is sdl2_2d_check_format() which reports the supported formats.
If we hit sdl2_2d_switch() with a format not whitelisted by
sdl2_2d_check_format() we have a bug somewhere in qemu ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

On 10.10.18 12:10, Gerd Hoffmann wrote:
> On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
>> There are some 2D resource formats that can be used through virtio-gpu,
> 
> Ahem, not really.  XRGB is the only one which works in practice, and
> virtio-gpu kms driver will stop advertising anything else soon (patches
> should land upstream with the next merge window).

OK, if virtio-gpu didn't support anything else, that'd be a fix, too.
But it sounds like you're talking about the Linux driver, I'm not.

This is not about Linux applications being able to abuse the Linux
driver to crash the VM, this is about malicious drivers (not necessarily
Linux drivers).

>> Add these formats in the switch converting pixman to SDL format
>> constants so a guest cannot crash the VM by triggering the
>> g_assert_not_reached() with an unsupported format.
> 
> Do you have a reproducer for that?

I have attached two RISC-V kernels, one (kernel-rgbx) setting
VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM, the other (kernel-bgra) setting
VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM.  Both crash qemu:

$ $QEMU/build/riscv64-softmmu/qemu-system-riscv64 -kernel kernel-rgbx \
    -serial stdio -M virt -device virtio-gpu-device
[platform-virt] Virt platform detected
[virtio-gpu] Found device @0x10008000
[virtio-gpu] Scanout 0: 0x0:1024x768
**
ERROR:$QEMU/ui/sdl2-2d.c:114:sdl2_2d_switch: code should not be reached
[1]    7151 abort (core dumped)

So this is not about a misbehaving Linux driver but about an own driver.
 Of course, if you can insert kernel code, there's noone stopping you
from hitting that assertion with Linux, too.

> There is sdl2_2d_check_format() which reports the supported formats.
> If we hit sdl2_2d_switch() with a format not whitelisted by
> sdl2_2d_check_format() we have a bug somewhere in qemu ...

I suppose the other solution would be for virtio_gpu_set_scanout() to
check whether the resource's format can actually be used for that
display.  Or in virtio_gpu_resource_create_2d(), I don't know whether
it's possible to use resources in other formats at all.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2266 bytes --]

On 10.10.18 19:35, Max Reitz wrote:
> On 10.10.18 12:10, Gerd Hoffmann wrote:
>> On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
>>> There are some 2D resource formats that can be used through virtio-gpu,
>>
>> Ahem, not really.  XRGB is the only one which works in practice, and
>> virtio-gpu kms driver will stop advertising anything else soon (patches
>> should land upstream with the next merge window).
> 
> OK, if virtio-gpu didn't support anything else, that'd be a fix, too.
> But it sounds like you're talking about the Linux driver, I'm not.
> 
> This is not about Linux applications being able to abuse the Linux
> driver to crash the VM, this is about malicious drivers (not necessarily
> Linux drivers).
> 
>>> Add these formats in the switch converting pixman to SDL format
>>> constants so a guest cannot crash the VM by triggering the
>>> g_assert_not_reached() with an unsupported format.
>>
>> Do you have a reproducer for that?
> 
> I have attached two RISC-V kernels, one (kernel-rgbx) setting
> VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM, the other (kernel-bgra) setting
> VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM.  Both crash qemu:

Of course I hadn't.

> $ $QEMU/build/riscv64-softmmu/qemu-system-riscv64 -kernel kernel-rgbx \
>     -serial stdio -M virt -device virtio-gpu-device
> [platform-virt] Virt platform detected
> [virtio-gpu] Found device @0x10008000
> [virtio-gpu] Scanout 0: 0x0:1024x768
> **
> ERROR:$QEMU/ui/sdl2-2d.c:114:sdl2_2d_switch: code should not be reached
> [1]    7151 abort (core dumped)
> 
> So this is not about a misbehaving Linux driver but about an own driver.
>  Of course, if you can insert kernel code, there's noone stopping you
> from hitting that assertion with Linux, too.
> 
>> There is sdl2_2d_check_format() which reports the supported formats.
>> If we hit sdl2_2d_switch() with a format not whitelisted by
>> sdl2_2d_check_format() we have a bug somewhere in qemu ...
> 
> I suppose the other solution would be for virtio_gpu_set_scanout() to
> check whether the resource's format can actually be used for that
> display.  Or in virtio_gpu_resource_create_2d(), I don't know whether
> it's possible to use resources in other formats at all.
> 
> Max
> 


[-- Attachment #1.2: kernel-rgbx.xz --]
[-- Type: application/x-xz, Size: 16592 bytes --]

[-- Attachment #1.3: kernel-bgra.xz --]
[-- Type: application/x-xz, Size: 16612 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Gerd Hoffmann]

On Wed, Oct 10, 2018 at 07:35:06PM +0200, Max Reitz wrote:
> On 10.10.18 12:10, Gerd Hoffmann wrote:
> > On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
> >> There are some 2D resource formats that can be used through virtio-gpu,
> > 
> > Ahem, not really.  XRGB is the only one which works in practice, and
> > virtio-gpu kms driver will stop advertising anything else soon (patches
> > should land upstream with the next merge window).
> 
> OK, if virtio-gpu didn't support anything else, that'd be a fix, too.
> But it sounds like you're talking about the Linux driver, I'm not.
> 
> This is not about Linux applications being able to abuse the Linux
> driver to crash the VM, this is about malicious drivers (not necessarily
> Linux drivers).
> 
> >> Add these formats in the switch converting pixman to SDL format
> >> constants so a guest cannot crash the VM by triggering the
> >> g_assert_not_reached() with an unsupported format.
> > 
> > Do you have a reproducer for that?
> 
> I have attached two RISC-V kernels, one (kernel-rgbx) setting
> VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM, the other (kernel-bgra) setting
> VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM.  Both crash qemu:
> 
> $ $QEMU/build/riscv64-softmmu/qemu-system-riscv64 -kernel kernel-rgbx \
>     -serial stdio -M virt -device virtio-gpu-device
> [platform-virt] Virt platform detected
> [virtio-gpu] Found device @0x10008000
> [virtio-gpu] Scanout 0: 0x0:1024x768
> **
> ERROR:$QEMU/ui/sdl2-2d.c:114:sdl2_2d_switch: code should not be reached
> [1]    7151 abort (core dumped)

Ok, I see.  If sdl2 can support those formats it makes sense to support
them, but they must be added to both sdl2_2d_check_format() and
sdl2_2d_switch() then.

> > There is sdl2_2d_check_format() which reports the supported formats.
> > If we hit sdl2_2d_switch() with a format not whitelisted by
> > sdl2_2d_check_format() we have a bug somewhere in qemu ...
> 
> I suppose the other solution would be for virtio_gpu_set_scanout() to
> check whether the resource's format can actually be used for that
> display.

Yes, that must be added (independent from the sdl2 patch).

> Or in virtio_gpu_resource_create_2d(), I don't know whether
> it's possible to use resources in other formats at all.

Not directly, they must be converted (using pixman) then.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Gerd Hoffmann]

On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
> There are some 2D resource formats that can be used through virtio-gpu,
> but which are not supported by SDL2 when used for a scanout; these are
> all alpha-channel formats and also XBGR (RGBX in non-BE pixman).
> 
> Add these formats in the switch converting pixman to SDL format
> constants so a guest cannot crash the VM by triggering the
> g_assert_not_reached() with an unsupported format.

fixed (sdl2_2d_check_format update) & queued.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On 12.10.18 14:48, Gerd Hoffmann wrote:
> On Mon, Oct 08, 2018 at 08:50:13PM +0200, Max Reitz wrote:
>> There are some 2D resource formats that can be used through virtio-gpu,
>> but which are not supported by SDL2 when used for a scanout; these are
>> all alpha-channel formats and also XBGR (RGBX in non-BE pixman).
>>
>> Add these formats in the switch converting pixman to SDL format
>> constants so a guest cannot crash the VM by triggering the
>> g_assert_not_reached() with an unsupported format.
> 
> fixed (sdl2_2d_check_format update) & queued.

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]