| Message ID | 20110720234638.5002.55773.stgit@dddsys0.bos.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2011 at 07:49:34PM -0400, Donald Dutile wrote: > Bounds check to avoid walking off config_map[] > and corrupting what is after it. > > Signed-off-by: Donald Dutile <ddutile@redhat.com> > cc: Alex Williamson <alex.williamson@redhat.com> > cc: Michael S. Tsirkin <mst@redhat.com> > --- > > hw/pci.c | 16 ++++++++++++++-- > 1 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 5deaa04..9a7ff2d 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > } > } else { > int i; > + uint32_t config_size = pci_config_size(pdev); This is actually slightly wrong: even for express devices, legacy capabilities can not spill out from the low 256 bytes: the extended config space has its own capability list always starting with the express capability at offset 256. > + > + /* ensure don't walk off end of config_map[] */ > + if (offset > (config_size - size)) { I prefer not to have () around math here. > + fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " > + "Attempt to add PCI capability 0x%x at offset 0x%x," > + "size 0x%x, which exceeds emulated PCI config space 0x%x\n", > + pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > + cap_id, offset, size, config_size); > + return -EINVAL; > + } > > for (i = offset; i < offset + size; i++) { > if (pdev->config_map[i]) { > fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " > - "Attempt to add PCI capability %x at offset " > - "%x overlaps existing capability %x at offset %x\n", > + "Attempt to add PCI capability 0x%x at offset " > + "0x%x overlaps existing capability 0x%x at offset 0x%x\n", > pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), > PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > cap_id, offset, pdev->config_map[i], i); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/21/2011 04:11 AM, Michael S. Tsirkin wrote: > On Wed, Jul 20, 2011 at 07:49:34PM -0400, Donald Dutile wrote: >> Bounds check to avoid walking off config_map[] >> and corrupting what is after it. >> >> Signed-off-by: Donald Dutile<ddutile@redhat.com> >> cc: Alex Williamson<alex.williamson@redhat.com> >> cc: Michael S. Tsirkin<mst@redhat.com> >> --- >> >> hw/pci.c | 16 ++++++++++++++-- >> 1 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci.c b/hw/pci.c >> index 5deaa04..9a7ff2d 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, >> } >> } else { >> int i; >> + uint32_t config_size = pci_config_size(pdev); > > This is actually slightly wrong: even for express > devices, legacy capabilities can not spill out from > the low 256 bytes: the extended config space has its > own capability list always starting with the express > capability at offset 256. > will just remove this checking & put it all in device-assignment.c >> + >> + /* ensure don't walk off end of config_map[] */ >> + if (offset> (config_size - size)) { > > I prefer not to have () around math here. > > ok. >> + fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " >> + "Attempt to add PCI capability 0x%x at offset 0x%x," >> + "size 0x%x, which exceeds emulated PCI config space 0x%x\n", >> + pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), >> + cap_id, offset, size, config_size); >> + return -EINVAL; >> + } >> >> for (i = offset; i< offset + size; i++) { >> if (pdev->config_map[i]) { >> fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " >> - "Attempt to add PCI capability %x at offset " >> - "%x overlaps existing capability %x at offset %x\n", >> + "Attempt to add PCI capability 0x%x at offset " >> + "0x%x overlaps existing capability 0x%x at offset 0x%x\n", >> pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), >> PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), >> cap_id, offset, pdev->config_map[i], i); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/hw/pci.c b/hw/pci.c index 5deaa04..9a7ff2d 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } } else { int i; + uint32_t config_size = pci_config_size(pdev); + + /* ensure don't walk off end of config_map[] */ + if (offset > (config_size - size)) { + fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " + "Attempt to add PCI capability 0x%x at offset 0x%x," + "size 0x%x, which exceeds emulated PCI config space 0x%x\n", + pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), + cap_id, offset, size, config_size); + return -EINVAL; + } for (i = offset; i < offset + size; i++) { if (pdev->config_map[i]) { fprintf(stderr, "ERROR: %04x:%02x:%02x.%x " - "Attempt to add PCI capability %x at offset " - "%x overlaps existing capability %x at offset %x\n", + "Attempt to add PCI capability 0x%x at offset " + "0x%x overlaps existing capability 0x%x at offset 0x%x\n", pci_find_domain(pdev->bus), pci_bus_num(pdev->bus), PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), cap_id, offset, pdev->config_map[i], i);
Bounds check to avoid walking off config_map[] and corrupting what is after it. Signed-off-by: Donald Dutile <ddutile@redhat.com> cc: Alex Williamson <alex.williamson@redhat.com> cc: Michael S. Tsirkin <mst@redhat.com> --- hw/pci.c | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html