Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[P J P <ppandit@redhat.com>]

  Hello Marc, all

+-- On Wed, 16 Nov 2016, Marc-André Lureau wrote --+
| For completeness, do you have a reproducer and/or a backtrace?

Yes, there is.

===
Thread 4 "qemu-system-x86" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7ffff002c700 (LWP 10506)]
0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, 
h=4096) at hw/display/cirrus_vga.c:735
735             sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;

(gdb) bt
#0  0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, h=4096) at hw/display/cirrus_vga.c:735
#1  0x00005555559a0134 in cirrus_bitblt_videotovideo_copy (s=0x55555758af60) at hw/display/cirrus_vga.c:793
#2  0x00005555559a0609 in cirrus_bitblt_videotovideo (s=0x55555758af60) at hw/display/cirrus_vga.c:915
#3  0x00005555559a0d77 in cirrus_bitblt_start (s=0x55555758af60) at hw/display/cirrus_vga.c:1056
#4  0x00005555559a1ad3 in cirrus_vga_write_gr (s=0x55555758af60, reg_index=42, reg_value=0) at hw/display/cirrus_vga.c:1572
#5  0x00005555559a3ad8 in cirrus_vga_ioport_write (opaque=0x55555758af60, addr=975, val=0, size=1) at hw/display/cirrus_vga.c:2678
#6  0x00005555557a8df7 in memory_region_write_accessor (mr=0x55555759ba50, addr=31, ...
#7  0x00005555557a900f in access_with_adjusted_size (addr=31, value=0x7ffff002b8b8, ...
#8  0x00005555557ab74f in memory_region_dispatch_write (mr=0x55555759ba50, addr=31, ...
#9  0x0000555555757003 in address_space_write_continue (as=0x55555621b5a0 <address_space_io>, ...
#10 0x000055555575714b in address_space_write (as=0x55555621b5a0 <address_space_io>, ...
#11 0x00005555557574d7 in address_space_rw (as=0x55555621b5a0 <address_space_io>, ...
#12 0x00005555557a53d1 in kvm_handle_io (port=975, attrs=..., data=0x7ffff7ff0000, ...
#13 0x00005555557a58d7 in kvm_cpu_exec (cpu=0x555556746f90)
#14 0x000055555578c752 in qemu_kvm_cpu_thread_fn (arg=0x555556746f90)
#15 0x00007ffff5e8d5ca in start_thread () from /lib64/libpthread.so.0
#16 0x00007ffff5bc70ed in clone () from /lib64/libc.so.6
===

| > --- a/hw/display/cirrus_vga.c
| > +++ b/hw/display/cirrus_vga.c
| > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
| > *s);
| >  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
| >                                    int32_t pitch, int32_t addr)
| >  {
| > +    if (!pitch) {
| > +        return true;
| > +    }
| >
| 
| That doesn't look directly related to 'cirrus_get_bpp', care to explain?

  'blit_region_is_unsafe' is called from 'blit_is_unsafe' to check if blit 
parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch)  are safe for 
'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy

    static int cirrus_do_copy(CirrusVGAState *s, ...)
    {
        ...
        sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
        sy = (src / ABS(s->cirrus_blt_srcpitch));
        dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
        dy = (dst / ABS(s->cirrus_blt_dstpitch));
    }

| btw, not directly related to your patch, but the code looks strange in
| cirrus_bitblt_videotovideo(), cirrus_bitblt_reset() is called if(ret), and
| later if (!ret) in cirrus_bitblt_start(), that looks a bit weird, but it
| may be fine.

  I think that is to avoid calling 'cirrus_bitblt_reset' twice.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F