ati-vga: increment mm_index in ati_mm_read/write

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. Increment the mm_index value to avoid it.

Reported-by: Ren Ding <rding@gatech.edu>
Reported-by: Hanqing Zhao <hanqing@gatech.edu>
Reported-by: Yi Ren <c4tren@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/display/ati.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Wed, Jun 03, 2020 at 06:17:32PM +0530, P J P wrote:
> 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.

Lovely.

> Increment the mm_index value to avoid it.

Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
before calling ati_mm_read/ati_mm_write?

cheers,
  Gerd

On Wed, 3 Jun 2020, Gerd Hoffmann wrote:
> On Wed, Jun 03, 2020 at 06:17:32PM +0530, P J P wrote:
>> 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.
>
> Lovely.
>
>> Increment the mm_index value to avoid it.
>
> Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
> before calling ati_mm_read/ati_mm_write?

I haven't found any mention in any docs that say MM_INDEX should auto 
increment so unless this is proven to do that on real hardware I also 
think forbiding indexed access to index registers should be enough.

Regards,
BALATON Zoltan

+-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
| Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
| before calling ati_mm_read/ati_mm_write?

  if (s->regs.mm_index & BIT(31)) {
     ...
  } else {
     ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
  }

Exit condition for recursion is to set (mm_index & BIT(31)), so recursion 
would continue even with non-zero values I think.

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

On Wed, Jun 03, 2020 at 08:05:50PM +0530, P J P wrote:
> +-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
> | Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
> | before calling ati_mm_read/ati_mm_write?
> 
>   if (s->regs.mm_index & BIT(31)) {
>      ...
>   } else {

} else if (s->regs.mm_index > 3) {

>      ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>   }
> 
> Exit condition for recursion is to set (mm_index & BIT(31)), so recursion 
> would continue even with non-zero values I think.

It's wrapped into a case switch for all the registers.  The code quoted
above is only entered for "addr >= MM_DATA && addr <= MM_DATA+3", so the
check should stop recursion.  A less strict "s->regs.mm_index > 0" may
recurse a few times but will not recurse endless either.

cheers,
  Gerd

On Wed, 3 Jun 2020, P J P wrote:
> +-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
> | Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
> | before calling ati_mm_read/ati_mm_write?
>
>  if (s->regs.mm_index & BIT(31)) {
>     ...
>  } else {
>     ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>  }
>
> Exit condition for recursion is to set (mm_index & BIT(31)), so recursion
> would continue even with non-zero values I think.

MM_INDEX and MM_DATA allows access to registers (or memory if BIT(31) is 
set) via only these two registers. This is called indexed access in docs. 
So it does not really make sense to allow access to these registers as 
well thus we can avoid infinite recursion. It's not meant to recurse until 
BIT(31) is set. I think you can do:

   if (s->regs.mm_index & BIT(31)) {
      ...
-  } else {
+  } else if (s->regs.mm_index >= BIOS_0_SCRATCH) {
      ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
   }

and do the same in ati_mm_read() as well.

Regards,
BALATON Zoltan

On Wed, 3 Jun 2020, BALATON Zoltan wrote:
> On Wed, 3 Jun 2020, P J P wrote:
>> +-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
>> | Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
>> | before calling ati_mm_read/ati_mm_write?
>>
>>  if (s->regs.mm_index & BIT(31)) {
>>     ...
>>  } else {
>>     ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>>  }
>> 
>> Exit condition for recursion is to set (mm_index & BIT(31)), so recursion
>> would continue even with non-zero values I think.
>
> MM_INDEX and MM_DATA allows access to registers (or memory if BIT(31) is set) 
> via only these two registers. This is called indexed access in docs. So it 
> does not really make sense to allow access to these registers as well thus we 
> can avoid infinite recursion. It's not meant to recurse until BIT(31) is set. 
> I think you can do:
>
>  if (s->regs.mm_index & BIT(31)) {
>     ...
> -  } else {
> +  } else if (s->regs.mm_index >= BIOS_0_SCRATCH) {

Actually this should be

+  } else if (s->regs.mm_index >= CLOCK_CNTL_INDEX) {

or even > MM_DATA + 3 may be best as that only refers to defines used in 
that case. So maybe

+  } else if (s->regs.mm_index > MM_DATA + 3) {

>     ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>  }
>
> and do the same in ati_mm_read() as well.
>
> Regards,
> BALATON Zoltan
>

+-- On Wed, 3 Jun 2020, BALATON Zoltan wrote --+
| or even > MM_DATA + 3 may be best as that only refers to defines used in 
| that case. So maybe
| 
| +  } else if (s->regs.mm_index > MM_DATA + 3) {
| >     ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
| >  }
| >
| > and do the same in ati_mm_read() as well.

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