Re: [PATCH] ati: mask x y display parameter values

[BALATON Zoltan]

On Sun, 18 Oct 2020, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> The source and destination x,y display parameters in ati_2d_blt()
> may run off the vga limits if either of s->regs.[src|dst]_[xy] is
> zero. Mask the register values to avoid potential crash.
>
> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/display/ati_2d.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 23a8ae0cd8..524bc03a83 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -53,10 +53,10 @@ void ati_2d_blt(ATIVGAState *s)
>             s->vga.vbe_start_addr, surface_data(ds), surface_stride(ds),
>             surface_bits_per_pixel(ds),
>             (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
> -    unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> -                      s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
> -    unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> -                      s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
> +    unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? s->regs.dst_x
> +                        : (s->regs.dst_x + 1 - s->regs.dst_width) & 0x3fff);
> +    unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? s->regs.dst_y
> +                        : (s->regs.dst_y + 1 - s->regs.dst_height) & 0x3fff);

I don't think that's the correct fix. VRAM size is settable via a property 
so we should check if the resulting values are inside VRAM for which a 
simple mask may not be enough. Rather, check the calculation in the if 
with the error that says "blt outside vram not implemented".

The s->regs.[src|dst]_[xy] values should not be over 0x3fff because we 
mask them on register write in ati.c and here [src|dst]_[x|y] local 
variables are declared unsigned so negative values come out as large 
integers that should be caught by the checks below as being over VRAM end 
but those checks may have an off by one error or some other mistake. Do 
you have a reproducer and did you test if this fixes the crash or more 
info on how this overflows?

Regards,
BALATON Zoltan

>     int bpp = ati_bpp_from_datatype(s);
>     if (!bpp) {
>         qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
> @@ -91,9 +91,9 @@ void ati_2d_blt(ATIVGAState *s)
>     case ROP3_SRCCOPY:
>     {
>         unsigned src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> -                       s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
> +           s->regs.src_x : (s->regs.src_x + 1 - s->regs.dst_width) & 0x3fff);
>         unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> -                       s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
> +           s->regs.src_y : (s->regs.src_y + 1 - s->regs.dst_height) & 0x3fff);
>         int src_stride = DEFAULT_CNTL ?
>                          s->regs.src_pitch : s->regs.default_pitch;
>         if (!src_stride) {