[PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

When building QEMU with DEBUG_ATI defined then running with
'-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
we get:

  ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
  ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
  ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
  ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
  ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
  ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
  ati_mm_write 4 0x1420 DST_Y <- 0x3fff
  ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
  ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
  ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
  ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
  ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
  Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
  (gdb) bt
  #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
  #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
  #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
  #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
  #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492

Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
the local dst_x and dst_y which adjust the (x, y) coordinates
depending on the direction in the SRCCOPY ROP3 operation, but
forgot to address the same issue for the PATCOPY, BLACKNESS and
WHITENESS operations, which also call pixman_fill().

Fix that now by using the adjusted coordinates in the pixman_fill
call, and update the related debug printf().

Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/ati_2d.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 4dc10ea7952..692bec91de4 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
     DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
             s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
             s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
-            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
+            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
             s->regs.dst_width, s->regs.dst_height,
             (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
             (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
@@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
         dst_stride /= sizeof(uint32_t);
         DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
                 dst_bits, dst_stride, bpp,
-                s->regs.dst_x, s->regs.dst_y,
+                dst_x, dst_y,
                 s->regs.dst_width, s->regs.dst_height,
                 filler);
         pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
-                    s->regs.dst_x, s->regs.dst_y,
+                    dst_x, dst_y,
                     s->regs.dst_width, s->regs.dst_height,
                     filler);
         if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
-- 
2.31.1

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Mauro Matteo Cascella]

On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> When building QEMU with DEBUG_ATI defined then running with
> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
> we get:
>
>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>   (gdb) bt
>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
>
> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
> the local dst_x and dst_y which adjust the (x, y) coordinates
> depending on the direction in the SRCCOPY ROP3 operation, but
> forgot to address the same issue for the PATCOPY, BLACKNESS and
> WHITENESS operations, which also call pixman_fill().
>
> Fix that now by using the adjusted coordinates in the pixman_fill
> call, and update the related debug printf().
>
> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/display/ati_2d.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 4dc10ea7952..692bec91de4 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
>              s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
>              s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
>              s->regs.dst_width, s->regs.dst_height,
>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
>          dst_stride /= sizeof(uint32_t);
>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
>                  dst_bits, dst_stride, bpp,
> -                s->regs.dst_x, s->regs.dst_y,
> +                dst_x, dst_y,
>                  s->regs.dst_width, s->regs.dst_height,
>                  filler);
>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
> -                    s->regs.dst_x, s->regs.dst_y,
> +                    dst_x, dst_y,
>                      s->regs.dst_width, s->regs.dst_height,
>                      filler);
>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
> --
> 2.31.1
>

Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>

Thanks.
-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

(Forgot to Cc Alex for eventual reproducer)

On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
> On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> When building QEMU with DEBUG_ATI defined then running with
>> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
>> we get:
>>
>>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
>>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>>   (gdb) bt
>>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
>>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
>>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
>>
>> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
>> the local dst_x and dst_y which adjust the (x, y) coordinates
>> depending on the direction in the SRCCOPY ROP3 operation, but
>> forgot to address the same issue for the PATCOPY, BLACKNESS and
>> WHITENESS operations, which also call pixman_fill().
>>
>> Fix that now by using the adjusted coordinates in the pixman_fill
>> call, and update the related debug printf().
>>
>> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
>> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/display/ati_2d.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>> index 4dc10ea7952..692bec91de4 100644
>> --- a/hw/display/ati_2d.c
>> +++ b/hw/display/ati_2d.c
>> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
>>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
>>              s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
>>              s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
>> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
>> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
>>              s->regs.dst_width, s->regs.dst_height,
>>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
>>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
>> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
>>          dst_stride /= sizeof(uint32_t);
>>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
>>                  dst_bits, dst_stride, bpp,
>> -                s->regs.dst_x, s->regs.dst_y,
>> +                dst_x, dst_y,
>>                  s->regs.dst_width, s->regs.dst_height,
>>                  filler);
>>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
>> -                    s->regs.dst_x, s->regs.dst_y,
>> +                    dst_x, dst_y,
>>                      s->regs.dst_width, s->regs.dst_height,
>>                      filler);
>>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
>> --
>> 2.31.1
>>
> 
> Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>

Thanks. I wouldn't be surprise if we get another CVE in this code /
file / function ASAP this patch get merged... The code calls for a
rewrite, as per this function comment in its header:

void ati_2d_blt(ATIVGAState *s)
{
    /* FIXME it is probably more complex than this and may need to be */
    /* rewritten but for now as a start just to get some output: */

Regards,

Phil.

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Alexander Bulekov]

On 210906 2019, Philippe Mathieu-Daudé wrote:
> (Forgot to Cc Alex for eventual reproducer)

Here you go. Should we be fuzzing this on OSS-Fuzz?

============= 8< =============

/*
 * cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
 * 512M -device ati-vga,romfile= -nodefaults -qtest /dev/null -qtest stdio
 * outl 0xcf8 0x80001018
 * outl 0xcfc 0xe1000000
 * outl 0xcf8 0x80001004
 * outw 0xcfc 0x02
 * write 0xe10016c4 0x1 0x04
 * write 0xe10016e4 0x1 0x58
 * write 0xe1001438 0x4 0x0400001a
 * write 0xe100143c 0x4 0x01000015
 * EOF
 */
static void test_fuzz(void)
{
    QTestState *s = qtest_init(
        "-display none , -m 512M -device ati-vga,romfile= -nodefaults -qtest /dev/null");
    qtest_outl(s, 0xcf8, 0x80001018);
    qtest_outl(s, 0xcfc, 0xe1000000);
    qtest_outl(s, 0xcf8, 0x80001004);
    qtest_outw(s, 0xcfc, 0x02);
    qtest_bufwrite(s, 0xe10016c4, "\x04", 0x1);
    qtest_bufwrite(s, 0xe10016e4, "\x58", 0x1);
    qtest_bufwrite(s, 0xe1001438, "\x04\x00\x00\x1a", 0x4);
    qtest_bufwrite(s, 0xe100143c, "\x01\x00\x00\x15", 0x4);
    qtest_quit(s);
}

============= >8 =============
-Alex

> 
> On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
> > On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>
> >> When building QEMU with DEBUG_ATI defined then running with
> >> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
> >> we get:
> >>
> >>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
> >>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
> >>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
> >>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
> >>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
> >>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
> >>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
> >>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
> >>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
> >>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
> >>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
> >>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
> >>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
> >>   (gdb) bt
> >>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
> >>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
> >>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
> >>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
> >>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
> >>
> >> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
> >> the local dst_x and dst_y which adjust the (x, y) coordinates
> >> depending on the direction in the SRCCOPY ROP3 operation, but
> >> forgot to address the same issue for the PATCOPY, BLACKNESS and
> >> WHITENESS operations, which also call pixman_fill().
> >>
> >> Fix that now by using the adjusted coordinates in the pixman_fill
> >> call, and update the related debug printf().
> >>
> >> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
> >> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  hw/display/ati_2d.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> >> index 4dc10ea7952..692bec91de4 100644
> >> --- a/hw/display/ati_2d.c
> >> +++ b/hw/display/ati_2d.c
> >> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
> >>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
> >>              s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
> >>              s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
> >> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
> >> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
> >>              s->regs.dst_width, s->regs.dst_height,
> >>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
> >>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
> >> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
> >>          dst_stride /= sizeof(uint32_t);
> >>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
> >>                  dst_bits, dst_stride, bpp,
> >> -                s->regs.dst_x, s->regs.dst_y,
> >> +                dst_x, dst_y,
> >>                  s->regs.dst_width, s->regs.dst_height,
> >>                  filler);
> >>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
> >> -                    s->regs.dst_x, s->regs.dst_y,
> >> +                    dst_x, dst_y,
> >>                      s->regs.dst_width, s->regs.dst_height,
> >>                      filler);
> >>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
> >> --
> >> 2.31.1
> >>
> > 
> > Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
> 
> Thanks. I wouldn't be surprise if we get another CVE in this code /
> file / function ASAP this patch get merged... The code calls for a
> rewrite, as per this function comment in its header:
> 
> void ati_2d_blt(ATIVGAState *s)
> {
>     /* FIXME it is probably more complex than this and may need to be */
>     /* rewritten but for now as a start just to get some output: */
> 
> Regards,
> 
> Phil.
> 
> 

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5875 bytes --]

On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
> (Forgot to Cc Alex for eventual reproducer)
>
> On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
>> On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> When building QEMU with DEBUG_ATI defined then running with
>>> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
>>> we get:
>>>
>>>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>>>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>>>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>>>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>>>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>>>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>>>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>>>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>>>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>>>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
>>>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>>>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>>>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>>>   (gdb) bt
>>>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
>>>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>>>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
>>>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>>>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
>>>
>>> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
>>> the local dst_x and dst_y which adjust the (x, y) coordinates
>>> depending on the direction in the SRCCOPY ROP3 operation, but
>>> forgot to address the same issue for the PATCOPY, BLACKNESS and
>>> WHITENESS operations, which also call pixman_fill().
>>>
>>> Fix that now by using the adjusted coordinates in the pixman_fill
>>> call, and update the related debug printf().
>>>
>>> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
>>> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/display/ati_2d.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>> index 4dc10ea7952..692bec91de4 100644
>>> --- a/hw/display/ati_2d.c
>>> +++ b/hw/display/ati_2d.c
>>> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
>>>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
>>>              s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
>>>              s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
>>> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
>>> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
>>>              s->regs.dst_width, s->regs.dst_height,
>>>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
>>>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
>>> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
>>>          dst_stride /= sizeof(uint32_t);
>>>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
>>>                  dst_bits, dst_stride, bpp,
>>> -                s->regs.dst_x, s->regs.dst_y,
>>> +                dst_x, dst_y,
>>>                  s->regs.dst_width, s->regs.dst_height,
>>>                  filler);
>>>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
>>> -                    s->regs.dst_x, s->regs.dst_y,
>>> +                    dst_x, dst_y,
>>>                      s->regs.dst_width, s->regs.dst_height,
>>>                      filler);
>>>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
>>> --
>>> 2.31.1
>>>
>>
>> Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
>
> Thanks. I wouldn't be surprise if we get another CVE in this code /
> file / function ASAP this patch get merged... The code calls for a
> rewrite, as per this function comment in its header:
>
> void ati_2d_blt(ATIVGAState *s)
> {
>    /* FIXME it is probably more complex than this and may need to be */
>    /* rewritten but for now as a start just to get some output: */

It's also broken currently since the previous CVE fixes when I've tried to 
change it to only use unsigned values to avoid underflows and get away 
with only checking for overflows which simplifies it a bit. But turns out 
that's wrong, the hardware does allow negative values and while most 
drivers don't use that (such as Linux and MorphOS, so they still work), at 
least Solaris driver does and it produces broken picture now once X 
starts. (This can be reproduced with Solaris 10 x86 iso, but Solaris also 
needs more features to be implemented to make it work so fixing this alone 
is not enough to get past the first screen, text will be still missing.) 
To fix this we will need to revert to signed values and check for both 
over and underflow. I planned to try that eventually but haven't yet got 
around to it.

I don't think assigning a CVE to a bug that is in an experimental and 
largely unused part and happens when one enables debug code really worth 
the hassle, this could be handled as a normal bug. As long as the proposed 
fix does not break MorphOS I'm OK with it as probably that's the only 
useful case for ati-vga currently (and maybe booting Linux on pegasos2) 
but these are hardly security critical. I don't think anybody would use it 
for anything else at least there were no contributions or reports so far. 
Have you tested if MorphOS still works as described at 
http://zero.eik.bme.hu/~balaton/qemu/amiga/ ?

Regards,
BALATON Zoltan

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

On 9/6/21 9:52 PM, BALATON Zoltan wrote:
> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
>> (Forgot to Cc Alex for eventual reproducer)
>>
>> On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
>>> On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé
>>> <philmd@redhat.com> wrote:
>>>>
>>>> When building QEMU with DEBUG_ATI defined then running with
>>>> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
>>>> we get:
>>>>
>>>>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>>>>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>>>>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>>>>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>>>>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>>>>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>>>>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>>>>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>>>>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>>>>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800
>>>> stride:2560 bpp:32 rop:0xff
>>>>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>>>>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8,
>>>> x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>>>>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation
>>>> fault.
>>>>   (gdb) bt
>>>>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at
>>>> /lib64/libpixman-1.so.0
>>>>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>>>>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at
>>>> hw/display/ati_2d.c:196
>>>>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800,
>>>> addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>>>>   #4  0x0000555558b90ec4 in memory_region_write_accessor
>>>> (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at
>>>> softmmu/memory.c:492
>>>>
>>>> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
>>>> the local dst_x and dst_y which adjust the (x, y) coordinates
>>>> depending on the direction in the SRCCOPY ROP3 operation, but
>>>> forgot to address the same issue for the PATCOPY, BLACKNESS and
>>>> WHITENESS operations, which also call pixman_fill().
>>>>
>>>> Fix that now by using the adjusted coordinates in the pixman_fill
>>>> call, and update the related debug printf().
>>>>
>>>> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
>>>> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/display/ati_2d.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>>> index 4dc10ea7952..692bec91de4 100644
>>>> --- a/hw/display/ati_2d.c
>>>> +++ b/hw/display/ati_2d.c
>>>> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
>>>>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
>>>>              s->regs.src_offset, s->regs.dst_offset,
>>>> s->regs.default_offset,
>>>>              s->regs.src_pitch, s->regs.dst_pitch,
>>>> s->regs.default_pitch,
>>>> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x,
>>>> s->regs.dst_y,
>>>> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
>>>>              s->regs.dst_width, s->regs.dst_height,
>>>>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
>>>>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
>>>> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
>>>>          dst_stride /= sizeof(uint32_t);
>>>>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
>>>>                  dst_bits, dst_stride, bpp,
>>>> -                s->regs.dst_x, s->regs.dst_y,
>>>> +                dst_x, dst_y,
>>>>                  s->regs.dst_width, s->regs.dst_height,
>>>>                  filler);
>>>>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
>>>> -                    s->regs.dst_x, s->regs.dst_y,
>>>> +                    dst_x, dst_y,
>>>>                      s->regs.dst_width, s->regs.dst_height,
>>>>                      filler);
>>>>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
>>>> -- 
>>>> 2.31.1
>>>>
>>>
>>> Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>
>> Thanks. I wouldn't be surprise if we get another CVE in this code /
>> file / function ASAP this patch get merged... The code calls for a
>> rewrite, as per this function comment in its header:
>>
>> void ati_2d_blt(ATIVGAState *s)
>> {
>>    /* FIXME it is probably more complex than this and may need to be */
>>    /* rewritten but for now as a start just to get some output: */
> 
> It's also broken currently since the previous CVE fixes when I've tried
> to change it to only use unsigned values to avoid underflows and get
> away with only checking for overflows which simplifies it a bit. But
> turns out that's wrong, the hardware does allow negative values and
> while most drivers don't use that (such as Linux and MorphOS, so they
> still work), at least Solaris driver does and it produces broken picture
> now once X starts. (This can be reproduced with Solaris 10 x86 iso, but

It would be useful to dump these Solaris I/O accesses to use them as
regression tests.

> Solaris also needs more features to be implemented to make it work so
> fixing this alone is not enough to get past the first screen, text will
> be still missing.) To fix this we will need to revert to signed values
> and check for both over and underflow. I planned to try that eventually
> but haven't yet got around to it.
> 
> I don't think assigning a CVE to a bug that is in an experimental and
> largely unused part and happens when one enables debug code really worth
> the hassle, this could be handled as a normal bug. As long as the

CVE assignment can happens outside of QEMU community, we try to make it
clear what is the "security boundary" but researchers filling CVEs
might not understand it well.

> proposed fix does not break MorphOS I'm OK with it as probably that's
> the only useful case for ati-vga currently (and maybe booting Linux on
> pegasos2) but these are hardly security critical. I don't think anybody
> would use it for anything else at least there were no contributions or
> reports so far. Have you tested if MorphOS still works as described at
> http://zero.eik.bme.hu/~balaton/qemu/amiga/ ?

Nop.

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

On 9/6/21 9:19 PM, Alexander Bulekov wrote:
> On 210906 2019, Philippe Mathieu-Daudé wrote:
>> (Forgot to Cc Alex for eventual reproducer)
> 
> Here you go. Should we be fuzzing this on OSS-Fuzz?

Should we limit what we fuzz there? All bugs found so far
have been useful. The issues fixed improved QEMU quality, and
the ones we couldn't fix (yet, mostly due to lack of time)
helped us understand design flaw and we are wondering how to
address them.

> ============= 8< =============
> 
> /*
>  * cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
>  * 512M -device ati-vga,romfile= -nodefaults -qtest /dev/null -qtest stdio
>  * outl 0xcf8 0x80001018
>  * outl 0xcfc 0xe1000000
>  * outl 0xcf8 0x80001004
>  * outw 0xcfc 0x02

Thanks, this was the missing part :>

>  * write 0xe10016c4 0x1 0x04
>  * write 0xe10016e4 0x1 0x58
>  * write 0xe1001438 0x4 0x0400001a
>  * write 0xe100143c 0x4 0x01000015
>  * EOF
>  */
> static void test_fuzz(void)
> {
>     QTestState *s = qtest_init(
>         "-display none , -m 512M -device ati-vga,romfile= -nodefaults -qtest /dev/null");
>     qtest_outl(s, 0xcf8, 0x80001018);
>     qtest_outl(s, 0xcfc, 0xe1000000);
>     qtest_outl(s, 0xcf8, 0x80001004);
>     qtest_outw(s, 0xcfc, 0x02);
>     qtest_bufwrite(s, 0xe10016c4, "\x04", 0x1);
>     qtest_bufwrite(s, 0xe10016e4, "\x58", 0x1);
>     qtest_bufwrite(s, 0xe1001438, "\x04\x00\x00\x1a", 0x4);
>     qtest_bufwrite(s, 0xe100143c, "\x01\x00\x00\x15", 0x4);
>     qtest_quit(s);
> }
> 
> ============= >8 =============
> -Alex
> 
>>
>> On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
>>> On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>> When building QEMU with DEBUG_ATI defined then running with
>>>> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
>>>> we get:
>>>>
>>>>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>>>>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>>>>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>>>>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>>>>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>>>>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>>>>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>>>>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>>>>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>>>>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
>>>>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>>>>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>>>>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>>>>   (gdb) bt
>>>>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
>>>>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>>>>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
>>>>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>>>>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
>>>>
>>>> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
>>>> the local dst_x and dst_y which adjust the (x, y) coordinates
>>>> depending on the direction in the SRCCOPY ROP3 operation, but
>>>> forgot to address the same issue for the PATCOPY, BLACKNESS and
>>>> WHITENESS operations, which also call pixman_fill().
>>>>
>>>> Fix that now by using the adjusted coordinates in the pixman_fill
>>>> call, and update the related debug printf().
>>>>
>>>> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
>>>> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/display/ati_2d.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>>> index 4dc10ea7952..692bec91de4 100644
>>>> --- a/hw/display/ati_2d.c
>>>> +++ b/hw/display/ati_2d.c
>>>> @@ -84,7 +84,7 @@ void ati_2d_blt(ATIVGAState *s)
>>>>      DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
>>>>              s->regs.src_offset, s->regs.dst_offset, s->regs.default_offset,
>>>>              s->regs.src_pitch, s->regs.dst_pitch, s->regs.default_pitch,
>>>> -            s->regs.src_x, s->regs.src_y, s->regs.dst_x, s->regs.dst_y,
>>>> +            s->regs.src_x, s->regs.src_y, dst_x, dst_y,
>>>>              s->regs.dst_width, s->regs.dst_height,
>>>>              (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ? '>' : '<'),
>>>>              (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ? 'v' : '^'));
>>>> @@ -180,11 +180,11 @@ void ati_2d_blt(ATIVGAState *s)
>>>>          dst_stride /= sizeof(uint32_t);
>>>>          DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
>>>>                  dst_bits, dst_stride, bpp,
>>>> -                s->regs.dst_x, s->regs.dst_y,
>>>> +                dst_x, dst_y,
>>>>                  s->regs.dst_width, s->regs.dst_height,
>>>>                  filler);
>>>>          pixman_fill((uint32_t *)dst_bits, dst_stride, bpp,
>>>> -                    s->regs.dst_x, s->regs.dst_y,
>>>> +                    dst_x, dst_y,
>>>>                      s->regs.dst_width, s->regs.dst_height,
>>>>                      filler);
>>>>          if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
>>>> --
>>>> 2.31.1
>>>>
>>>
>>> Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>
>> Thanks. I wouldn't be surprise if we get another CVE in this code /
>> file / function ASAP this patch get merged... The code calls for a
>> rewrite, as per this function comment in its header:
>>
>> void ati_2d_blt(ATIVGAState *s)
>> {
>>     /* FIXME it is probably more complex than this and may need to be */
>>     /* rewritten but for now as a start just to get some output: */
>>
>> Regards,
>>
>> Phil.
>>
>>
> 

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

On 9/6/21 5:31 PM, Philippe Mathieu-Daudé wrote:
> When building QEMU with DEBUG_ATI defined then running with
> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
> we get:
> 
>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>   (gdb) bt
>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
> 
> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
> the local dst_x and dst_y which adjust the (x, y) coordinates
> depending on the direction in the SRCCOPY ROP3 operation, but
> forgot to address the same issue for the PATCOPY, BLACKNESS and
> WHITENESS operations, which also call pixman_fill().
> 
> Fix that now by using the adjusted coordinates in the pixman_fill
> call, and update the related debug printf().
> 

Forgot here:

Cc: qemu-stable@nongnu.org

> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/display/ati_2d.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

On 9/7/21 7:38 AM, Philippe Mathieu-Daudé wrote:
> On 9/6/21 9:52 PM, BALATON Zoltan wrote:
>> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
>>> (Forgot to Cc Alex for eventual reproducer)
>>>
>>> On 9/6/21 6:44 PM, Mauro Matteo Cascella wrote:
>>>> On Mon, Sep 6, 2021 at 5:31 PM Philippe Mathieu-Daudé
>>>> <philmd@redhat.com> wrote:
>>>>>
>>>>> When building QEMU with DEBUG_ATI defined then running with
>>>>> '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
>>>>> we get:
>>>>>
>>>>>   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
>>>>>   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
>>>>>   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
>>>>>   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
>>>>>   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
>>>>>   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
>>>>>   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
>>>>>   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
>>>>>   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
>>>>>   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800
>>>>> stride:2560 bpp:32 rop:0xff
>>>>>   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
>>>>>   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8,
>>>>> x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
>>>>>   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation
>>>>> fault.
>>>>>   (gdb) bt
>>>>>   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at
>>>>> /lib64/libpixman-1.so.0
>>>>>   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
>>>>>   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at
>>>>> hw/display/ati_2d.c:196
>>>>>   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800,
>>>>> addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
>>>>>   #4  0x0000555558b90ec4 in memory_region_write_accessor
>>>>> (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at
>>>>> softmmu/memory.c:492
>>>>>
>>>>> Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
>>>>> the local dst_x and dst_y which adjust the (x, y) coordinates
>>>>> depending on the direction in the SRCCOPY ROP3 operation, but
>>>>> forgot to address the same issue for the PATCOPY, BLACKNESS and
>>>>> WHITENESS operations, which also call pixman_fill().
>>>>>
>>>>> Fix that now by using the adjusted coordinates in the pixman_fill
>>>>> call, and update the related debug printf().
>>>>>
>>>>> Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
>>>>> Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>> ---
>>>>>  hw/display/ati_2d.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)

>> I don't think assigning a CVE to a bug that is in an experimental and
>> largely unused part and happens when one enables debug code really worth
>> the hassle, this could be handled as a normal bug. As long as the
> 
> CVE assignment can happens outside of QEMU community, we try to make it
> clear what is the "security boundary" but researchers filling CVEs
> might not understand it well.

BTW see commit b317006a3f1 ("docs/secure-coding-practices: Describe how
to use 'null-co' block driver") which is related to your suggestion.

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Mauro Matteo Cascella]

On Tue, Sep 7, 2021 at 8:22 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 9/7/21 7:38 AM, Philippe Mathieu-Daudé wrote:
> > On 9/6/21 9:52 PM, BALATON Zoltan wrote:
> >> I don't think assigning a CVE to a bug that is in an experimental and
> >> largely unused part and happens when one enables debug code really worth
> >> the hassle, this could be handled as a normal bug. As long as the
> >
> > CVE assignment can happens outside of QEMU community, we try to make it
> > clear what is the "security boundary" but researchers filling CVEs
> > might not understand it well.
>
> BTW see commit b317006a3f1 ("docs/secure-coding-practices: Describe how
> to use 'null-co' block driver") which is related to your suggestion.

I agree we can avoid assigning CVEs to ati-vga and similar
experimental devices that are not intended to be used in production,
even if they fall under the virtualization use case. Maybe we can
improve the documentation
(https://qemu-project.gitlab.io/qemu/system/security.html) to clearly
state that some devices are not security supported? Would it be
possible/feasible to get a list of such devices? Or maybe the other
way around, document the list of devices that are undeniably security
supported (e.g., virtio*, *hci, e1000, etc.)?


--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Philippe Mathieu-Daudé]

On 9/9/21 11:16 AM, Mauro Matteo Cascella wrote:
> On Tue, Sep 7, 2021 at 8:22 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 9/7/21 7:38 AM, Philippe Mathieu-Daudé wrote:
>>> On 9/6/21 9:52 PM, BALATON Zoltan wrote:
>>>> I don't think assigning a CVE to a bug that is in an experimental and
>>>> largely unused part and happens when one enables debug code really worth
>>>> the hassle, this could be handled as a normal bug. As long as the
>>>
>>> CVE assignment can happens outside of QEMU community, we try to make it
>>> clear what is the "security boundary" but researchers filling CVEs
>>> might not understand it well.
>>
>> BTW see commit b317006a3f1 ("docs/secure-coding-practices: Describe how
>> to use 'null-co' block driver") which is related to your suggestion.
> 
> I agree we can avoid assigning CVEs to ati-vga and similar
> experimental devices that are not intended to be used in production,
> even if they fall under the virtualization use case. Maybe we can
> improve the documentation
> (https://qemu-project.gitlab.io/qemu/system/security.html) to clearly
> state that some devices are not security supported? Would it be
> possible/feasible to get a list of such devices? Or maybe the other
> way around, document the list of devices that are undeniably security
> supported (e.g., virtio*, *hci, e1000, etc.)?

I just posted a suggestion as RFC but forgot to Cc you:
"security: Introduce qemu_security_policy_taint() API"
https://lore.kernel.org/qemu-devel/20210908232024.2399215-1-philmd@redhat.com/
In particular for the ati-vga device:
https://lore.kernel.org/qemu-devel/20210908232024.2399215-8-philmd@redhat.com/

Re: [PATCH] hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638)

[Qiang Liu]

Hi all,

I found this patch is still not merged. Should we merge this and close
this issue?

Best,
Qiang

On Tue, Sep 7, 2021 at 2:20 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 9/6/21 5:31 PM, Philippe Mathieu-Daudé wrote:
> > When building QEMU with DEBUG_ATI defined then running with
> > '-device ati-vga,romfile="" -d unimp,guest_errors -trace ati\*'
> > we get:
> >
> >   ati_mm_write 4 0x16c0 DP_CNTL <- 0x1
> >   ati_mm_write 4 0x146c DP_GUI_MASTER_CNTL <- 0x2
> >   ati_mm_write 4 0x16c8 DP_MIX <- 0xff0000
> >   ati_mm_write 4 0x16c4 DP_DATATYPE <- 0x2
> >   ati_mm_write 4 0x224 CRTC_OFFSET <- 0x0
> >   ati_mm_write 4 0x142c DST_PITCH_OFFSET <- 0xfe00000
> >   ati_mm_write 4 0x1420 DST_Y <- 0x3fff
> >   ati_mm_write 4 0x1410 DST_HEIGHT <- 0x3fff
> >   ati_mm_write 4 0x1588 DST_WIDTH_X <- 0x3fff3fff
> >   ati_2d_blt: vram:0x7fff5fa00000 addr:0 ds:0x7fff61273800 stride:2560 bpp:32 rop:0xff
> >   ati_2d_blt: 0 0 0, 0 127 0, (0,0) -> (16383,16383) 16383x16383 > ^
> >   ati_2d_blt: pixman_fill(dst:0x7fff5fa00000, stride:254, bpp:8, x:16383, y:16383, w:16383, h:16383, xor:0xff000000)
> >   Thread 3 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
> >   (gdb) bt
> >   #0  0x00007ffff7f62ce0 in sse2_fill.lto_priv () at /lib64/libpixman-1.so.0
> >   #1  0x00007ffff7f09278 in pixman_fill () at /lib64/libpixman-1.so.0
> >   #2  0x0000555557b5a9af in ati_2d_blt (s=0x631000028800) at hw/display/ati_2d.c:196
> >   #3  0x0000555557b4b5a2 in ati_mm_write (opaque=0x631000028800, addr=5512, data=1073692671, size=4) at hw/display/ati.c:843
> >   #4  0x0000555558b90ec4 in memory_region_write_accessor (mr=0x631000039cc0, addr=5512, ..., size=4, ...) at softmmu/memory.c:492
> >
> > Commit 584acf34cb0 ("ati-vga: Fix reverse bit blts") introduced
> > the local dst_x and dst_y which adjust the (x, y) coordinates
> > depending on the direction in the SRCCOPY ROP3 operation, but
> > forgot to address the same issue for the PATCOPY, BLACKNESS and
> > WHITENESS operations, which also call pixman_fill().
> >
> > Fix that now by using the adjusted coordinates in the pixman_fill
> > call, and update the related debug printf().
> >
>
> Forgot here:
>
> Cc: qemu-stable@nongnu.org
>
> > Reported-by: Qiang Liu <qiangliu@zju.edu.cn>
> > Fixes: 584acf34cb0 ("ati-vga: Fix reverse bit blts")
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  hw/display/ati_2d.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>