From: Prasad J Pandit <pjp@fedoraproject.org>

While accessing VGA registers via ati_mm_read/write routines,
a guest may set 's->regs.mm_index' such that it leads to infinite
recursion. Check mm_index value to avoid such recursion. Log an
error message for wrong values.

Reported-by: Ren Ding <rding@gatech.edu>
Reported-by: Hanqing Zhao <hanqing@gatech.edu>
Reported-by: Yi Ren <c4tren@gmail.com>
Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/display/ati.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Update v3: log error message
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00827.html

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 60c5f246fd..98c21e7bd5 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -285,8 +285,11 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
             if (idx <= s->vga.vram_size - size) {
                 val = ldn_le_p(s->vga.vram_ptr + idx, size);
             }
-        } else {
+        } else if (s->regs.mm_index > MM_DATA + 3) {
             val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "ati_mm_read: mm_index too small: %u\n", s->regs.mm_index);
         }
         break;
     case BIOS_0_SCRATCH ... BUS_CNTL - 1:
@@ -523,8 +526,11 @@ static void ati_mm_write(void *opaque, hwaddr addr,
             if (idx <= s->vga.vram_size - size) {
                 stn_le_p(s->vga.vram_ptr + idx, size, data);
             }
-        } else {
+        } else if (s->regs.mm_index > MM_DATA + 3) {
             ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "ati_mm_write: mm_index too small: %u\n", s->regs.mm_index);
         }
         break;
     case BIOS_0_SCRATCH ... BUS_CNTL - 1:
-- 
2.26.2

> +        } else if (s->regs.mm_index > MM_DATA + 3) {
>              val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);

MM_INDEX is 0
MM_DATA  is 4
"normal" registers start at 8.

So we want allow indirect access for offset 8 and above and deny offsets
0-7.  mm_index is interpreted with an offset, see "- MM_DATA" in the
call above.

Not clear to me why this offset is 4, that doesn't make sense to me.
I'd expect either no offset or offset being 8.  BALATON, can you
double-check that with the specs?

Assuming offset 4 is correct we must require mm_index being larger than
MM_DATA + MM_DATA + 3 ( == 11) to compensate for the offset.

take care,
  Gerd

On Thu, 4 Jun 2020, Gerd Hoffmann wrote:
>> +        } else if (s->regs.mm_index > MM_DATA + 3) {
>>              val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);
>
> MM_INDEX is 0
> MM_DATA  is 4
> "normal" registers start at 8.
>
> So we want allow indirect access for offset 8 and above and deny offsets
> 0-7.  mm_index is interpreted with an offset, see "- MM_DATA" in the
> call above.

MM_INDEX is the register to read, addr - MM_DATA is an offset for 
unaligned access (when guest reads MM_DATA + 1, size=2 then we need to 
return regs[valueof(MM_INDEX) + 1], size=2.

> Not clear to me why this offset is 4, that doesn't make sense to me.
> I'd expect either no offset or offset being 8.  BALATON, can you
> double-check that with the specs?

We check that valueof(MM_INDEX) is at least MM_DATA + 4 = 8

> Assuming offset 4 is correct we must require mm_index being larger than
> MM_DATA + MM_DATA + 3 ( == 11) to compensate for the offset.

I don't get this, I think you're confusing value of MM_INDEX and offset of 
reading MM_DATA reg itself which together define what register is read 
with what offset during recursion. We don't want to recurse if clients 
tries to access either MM_INDEX or MM_DATA via these regs themselves to 
avoid infinite recursion.

Regards,
BALATON Zoltan

On Thu, Jun 04, 2020 at 03:59:05PM +0200, BALATON Zoltan wrote:
> On Thu, 4 Jun 2020, Gerd Hoffmann wrote:
> > > +        } else if (s->regs.mm_index > MM_DATA + 3) {
> > >              val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);
> > 
> > MM_INDEX is 0
> > MM_DATA  is 4
> > "normal" registers start at 8.
> > 
> > So we want allow indirect access for offset 8 and above and deny offsets
> > 0-7.  mm_index is interpreted with an offset, see "- MM_DATA" in the
> > call above.
> 
> MM_INDEX is the register to read, addr - MM_DATA is an offset for unaligned
> access (when guest reads MM_DATA + 1, size=2 then we need to return
> regs[valueof(MM_INDEX) + 1], size=2.

Ah, right.  Scratch my comment then, patch is correct.
Added to vga queue.

thanks,
  Gerd