* [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
@ 2020-06-04 10:55 P J P
2020-06-04 11:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: P J P @ 2020-06-04 10:55 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Daniel P . Berrangé,
Prasad J Pandit, Michael S . Tsirkin, Yi Ren, QEMU Developers,
Ren Ding, Philippe Mathieu-Daudé,
Hanqing Zhao
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>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-06-04 10:55 [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791) P J P
@ 2020-06-04 11:49 ` Michael S. Tsirkin
2020-06-04 11:56 ` Philippe Mathieu-Daudé
2020-06-04 12:01 ` Paolo Bonzini
0 siblings, 2 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2020-06-04 11:49 UTC (permalink / raw)
To: P J P
Cc: Daniel P . Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, Gerd Hoffmann,
Ren Ding, pbonzini, Philippe Mathieu-Daudé,
Hanqing Zhao
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-06-04 11:49 ` Michael S. Tsirkin
@ 2020-06-04 11:56 ` Philippe Mathieu-Daudé
2020-06-04 12:00 ` Michael S. Tsirkin
2020-06-04 12:01 ` Paolo Bonzini
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-04 11:56 UTC (permalink / raw)
To: Michael S. Tsirkin, P J P
Cc: Daniel P. Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, Gerd Hoffmann,
Ren Ding, pbonzini, Hanqing Zhao
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-06-04 11:56 ` Philippe Mathieu-Daudé
@ 2020-06-04 12:00 ` Michael S. Tsirkin
2020-07-02 7:54 ` Michael Tokarev
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2020-06-04 12:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Daniel P. Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, Gerd Hoffmann,
Ren Ding, pbonzini, P J P, Hanqing Zhao
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-06-04 11:49 ` Michael S. Tsirkin
2020-06-04 11:56 ` Philippe Mathieu-Daudé
@ 2020-06-04 12:01 ` Paolo Bonzini
1 sibling, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2020-06-04 12:01 UTC (permalink / raw)
To: Michael S. Tsirkin, P J P
Cc: Daniel P. Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, Gerd Hoffmann,
Ren Ding, Philippe Mathieu-Daudé,
Hanqing Zhao
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-06-04 12:00 ` Michael S. Tsirkin
@ 2020-07-02 7:54 ` Michael Tokarev
2020-07-02 9:36 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Michael Tokarev @ 2020-07-02 7:54 UTC (permalink / raw)
To: Michael S. Tsirkin, Philippe Mathieu-Daudé
Cc: Daniel P. Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, P J P, Gerd Hoffmann,
Ren Ding, pbonzini, Hanqing Zhao
[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
>>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791)
2020-07-02 7:54 ` Michael Tokarev
@ 2020-07-02 9:36 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2020-07-02 9:36 UTC (permalink / raw)
To: Michael Tokarev, Michael S. Tsirkin, Philippe Mathieu-Daudé
Cc: Daniel P. Berrangé,
Prasad J Pandit, Yi Ren, QEMU Developers, P J P, Gerd Hoffmann,
Ren Ding, Hanqing Zhao
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-02 9:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04 10:55 [PATCH v3] ati-vga: check address before reading configuration bytes (CVE-2020-13791) P J P
2020-06-04 11:49 ` Michael S. Tsirkin
2020-06-04 11:56 ` Philippe Mathieu-Daudé
2020-06-04 12:00 ` Michael S. Tsirkin
2020-07-02 7:54 ` Michael Tokarev
2020-07-02 9:36 ` Paolo Bonzini
2020-06-04 12:01 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.