[Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Gerd Hoffmann]

Just found while investigating
  https://bugzilla.redhat.com/show_bug.cgi?id=1707118

Found PCIe extended config space filled with random crap due to
allocation being too small (conventional pci config space only).

PCI(e) config space is guest writable.  Writes are limited by
write mask (which probably is also filled with random stuff),
so the guest can only flip enabled bits.  But I suspect it
still might be exploitable, so rather serious because it might
be a host escape for the guest.  On the other hand the device
is probably not yet in widespread use.

Migitation: use "-device bochs-display" as conventional pci
device only.

Note: qemu 4.1 release is planned for tomorrow.

Gerd Hoffmann (1):
  display/bochs: fix pcie support

 hw/display/bochs-display.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.18.1

[Qemu-devel] [PATCH 1/1] display/bochs: fix pcie support

[Gerd Hoffmann]

Set QEMU_PCI_CAP_EXPRESS unconditionally in init(), then clear it in
realize() in case the device is not connected to a PCIe bus.

This makes sure the pci config space allocation is big enough, so
accessing the PCIe extended config space doesn't overflow the pci
config space buffer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/bochs-display.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 582133dd719c..8e83b5164b67 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -297,9 +297,10 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     }
 
     if (pci_bus_is_express(pci_get_bus(dev))) {
-        dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
         ret = pcie_endpoint_cap_init(dev, 0x80);
         assert(ret > 0);
+    } else {
+        dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
     }
 
     memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
@@ -322,11 +323,15 @@ static void bochs_display_set_big_endian_fb(Object *obj, bool value,
 
 static void bochs_display_init(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
+
     /* Expose framebuffer byteorder via QOM */
     object_property_add_bool(obj, "big-endian-framebuffer",
                              bochs_display_get_big_endian_fb,
                              bochs_display_set_big_endian_fb,
                              NULL);
+
+    dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
 }
 
 static void bochs_display_exit(PCIDevice *dev)
-- 
2.18.1

Re: [Qemu-devel] [PATCH 1/1] display/bochs: fix pcie support

[Alex Williamson]

On Mon, 12 Aug 2019 08:52:21 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Set QEMU_PCI_CAP_EXPRESS unconditionally in init(), then clear it in
> realize() in case the device is not connected to a PCIe bus.
> 
> This makes sure the pci config space allocation is big enough, so
> accessing the PCIe extended config space doesn't overflow the pci
> config space buffer.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/bochs-display.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)


Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

 
> diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
> index 582133dd719c..8e83b5164b67 100644
> --- a/hw/display/bochs-display.c
> +++ b/hw/display/bochs-display.c
> @@ -297,9 +297,10 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      if (pci_bus_is_express(pci_get_bus(dev))) {
> -        dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>          ret = pcie_endpoint_cap_init(dev, 0x80);
>          assert(ret > 0);
> +    } else {
> +        dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
>      }
>  
>      memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
> @@ -322,11 +323,15 @@ static void bochs_display_set_big_endian_fb(Object *obj, bool value,
>  
>  static void bochs_display_init(Object *obj)
>  {
> +    PCIDevice *dev = PCI_DEVICE(obj);
> +
>      /* Expose framebuffer byteorder via QOM */
>      object_property_add_bool(obj, "big-endian-framebuffer",
>                               bochs_display_get_big_endian_fb,
>                               bochs_display_set_big_endian_fb,
>                               NULL);
> +
> +    dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>  }
>  
>  static void bochs_display_exit(PCIDevice *dev)

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Paolo Bonzini]

On 12/08/19 08:52, Gerd Hoffmann wrote:
> Just found while investigating
>   https://bugzilla.redhat.com/show_bug.cgi?id=1707118
> 
> Found PCIe extended config space filled with random crap due to
> allocation being too small (conventional pci config space only).
> 
> PCI(e) config space is guest writable.  Writes are limited by
> write mask (which probably is also filled with random stuff),

Yes, it is also allocated with 256 bytes only.

> so the guest can only flip enabled bits.  But I suspect it
> still might be exploitable, so rather serious because it might
> be a host escape for the guest.  On the other hand the device
> is probably not yet in widespread use.
> 
> Migitation: use "-device bochs-display" as conventional pci
> device only.
> 
> Note: qemu 4.1 release is planned for tomorrow.
> 
> Gerd Hoffmann (1):
>   display/bochs: fix pcie support
> 
>  hw/display/bochs-display.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 

Looks good to me, and no other device seems to have the same issue.  We
could add an assertion that pci_config_size has not increased after
calling pc->realize.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Philippe Mathieu-Daudé]

On 8/12/19 2:45 PM, Paolo Bonzini wrote:
> On 12/08/19 08:52, Gerd Hoffmann wrote:
>> Just found while investigating
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1707118
>>
>> Found PCIe extended config space filled with random crap due to
>> allocation being too small (conventional pci config space only).
>>

Can you amend this information to the commit description?

<...

>> PCI(e) config space is guest writable.  Writes are limited by
>> write mask (which probably is also filled with random stuff),
> 
> Yes, it is also allocated with 256 bytes only.
> 
>> so the guest can only flip enabled bits.  But I suspect it
>> still might be exploitable, so rather serious because it might
>> be a host escape for the guest.  On the other hand the device
>> is probably not yet in widespread use.

...>

>> Migitation: use "-device bochs-display" as conventional pci
>> device only.
>>
>> Note: qemu 4.1 release is planned for tomorrow.
>>
>> Gerd Hoffmann (1):
>>   display/bochs: fix pcie support
>>
>>  hw/display/bochs-display.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
> 
> Looks good to me, and no other device seems to have the same issue.  We
> could add an assertion that pci_config_size has not increased after
> calling pc->realize.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Peter Maydell]

On Mon, 12 Aug 2019 at 13:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/12/19 2:45 PM, Paolo Bonzini wrote:
> > On 12/08/19 08:52, Gerd Hoffmann wrote:
> >> Just found while investigating
> >>   https://bugzilla.redhat.com/show_bug.cgi?id=1707118
> >>
> >> Found PCIe extended config space filled with random crap due to
> >> allocation being too small (conventional pci config space only).
> >>
>
> Can you amend this information to the commit description?
>
> <...
>
> >> PCI(e) config space is guest writable.  Writes are limited by
> >> write mask (which probably is also filled with random stuff),
> >
> > Yes, it is also allocated with 256 bytes only.
> >
> >> so the guest can only flip enabled bits.  But I suspect it
> >> still might be exploitable, so rather serious because it might
> >> be a host escape for the guest.  On the other hand the device
> >> is probably not yet in widespread use.
>
> ...>

I can add to the commit this paragraph of the cover letter,
and I think also the 'mitigation' note might as well go in.

I've also put the cc:stable into the commit message.

Updated commit, ready to apply to master if we're OK with it:

https://git.linaro.org/people/peter.maydell/qemu-arm.git/commit/?h=staging&id=c075b5f318a8be628ab8edf93be33f5a93a4aacd

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Philippe Mathieu-Daudé]

On 8/12/19 3:39 PM, Peter Maydell wrote:
> On Mon, 12 Aug 2019 at 13:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 8/12/19 2:45 PM, Paolo Bonzini wrote:
>>> On 12/08/19 08:52, Gerd Hoffmann wrote:
>>>> Just found while investigating
>>>>   https://bugzilla.redhat.com/show_bug.cgi?id=1707118
>>>>
>>>> Found PCIe extended config space filled with random crap due to
>>>> allocation being too small (conventional pci config space only).
>>>>
>>
>> Can you amend this information to the commit description?
>>
>> <...
>>
>>>> PCI(e) config space is guest writable.  Writes are limited by
>>>> write mask (which probably is also filled with random stuff),
>>>
>>> Yes, it is also allocated with 256 bytes only.
>>>
>>>> so the guest can only flip enabled bits.  But I suspect it
>>>> still might be exploitable, so rather serious because it might
>>>> be a host escape for the guest.  On the other hand the device
>>>> is probably not yet in widespread use.
>>
>> ...>
> 
> I can add to the commit this paragraph of the cover letter,
> and I think also the 'mitigation' note might as well go in.

Yes.

> 
> I've also put the cc:stable into the commit message.
> 
> Updated commit, ready to apply to master if we're OK with it:
> 
> https://git.linaro.org/people/peter.maydell/qemu-arm.git/commit/?h=staging&id=c075b5f318a8be628ab8edf93be33f5a93a4aacd

Thank you!

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Alex Williamson]

On Mon, 12 Aug 2019 14:39:53 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 12 Aug 2019 at 13:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > On 8/12/19 2:45 PM, Paolo Bonzini wrote:  
> > > On 12/08/19 08:52, Gerd Hoffmann wrote:  
> > >> Just found while investigating
> > >>   https://bugzilla.redhat.com/show_bug.cgi?id=1707118
> > >>
> > >> Found PCIe extended config space filled with random crap due to
> > >> allocation being too small (conventional pci config space only).
> > >>  
> >
> > Can you amend this information to the commit description?
> >
> > <...
> >  
> > >> PCI(e) config space is guest writable.  Writes are limited by
> > >> write mask (which probably is also filled with random stuff),  
> > >
> > > Yes, it is also allocated with 256 bytes only.
> > >  
> > >> so the guest can only flip enabled bits.  But I suspect it
> > >> still might be exploitable, so rather serious because it might
> > >> be a host escape for the guest.  On the other hand the device
> > >> is probably not yet in widespread use.  
> >  
> > ...>  
> 
> I can add to the commit this paragraph of the cover letter,
> and I think also the 'mitigation' note might as well go in.
> 
> I've also put the cc:stable into the commit message.
> 
> Updated commit, ready to apply to master if we're OK with it:
> 
> https://git.linaro.org/people/peter.maydell/qemu-arm.git/commit/?h=staging&id=c075b5f318a8be628ab8edf93be33f5a93a4aacd

Quoting new commit log:

	This makes sure the pci config space allocation is big enough,
	so accessing the PCIe extended config space doesn't overflow
	the pci config space buffer.

	PCI(e) config space is guest writable.  Writes are limited
	bywrite mask (which probably is also filled with random stuff),
	so the guest can only flip enabled bits.  But I suspect it
	still might be exploitable, so rather serious because it might
	be a host escape for the guest.  On the other hand the device
	is probably not yet in widespread use.

	Mitigation: use "-device bochs-display" as conventional pci
	device only.

Is it clear to others that this mitigation remark seems to be
referencing an alternative configuration constraint to avoid the issue
rather than what's actually implemented in this patch?  IOW, if we
never place the bochs-display device into a PCIe hierarchy, then
extended config space is never accessible to the guest anyway, and
there is no issue.  I think this was meant to be an alternative to the
patch but the enforcement of that would happen above QEMU, probably why
it was mentioned in the cover letter rather than the original commit
log.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Peter Maydell]

On Mon, 12 Aug 2019 at 16:35, Alex Williamson
<alex.williamson@redhat.com> wrote:
> Quoting new commit log:
>
>         This makes sure the pci config space allocation is big enough,
>         so accessing the PCIe extended config space doesn't overflow
>         the pci config space buffer.
>
>         PCI(e) config space is guest writable.  Writes are limited
>         bywrite mask (which probably is also filled with random stuff),
>         so the guest can only flip enabled bits.  But I suspect it
>         still might be exploitable, so rather serious because it might
>         be a host escape for the guest.  On the other hand the device
>         is probably not yet in widespread use.
>
>         Mitigation: use "-device bochs-display" as conventional pci
>         device only.
>
> Is it clear to others that this mitigation remark seems to be
> referencing an alternative configuration constraint to avoid the issue
> rather than what's actually implemented in this patch?  IOW, if we
> never place the bochs-display device into a PCIe hierarchy, then
> extended config space is never accessible to the guest anyway, and
> there is no issue.  I think this was meant to be an alternative to the
> patch but the enforcement of that would happen above QEMU, probably why
> it was mentioned in the cover letter rather than the original commit
> log.  Thanks,

Yeah, that's unclear in retrospect. How about:

# (For a QEMU version without this commit, a mitigation for the
# bug is available: use "-device bochs-display" as a conventional pci
# device only.)

?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Alex Williamson]

On Mon, 12 Aug 2019 16:38:05 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 12 Aug 2019 at 16:35, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > Quoting new commit log:
> >
> >         This makes sure the pci config space allocation is big enough,
> >         so accessing the PCIe extended config space doesn't overflow
> >         the pci config space buffer.
> >
> >         PCI(e) config space is guest writable.  Writes are limited
> >         bywrite mask (which probably is also filled with random stuff),
> >         so the guest can only flip enabled bits.  But I suspect it
> >         still might be exploitable, so rather serious because it might
> >         be a host escape for the guest.  On the other hand the device
> >         is probably not yet in widespread use.
> >
> >         Mitigation: use "-device bochs-display" as conventional pci
> >         device only.
> >
> > Is it clear to others that this mitigation remark seems to be
> > referencing an alternative configuration constraint to avoid the issue
> > rather than what's actually implemented in this patch?  IOW, if we
> > never place the bochs-display device into a PCIe hierarchy, then
> > extended config space is never accessible to the guest anyway, and
> > there is no issue.  I think this was meant to be an alternative to the
> > patch but the enforcement of that would happen above QEMU, probably why
> > it was mentioned in the cover letter rather than the original commit
> > log.  Thanks,  
> 
> Yeah, that's unclear in retrospect. How about:
> 
> # (For a QEMU version without this commit, a mitigation for the
> # bug is available: use "-device bochs-display" as a conventional pci
> # device only.)

Yes, better.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/1] display/bochs: fix pcie support (qemu security issue)

[Peter Maydell]

On Mon, 12 Aug 2019 at 16:48, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 12 Aug 2019 16:38:05 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Mon, 12 Aug 2019 at 16:35, Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > > Quoting new commit log:
> > >
> > >         This makes sure the pci config space allocation is big enough,
> > >         so accessing the PCIe extended config space doesn't overflow
> > >         the pci config space buffer.
> > >
> > >         PCI(e) config space is guest writable.  Writes are limited
> > >         bywrite mask (which probably is also filled with random stuff),
> > >         so the guest can only flip enabled bits.  But I suspect it
> > >         still might be exploitable, so rather serious because it might
> > >         be a host escape for the guest.  On the other hand the device
> > >         is probably not yet in widespread use.
> > >
> > >         Mitigation: use "-device bochs-display" as conventional pci
> > >         device only.
> > >
> > > Is it clear to others that this mitigation remark seems to be
> > > referencing an alternative configuration constraint to avoid the issue
> > > rather than what's actually implemented in this patch?  IOW, if we
> > > never place the bochs-display device into a PCIe hierarchy, then
> > > extended config space is never accessible to the guest anyway, and
> > > there is no issue.  I think this was meant to be an alternative to the
> > > patch but the enforcement of that would happen above QEMU, probably why
> > > it was mentioned in the cover letter rather than the original commit
> > > log.  Thanks,
> >
> > Yeah, that's unclear in retrospect. How about:
> >
> > # (For a QEMU version without this commit, a mitigation for the
> > # bug is available: use "-device bochs-display" as a conventional pci
> > # device only.)
>
> Yes, better.  Thanks,

Cool. Updated commit message now pushed to master.

-- PMM