[PATCH] ati: mask x y display parameter values

[PATCH] ati: mask x y display parameter values

[P J P]

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);
     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) {
-- 
2.26.2

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

[BALATON Zoltan via]

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) {
>

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

[P J P]

+-- On Sun, 18 Oct 2020, BALATON Zoltan wrote --+
| The s->regs.[src|dst]_[xy] values should not be over 0x3fff because we mask 
| them on register write in ati.c

  Yes, those register values are set to zero(0).

| 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

  As above register values are zero(0), following expression(s)

    dst_x = ... (s->regs.dst_x(=0) + 1 - s->regs.dst_width(=16383))
    dst_y = ... (s->regs.dst_y(=0) + 1 - s->regs.dst_height(=16383))

result in large unsigned values: 

  ati_2d_blt
    pixman_blt(0x7f03cbe00000, 0x7f03cbe00000, 131064, 131064, 32, 32,
       src_x=0, src_y=-16382, dst_x=0, dst_y=-16382, 16383, 16383)

Shown as negative values due to signed '%d' conversion.

| but those checks may have an off by one error or some other mistake.

    uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) * dst_stride >= end) {
        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
        return;
    }

* Above check does not seem to catch it.

* Does a check below look okay?
===
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 524bc03a83..b75acc7fda 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -54,9 +54,13 @@ void ati_2d_blt(ATIVGAState *s)
...
+    if (dst_x > 0x3fff || dst_y > 0x3fff) {
+        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
+        return;
+    }
...
+        if (src_x > 0x3fff || src_y > 0x3fff) {
+            qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
+            return;
+        }
===

* ati_2d_blt() routine looks complex. Maybe it can be divided in two halves.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[BALATON Zoltan via]

On Mon, 19 Oct 2020, P J P wrote:
> +-- On Sun, 18 Oct 2020, BALATON Zoltan wrote --+
> | The s->regs.[src|dst]_[xy] values should not be over 0x3fff because we mask
> | them on register write in ati.c
>
>  Yes, those register values are set to zero(0).
>
> | 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
>
>  As above register values are zero(0), following expression(s)
>
>    dst_x = ... (s->regs.dst_x(=0) + 1 - s->regs.dst_width(=16383))
>    dst_y = ... (s->regs.dst_y(=0) + 1 - s->regs.dst_height(=16383))
>
> result in large unsigned values:

So far so good, this should result in dst_bits or dst_bits + the changed 
region being way over vram end which the check below should catch or 
otherwise it should not crash even if results are wrong.

>  ati_2d_blt
>    pixman_blt(0x7f03cbe00000, 0x7f03cbe00000, 131064, 131064, 32, 32,
>       src_x=0, src_y=-16382, dst_x=0, dst_y=-16382, 16383, 16383)
>
> Shown as negative values due to signed '%d' conversion.

Checking the docs again I see that the allowed range of at least 
s->regs.[src|dst]_[xy] can actually be negative (-8192:8191) so we 
probably misintrepret these and I'll have to look at this again to see how 
to do this properly but not sure when can I get to that. If this is urgent 
to fix now we could add some more checks but I don't like masking because 
that can lead to wrong results truncating parameters that should be 
rejected.

> | but those checks may have an off by one error or some other mistake.
>
>    uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
>    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) * dst_stride >= end) {
>        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
>        return;
>    }
>
> * Above check does not seem to catch it.

And that's the real problem. Could it be it overflows? If not then there's 
some other problem with interpreting the values later or with this check.

> * Does a check below look okay?
> ===
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 524bc03a83..b75acc7fda 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -54,9 +54,13 @@ void ati_2d_blt(ATIVGAState *s)
> ...
> +    if (dst_x > 0x3fff || dst_y > 0x3fff) {
> +        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
> +        return;
> +    }
> ...
> +        if (src_x > 0x3fff || src_y > 0x3fff) {
> +            qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
> +            return;
> +        }
> ===

This seems redundant, maybe rather add additional term for src|dst_x|y to 
the already existing checks if their condition cannot be fixed to detect 
it properly.

> * ati_2d_blt() routine looks complex. Maybe it can be divided in two halves.

Yes, as noted in a comment at the beginning this may need to be rewritten 
when it gets even more complex adding more of the features the card has. 
Maybe splitting off bounds checking to separate functions that can be 
reused for both dst and src values could be done to make it a bit better. 
I hoped the checks are simple enough for now but seems they are not. I'm 
not sure I can finish this before the freeze so unless it's OK to patch 
this during soft freeze or leave it for onther release I'd be OK with a 
patch that enhances the existing checks for now to also check src|dst_x|y 
instead of truncating their value. That may break it anyway but it's 
already broken and at least the checks are at the same place that way.

Regards,
BALATON Zoltan

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

[P J P]

  Hi,

+-- On Mon, 19 Oct 2020, BALATON Zoltan wrote --+
| On Mon, 19 Oct 2020, P J P wrote:
| >    dst_x = ... (s->regs.dst_x(=0) + 1 - s->regs.dst_width(=16383))
| >    dst_y = ... (s->regs.dst_y(=0) + 1 - s->regs.dst_height(=16383))
| >
| >  ati_2d_blt
| >    pixman_blt(0x7f03cbe00000, 0x7f03cbe00000, 131064, 131064, 32, 32,
| >       src_x=0, src_y=-16382, dst_x=0, dst_y=-16382, 16383, 16383)
| >
| > Shown as negative values due to signed '%d' conversion.
| 
| Checking the docs again I see that the allowed range of at least
| s->regs.[src|dst]_[xy] can actually be negative (-8192:8191)

* But 'struct ATIVGARegs' declares these fields as 'uint32_t' type. Ie. no 
  negativeve values.

* I guess that range applies to src->regs.dst_[width|height] too? Considering 
  it being subtracted from s->regs.dst_[xy] values above.


| >    uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
| >    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) *
| >    dst_stride >= end) {
| >        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
| >        return;
| 
| ... Could it be it overflows?

* Yes, following expression

  dst_y(=4294950914(=-16382)) + s->regs.dst_height(=16383)) overflows to => 1

Ie. (dst_bits + dst_x(=0) + (1) * dst_stride >= end) returns false.


| maybe rather add additional term for src|dst_x|y to the already existing 
| checks if their condition cannot be fixed to detect it properly.
===
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 524bc03a83..5fa7362305 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -54,9 +54,9 @@ void ati_2d_blt(ATIVGAState *s)
...
-    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) *
-        dst_stride >= end) {
+    if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
+        || dst_bits + dst_x + (dst_y + s->regs.dst_height)
+         * dst_stride >= end) {
         qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
         return;
     }
...
-        if (src_bits >= end || src_bits + src_x +
-            (src_y + s->regs.dst_height) * src_stride >= end) {
+        if (src_x > 0x3fff || src_y > 0x3ff || src_bits >= end
+            || src_bits + src_x + (src_y + s->regs.dst_height)
+             * src_stride >= end) {
             qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
             return;
         }
===

* Does it look okay?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[BALATON Zoltan via]

Hello,

On Tue, 20 Oct 2020, P J P wrote:
> +-- On Mon, 19 Oct 2020, BALATON Zoltan wrote --+
> | On Mon, 19 Oct 2020, P J P wrote:
> | >    dst_x = ... (s->regs.dst_x(=0) + 1 - s->regs.dst_width(=16383))
> | >    dst_y = ... (s->regs.dst_y(=0) + 1 - s->regs.dst_height(=16383))
> | >
> | >  ati_2d_blt
> | >    pixman_blt(0x7f03cbe00000, 0x7f03cbe00000, 131064, 131064, 32, 32,
> | >       src_x=0, src_y=-16382, dst_x=0, dst_y=-16382, 16383, 16383)
> | >
> | > Shown as negative values due to signed '%d' conversion.
> |
> | Checking the docs again I see that the allowed range of at least
> | s->regs.[src|dst]_[xy] can actually be negative (-8192:8191)
>
> * But 'struct ATIVGARegs' declares these fields as 'uint32_t' type. Ie. no
>  negativeve values.

The card has 32 bit registers with values in them interpreted differently 
for different regs. For dst_x|y lower 14 bits can be set and value should 
be interpreted as -8192:8191 according to docs. I've got this wrong 
because all guests I've tried did not actually use negative values. The 
Solaris driver I was recently shown not to work may use that so I plan to 
look at it and fix it when I'll have time.

> * I guess that range applies to src->regs.dst_[width|height] too? Considering
>  it being subtracted from s->regs.dst_[xy] values above.

Docs aren't very clear on that but I think these cannot be negative so 
0:8191 is valid range because it mentions that also bits 0-13 (or maybe 
0-15, the docs have a lot of copy&paste errors) are valid but only 0-12 
are used for rectangles, 13-15 used only for trapezoids (which we don't 
emulate). The docs are really bad so we have to guess and see what guest 
drivers do most of the time.

> | >    uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
> | >    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) *
> | >    dst_stride >= end) {
> | >        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
> | >        return;
> |
> | ... Could it be it overflows?
>
> * Yes, following expression
>
>  dst_y(=4294950914(=-16382)) + s->regs.dst_height(=16383)) overflows to => 1
>
> Ie. (dst_bits + dst_x(=0) + (1) * dst_stride >= end) returns false.

So maybe we should cast something (like dst_stride) to uint64_t here to 
promote everything to 64 bit and prevent it overflowing which then should 
catch this as well?

> | maybe rather add additional term for src|dst_x|y to the already existing
> | checks if their condition cannot be fixed to detect it properly.
> ===
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 524bc03a83..5fa7362305 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -54,9 +54,9 @@ void ati_2d_blt(ATIVGAState *s)
> ...
> -    if (dst_bits >= end || dst_bits + dst_x + (dst_y + s->regs.dst_height) *
> -        dst_stride >= end) {
> +    if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
> +        || dst_bits + dst_x + (dst_y + s->regs.dst_height)
> +         * dst_stride >= end) {
>         qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
>         return;
>     }
> ...
> -        if (src_bits >= end || src_bits + src_x +
> -            (src_y + s->regs.dst_height) * src_stride >= end) {
> +        if (src_x > 0x3fff || src_y > 0x3ff || src_bits >= end
> +            || src_bits + src_x + (src_y + s->regs.dst_height)
> +             * src_stride >= end) {
>             qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
>             return;
>         }
> ===
>
> * Does it look okay?

I can live with that until I have a chance to rewrite it but are you sure 
this will catch all possible overflows with all vram sizes that can be set 
with vgamem_mb property?

Regards,
BALATON Zoltan

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

[P J P]

+-- On Tue, 20 Oct 2020, BALATON Zoltan wrote --+
| The card has 32 bit registers with values in them interpreted differently for
| different regs. For dst_x|y lower 14 bits can be set and value should be
| interpreted as -8192:8191 according to docs. I've got this wrong because all
| guests I've tried did not actually use negative values. The Solaris driver I
| was recently shown not to work may use that so I plan to look at it and fix it
| when I'll have time. 
... 
| Docs aren't very clear on that but I think these cannot be negative so 
| 0:8191 is valid range because it mentions that also bits 0-13 (or maybe 
| 0-15, the docs have a lot of copy&paste errors) are valid but only 0-12 are 
| used for rectangles, 13-15 used only for trapezoids (which we don't 
| emulate). The docs are really bad so we have to guess and see what guest 
| drivers do most of the time.

* I see. Are the docs available/accessible online?

| >  dst_y(=4294950914(=-16382)) + s->regs.dst_height(=16383)) overflows to => 1
| > Ie. (dst_bits + dst_x(=0) + (1) * dst_stride >= end) returns false.
| 
| So maybe we should cast something (like dst_stride) to uint64_t here to
| promote everything to 64 bit and prevent it overflowing which then should
| catch this as well?
... 
| > +    if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
| > +        || dst_bits + dst_x + (dst_y + s->regs.dst_height)
| > +         * dst_stride >= end) {
| > ...
| > +        if (src_x > 0x3fff || src_y > 0x3ff || src_bits >= end
| > +            || src_bits + src_x + (src_y + s->regs.dst_height)
| > +             * src_stride >= end) {
| >             qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
| 
| I can live with that until I have a chance to rewrite it but are you sure this
| will catch all possible overflows with all vram sizes that can be set with
| vgamem_mb property?

* Considering all fields are 'uint32_t' type; And majority of the values 
  s->regs.[src|dst]_[xy], s->regs.dst_height are masked with '0x3fff', it 
  should help to avoid overflows.

* Not sure about all vram sizes. What are possible/supported size options?

* Between casting expression to 64 bits & explicit src_[xy] > 0x3fff check, 
  I'd go with explicit check, as it's easy to follow.

  Will send a revised patch with src_[xy] > 0x3fff if it's okay with you.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D