From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=38932 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OHw7O-0001Kc-9s
	for qemu-devel@nongnu.org; Fri, 28 May 2010 05:44:43 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <yamahata@valinux.co.jp>) id 1OHw7M-0003a2-SH
	for qemu-devel@nongnu.org; Fri, 28 May 2010 05:44:42 -0400
Received: from mail.valinux.co.jp ([210.128.90.3]:40056)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <yamahata@valinux.co.jp>) id 1OHw7M-0003Zj-Jq
	for qemu-devel@nongnu.org; Fri, 28 May 2010 05:44:40 -0400
Date: Fri, 28 May 2010 18:41:15 +0900
From: Isaku Yamahata <yamahata@valinux.co.jp>
Message-ID: <20100528094115.GC13840@valinux.co.jp>
References: <20100527054442.GI31807@valinux.co.jp>
	<20100527141316.GB8287@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20100527141316.GB8287@redhat.com>
Subject: [Qemu-devel] Re: [PATCH] pci: fix pci_default_read_config().
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org

On Thu, May 27, 2010 at 05:13:16PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 27, 2010 at 02:44:42PM +0900, Isaku Yamahata wrote:
> > address and config_size are both unsigned.
> > So check which is bigger before minus operation.
> > Otherwise the result of minus can be unexpected
> > big value.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> An this happen in practice? If yes, how?

Yes, it happens.

When MMCONFIG is enabled, Linux tries to determine the configuration
space size of  pci host bridget by accessing express extended area
without checking pci express capability.
See pci_cfg_space_size_ext()/pci_cfg_space_size()
in linux/drivers/pci/probes.c

If the pci host bridge is not express capable, pci_default_read_config()
accesses dev->config beyond its size.


> 
> > ---
> >  hw/pci.c |    9 +++++++--
> >  1 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 3362842..39a6206 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -988,9 +988,14 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >                                   uint32_t address, int len)
> >  {
> >      uint32_t val = 0;
> > +    uint32_t config_size = pci_config_size(d);
> >      assert(len == 1 || len == 2 || len == 4);
> > -    len = MIN(len, pci_config_size(d) - address);
> > -    memcpy(&val, d->config + address, len);
> > +    if (address < config_size) {
> > +        len = MIN(len, config_size - address);
> > +        memcpy(&val, d->config + address, len);
> > +    } else {
> > +        val = ~0;
> > +    }
> >      return le32_to_cpu(val);
> >  }
> >  
> > -- 
> > 1.6.6.1
> > 
> 

-- 
yamahata