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

[Marc-André Lureau]

Hi

On Tue, Oct 18, 2016 at 11:46 AM P J P <address@hidden> wrote:

> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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