From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32960) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cBFKV-0003qB-R6 for qemu-devel@nongnu.org; Mon, 28 Nov 2016 01:22:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cBFKS-0000Ys-MW for qemu-devel@nongnu.org; Mon, 28 Nov 2016 01:22:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43748) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cBFKS-0000YG-E2 for qemu-devel@nongnu.org; Mon, 28 Nov 2016 01:22:16 -0500 Date: Mon, 28 Nov 2016 11:52:08 +0530 (IST) From: P J P In-Reply-To: Message-ID: References: <1476776717-24807-1-git-send-email-ppandit@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Marc-Andr=E9_Lureau?= Cc: Qemu Developers , Peter Maydell , Huawei PSIRT Hello Marc, all +-- On Wed, 16 Nov 2016, Marc-Andr=C3=A9 Lureau wrote --+ | For completeness, do you have a reproducer and/or a backtrace? Yes, there is. =3D=3D=3D Thread 4 "qemu-system-x86" received signal SIGFPE, Arithmetic exception. [Switching to Thread 0x7ffff002c700 (LWP 10506)] 0x000055555599fe2e in cirrus_do_copy (s=3D0x55555758af60, dst=3D0, src=3D= 0, w=3D2048,=20 h=3D4096) at hw/display/cirrus_vga.c:735 735 sx =3D (src % ABS(s->cirrus_blt_srcpitch)) / depth; (gdb) bt #0 0x000055555599fe2e in cirrus_do_copy (s=3D0x55555758af60, dst=3D0, sr= c=3D0, w=3D2048, h=3D4096) at hw/display/cirrus_vga.c:735 #1 0x00005555559a0134 in cirrus_bitblt_videotovideo_copy (s=3D0x55555758= af60) at hw/display/cirrus_vga.c:793 #2 0x00005555559a0609 in cirrus_bitblt_videotovideo (s=3D0x55555758af60)= at hw/display/cirrus_vga.c:915 #3 0x00005555559a0d77 in cirrus_bitblt_start (s=3D0x55555758af60) at hw/= display/cirrus_vga.c:1056 #4 0x00005555559a1ad3 in cirrus_vga_write_gr (s=3D0x55555758af60, reg_in= dex=3D42, reg_value=3D0) at hw/display/cirrus_vga.c:1572 #5 0x00005555559a3ad8 in cirrus_vga_ioport_write (opaque=3D0x55555758af6= 0, addr=3D975, val=3D0, size=3D1) at hw/display/cirrus_vga.c:2678 #6 0x00005555557a8df7 in memory_region_write_accessor (mr=3D0x55555759ba= 50, addr=3D31, ... #7 0x00005555557a900f in access_with_adjusted_size (addr=3D31, value=3D0= x7ffff002b8b8, ... #8 0x00005555557ab74f in memory_region_dispatch_write (mr=3D0x55555759ba= 50, addr=3D31, ... #9 0x0000555555757003 in address_space_write_continue (as=3D0x55555621b5= a0 , ... #10 0x000055555575714b in address_space_write (as=3D0x55555621b5a0 , ... #11 0x00005555557574d7 in address_space_rw (as=3D0x55555621b5a0 , ... #12 0x00005555557a53d1 in kvm_handle_io (port=3D975, attrs=3D..., data=3D= 0x7ffff7ff0000, ... #13 0x00005555557a58d7 in kvm_cpu_exec (cpu=3D0x555556746f90) #14 0x000055555578c752 in qemu_kvm_cpu_thread_fn (arg=3D0x555556746f90) #15 0x00007ffff5e8d5ca in start_thread () from /lib64/libpthread.so.0 #16 0x00007ffff5bc70ed in clone () from /lib64/libc.so.6 =3D=3D=3D | > --- a/hw/display/cirrus_vga.c | > +++ b/hw/display/cirrus_vga.c | > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGA= State | > *s); | > static bool blit_region_is_unsafe(struct CirrusVGAState *s, | > int32_t pitch, int32_t addr) | > { | > + if (!pitch) { | > + return true; | > + } | > |=20 | 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 bli= t=20 parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch) are safe for=20 'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy static int cirrus_do_copy(CirrusVGAState *s, ...) { ... sx =3D (src % ABS(s->cirrus_blt_srcpitch)) / depth; sy =3D (src / ABS(s->cirrus_blt_srcpitch)); dx =3D (dst % ABS(s->cirrus_blt_dstpitch)) / depth; dy =3D (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 i= t | 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