From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8auB-0002Jo-DJ for qemu-devel@nongnu.org; Tue, 27 Sep 2011 12:53:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R8auA-00077Y-D1 for qemu-devel@nongnu.org; Tue, 27 Sep 2011 12:53:15 -0400 Message-ID: <4E81FF57.7070206@freescale.com> Date: Tue, 27 Sep 2011 11:52:39 -0500 From: Scott Wood MIME-Version: 1.0 References: <1317111439-6478-1-git-send-email-yu.liu@freescale.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Liu Yu , qemu-ppc@nongnu.org, qemu-devel Developers On 09/27/2011 07:45 AM, Alexander Graf wrote: > On 27.09.2011, at 10:17, Liu Yu wrote: >> --- >> hw/ppce500_pci.c | 26 ++++++++++++++++---------- >> 1 files changed, 16 insertions(+), 10 deletions(-) >> >> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c >> index 2db365d..3e24e85 100644 >> --- a/hw/ppce500_pci.c >> +++ b/hw/ppce500_pci.c >> @@ -108,15 +108,18 @@ static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr) >> >> case PPCE500_PCI_IW3: >> case PPCE500_PCI_IW2: >> - case PPCE500_PCI_IW1: >> + case PPCE500_PCI_IW1: { >> + int idx = ((addr >> 5) & 0x3) - 1; > > So this is the main change, right? Why the -1? A guest could potentially access pib[-1] using this, no? Not with the values of addr that lead to this code. The -1 is because IW1/2/3 are 0x1e0/0x1c0/0x1a0. Previously IW1 would overflow the array. >> switch (addr & 0xC) { >> - case PCI_PITAR: value = pci->pib[(addr >> 5) & 0x3].pitar; break; >> - case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 0x3].piwbar; break; >> - case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 0x3].piwbear; break; >> - case PCI_PIWAR: value = pci->pib[(addr >> 5) & 0x3].piwar; break; >> + case PCI_PITAR: value = pci->pib[idx].pitar; break; >> + case PCI_PIWBAR: value = pci->pib[idx].piwbar; break; >> + case PCI_PIWBEAR: value = pci->pib[idx].piwbear; break; >> + case PCI_PIWAR: value = pci->pib[idx].piwar; break; > > I'm fairly sure this breaks checkpatch.pl. So does the original code... If this is to be fixed, the outbound window switch should be fixed too (and made to use idx, for consistency). -Scott