| Message ID | 20200604090830.33885-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ati-vga: check mm_index before recursive call (CVE-2020-13800) | expand |
> + } 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
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: