Hi
On Tue, Oct 18, 2016 at 11:46 AM P J P <ppan...@redhat.com> wrote:
> From: Prasad J Pandit <p...@fedoraproject.org>
>
> In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA,
> 'cirrus_get_bpp' returns zero(0), which could lead to a divide
> by zero error in while copying pixel data. The same could occur
> via blit pitch values. Add check to avoid it.
>
For completeness, do you have a reproducer and/or a backtrace?
>
> Reported-by: Huawei PSIRT <ps...@huawei.com>
> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> ---
> hw/display/cirrus_vga.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 3d712d5..bdb092e 100644
> --- 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?
if (pitch < 0) {
> int64_t min = addr
> + ((int64_t)s->cirrus_blt_height-1) * pitch;
> @@ -715,7 +718,7 @@ static int
> cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
> s->cirrus_addr_mask));
> }
>
> -static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w,
> int h)
> +static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int
> h)
> {
> int sx = 0, sy = 0;
> int dx = 0, dy = 0;
> @@ -729,6 +732,9 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst,
> int src, int w, int h)
> int width, height;
>
> depth = s->vga.get_bpp(&s->vga) / 8;
> + if (!depth) {
> + return 0;
> + }
>
Makes sense, since 'cirrus_get_bpp' would return 0 in VGA mode. But isn't
this a cirrus operation (not VGA), how did it get there? Perhaps this
should be catched earlier (invalid VGA operations).
s->vga.get_resolution(&s->vga, &width, &height);
>
> /* extra x, y */
> @@ -783,6 +789,8 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst,
> int src, int w, int h)
> cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
> s->cirrus_blt_dstpitch,
> s->cirrus_blt_width,
> s->cirrus_blt_height);
> +
> + return 1;
> }
>
> static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
> @@ -790,11 +798,9 @@ static int
> cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
> if (blit_is_unsafe(s))
> return 0;
>
> - cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
> + return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
> s->cirrus_blt_srcaddr - s->vga.start_addr,
> s->cirrus_blt_width, s->cirrus_blt_height);
> -
> - return 1;
>
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 hope someone more familiar with the code can help review your patch.
Thanks
}
>
> /***************************************
> --
> 2.7.4
>
>
> --
Marc-André Lureau
