From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Dutile Subject: Re: [PATCH 1/2] pci: bounds check offsets into config_map Date: Thu, 21 Jul 2011 11:52:56 -0400 Message-ID: <4E284B58.9020200@redhat.com> References: <20110720233434.5002.3703.stgit@dddsys0.bos.redhat.com> <20110720234638.5002.55773.stgit@dddsys0.bos.redhat.com> <20110721081127.GC20360@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, alex.williamson@redhat.com To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51110 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751571Ab1GUPw5 (ORCPT ); Thu, 21 Jul 2011 11:52:57 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p6LFqv46026395 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 21 Jul 2011 11:52:57 -0400 In-Reply-To: <20110721081127.GC20360@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> cc: Alex Williamson >> cc: Michael S. Tsirkin >> --- >> >> 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);