| Message ID | 20200604105524.46158-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791) | expand |
On Thu, Jun 04, 2020 at 04:25:24PM +0530, P J P wrote: > From: Prasad J Pandit <pjp@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 <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> > Signed-off-by: Prasad J Pandit <pjp@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? > --- > 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
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 <pjp@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 <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> >> Signed-off-by: Prasad J Pandit <pjp@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 >
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 <pjp@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 <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> > >> Signed-off-by: Prasad J Pandit <pjp@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 > >
On 04/06/20 13:49, Michael S. Tsirkin wrote: > On Thu, Jun 04, 2020 at 04:25:24PM +0530, P J P wrote: >> From: Prasad J Pandit <pjp@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 <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> >> Signed-off-by: Prasad J Pandit <pjp@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? In this case the path should be address_space_stl_le address_space_stl_internal memory_region_dispatch_write memory_region_access_valid and then it does check valid.unaligned. Is there a testcase? Paolo > >> --- >> 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 >
[Please excuse me for top-posting, I want to preserve somewhat old context] So, is this CVE-2020-13791 issue fixed by the fix for CVE-2020-13754, by this commit: https://git.qemu.org/?p=qemu.git;a=commit;h=5d971f9e672507210e77d020d89e0e89165c8fc9 ? Thanks, /mjt 04.06.2020 15:00, Michael S. Tsirkin wrote: > 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 <pjp@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 <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> >>>> Signed-off-by: Prasad J Pandit <pjp@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 >>> > >
On 02/07/20 09:54, Michael Tokarev wrote: > [Please excuse me for top-posting, I want to preserve somewhat old context] > > So, is this CVE-2020-13791 issue fixed by the fix for CVE-2020-13754, > by this commit: > https://git.qemu.org/?p=qemu.git;a=commit;h=5d971f9e672507210e77d020d89e0e89165c8fc9 Yes, it is. Paolo
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;