[Qemu-devel] [PATCH for-3.1] hw/xen/xen_pt_graphics: Don't trust the BIOS ROM contents so much

[Qemu-devel] [PATCH for-3.1] hw/xen/xen_pt_graphics: Don't trust the BIOS ROM contents so much

[Peter Maydell]

Coverity (CID 796599) points out that xen_pt_setup_vga() trusts
the rom->size field in the BIOS ROM from a PCI passthrough VGA
device, and uses it as an index into the memory which contains
the BIOS image. A corrupt BIOS ROM could therefore cause us to
index off the end of the buffer.

Check that the size is within bounds before we use it.

We are also trusting the pcioffset field, and assuming that
the whole rom_header is present; Coverity doesn't notice these,
but check them too.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Disclaimer: compile tested only, as I don't have a Xen setup,
let alone one with pass-through PCI graphics.

Note that https://xenbits.xen.org/xsa/advisory-124.html
defines that bugs which are only exploitable by a malicious
piece of hardware that is passed through to the guest are
not security vulnerabilities as far as the Xen Project is
concerned, and are treated like normal non-security-related bugs.
So this is just a bugfix, not a security issue.

Marked "for-3.1" because it would let us squash another Coverity
issue, and it is a bug fix; on the other hand it's an obscure
corner case and has been this way since forever.

---
 hw/xen/xen_pt_graphics.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 135c8df1e72..60d6b4a5563 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -185,8 +185,19 @@ void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
         return;
     }
 
+    if (bios_size < sizeof(struct rom_header)) {
+        error_setg(errp, "VGA: VBIOS image corrupt (too small)");
+        return;
+    }
+
     /* Currently we fixed this address as a primary. */
     rom = (struct rom_header *)bios;
+
+    if (rom->pcioffset + sizeof(struct pci_data) > bios_size) {
+        error_setg(errp, "VGA: VBIOS image corrupt (bad pcioffset field)");
+        return;
+    }
+
     pd = (void *)(bios + (unsigned char)rom->pcioffset);
 
     /* We may need to fixup Device Identification. */
@@ -194,6 +205,11 @@ void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
         pd->device = s->real_device.device_id;
 
         len = rom->size * 512;
+        if (len > bios_size) {
+            error_setg(errp, "VGA: VBIOS image corrupt (bad size field)");
+            return;
+        }
+
         /* Then adjust the bios checksum */
         for (c = (char *)bios; c < ((char *)bios + len); c++) {
             checksum += *c;
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1] hw/xen/xen_pt_graphics: Don't trust the BIOS ROM contents so much

[Anthony PERARD]

On Mon, Nov 19, 2018 at 04:26:58PM +0000, Peter Maydell wrote:
> Coverity (CID 796599) points out that xen_pt_setup_vga() trusts
> the rom->size field in the BIOS ROM from a PCI passthrough VGA
> device, and uses it as an index into the memory which contains
> the BIOS image. A corrupt BIOS ROM could therefore cause us to
> index off the end of the buffer.
> 
> Check that the size is within bounds before we use it.
> 
> We are also trusting the pcioffset field, and assuming that
> the whole rom_header is present; Coverity doesn't notice these,
> but check them too.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Disclaimer: compile tested only, as I don't have a Xen setup,
> let alone one with pass-through PCI graphics.
> 
> Note that https://xenbits.xen.org/xsa/advisory-124.html
> defines that bugs which are only exploitable by a malicious
> piece of hardware that is passed through to the guest are
> not security vulnerabilities as far as the Xen Project is
> concerned, and are treated like normal non-security-related bugs.
> So this is just a bugfix, not a security issue.
> 
> Marked "for-3.1" because it would let us squash another Coverity
> issue, and it is a bug fix; on the other hand it's an obscure
> corner case and has been this way since forever.

I haven't tested that patch either, but the changes looks fine, so:

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH for-3.1] hw/xen/xen_pt_graphics: Don't trust the BIOS ROM contents so much

[Peter Maydell]

On Mon, 26 Nov 2018 at 15:03, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> On Mon, Nov 19, 2018 at 04:26:58PM +0000, Peter Maydell wrote:
> > Coverity (CID 796599) points out that xen_pt_setup_vga() trusts
> > the rom->size field in the BIOS ROM from a PCI passthrough VGA
> > device, and uses it as an index into the memory which contains
> > the BIOS image. A corrupt BIOS ROM could therefore cause us to
> > index off the end of the buffer.
> >
> > Check that the size is within bounds before we use it.
> >
> > We are also trusting the pcioffset field, and assuming that
> > the whole rom_header is present; Coverity doesn't notice these,
> > but check them too.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Disclaimer: compile tested only, as I don't have a Xen setup,
> > let alone one with pass-through PCI graphics.
> >
> > Note that https://xenbits.xen.org/xsa/advisory-124.html
> > defines that bugs which are only exploitable by a malicious
> > piece of hardware that is passed through to the guest are
> > not security vulnerabilities as far as the Xen Project is
> > concerned, and are treated like normal non-security-related bugs.
> > So this is just a bugfix, not a security issue.
> >
> > Marked "for-3.1" because it would let us squash another Coverity
> > issue, and it is a bug fix; on the other hand it's an obscure
> > corner case and has been this way since forever.
>
> I haven't tested that patch either, but the changes looks fine, so:
>
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Ping! Would the Xen folks like to test this and/or send it in
via a xen pullreq now that 4.0 has reopened for development?

Alternatively I can put it in via a pullreq I'm currently
doing in its current "not tested but looks fine" state :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-3.1] hw/xen/xen_pt_graphics: Don't trust the BIOS ROM contents so much

[Stefano Stabellini]

On Fri, 14 Dec 2018, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 15:03, Anthony PERARD <anthony.perard@citrix.com> wrote:
> >
> > On Mon, Nov 19, 2018 at 04:26:58PM +0000, Peter Maydell wrote:
> > > Coverity (CID 796599) points out that xen_pt_setup_vga() trusts
> > > the rom->size field in the BIOS ROM from a PCI passthrough VGA
> > > device, and uses it as an index into the memory which contains
> > > the BIOS image. A corrupt BIOS ROM could therefore cause us to
> > > index off the end of the buffer.
> > >
> > > Check that the size is within bounds before we use it.
> > >
> > > We are also trusting the pcioffset field, and assuming that
> > > the whole rom_header is present; Coverity doesn't notice these,
> > > but check them too.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > Disclaimer: compile tested only, as I don't have a Xen setup,
> > > let alone one with pass-through PCI graphics.
> > >
> > > Note that https://xenbits.xen.org/xsa/advisory-124.html
> > > defines that bugs which are only exploitable by a malicious
> > > piece of hardware that is passed through to the guest are
> > > not security vulnerabilities as far as the Xen Project is
> > > concerned, and are treated like normal non-security-related bugs.
> > > So this is just a bugfix, not a security issue.
> > >
> > > Marked "for-3.1" because it would let us squash another Coverity
> > > issue, and it is a bug fix; on the other hand it's an obscure
> > > corner case and has been this way since forever.
> >
> > I haven't tested that patch either, but the changes looks fine, so:
> >
> > Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Ping! Would the Xen folks like to test this and/or send it in
> via a xen pullreq now that 4.0 has reopened for development?
> 
> Alternatively I can put it in via a pullreq I'm currently
> doing in its current "not tested but looks fine" state :-)

Hi Peter,

I know that Anthony is preparing a pretty large pull request for you.
You should see something coming your way soon.

Cheers,

Stefano