On 6/4/20 1:49 PM, Michael S. Tsirkin wrote: > On Thu, Jun 04, 2020 at 04:25:24PM +0530, P J P wrote: >> From: Prasad J Pandit <p...@fedoraproject.org> >> >> While reading PCI configuration bytes, a guest may send an >> address towards the end of the configuration space. It may lead >> to an OOB access issue. Add check to ensure 'address + size' is >> within PCI configuration space. >> >> Reported-by: Ren Ding <rd...@gatech.edu> >> Reported-by: Hanqing Zhao <hanq...@gatech.edu> >> Reported-by: Yi Ren <c4t...@gmail.com> >> Suggested-by: BALATON Zoltan <bala...@eik.bme.hu> >> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> > > BTW, this only happens on unaligned accesses. > And the IO memory region in question does not set valid.unaligned > or .impl.unaligned. > > And the documentation says: > > - .valid.unaligned specifies that the *device being modelled* supports > unaligned accesses; if false, unaligned accesses will invoke the > appropriate bus or CPU specific behaviour. > > and > > - .impl.unaligned specifies that the *implementation* supports unaligned > accesses; if false, unaligned accesses will be emulated by two aligned > accesses. > > Is this then another case of a memory core bug which should have either > failed the access or split it?
Related: https://www.mail-archive.com/qemu-devel@nongnu.org/msg695362.html earlier comment: https://www.mail-archive.com/qemu-devel@nongnu.org/msg694805.html > >> --- >> hw/display/ati.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> Update v3: avoid modifying 'addr' variable >> -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00834.html >> >> diff --git a/hw/display/ati.c b/hw/display/ati.c >> index 67604e68de..b4d0fd88b7 100644 >> --- a/hw/display/ati.c >> +++ b/hw/display/ati.c >> @@ -387,7 +387,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, >> unsigned int size) >> val = s->regs.crtc_pitch; >> break; >> case 0xf00 ... 0xfff: >> - val = pci_default_read_config(&s->dev, addr - 0xf00, size); >> + if ((addr - 0xf00) + size <= pci_config_size(&s->dev)) { >> + val = pci_default_read_config(&s->dev, addr - 0xf00, size); >> + } >> break; >> case CUR_OFFSET: >> val = s->regs.cur_offset; >> -- >> 2.26.2 >