On Thu, Jun 04, 2020 at 01:56:45PM +0200, Philippe Mathieu-Daudé wrote: > 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
Yea looks like more devices following documentation and memory core doing something else instead. > > > >> --- > >> 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 > >