| Message ID | 20110728072324.GF14976@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 28, 2011 at 04:23:24PM +0900, Isaku Yamahata wrote: > This might be a bit late comment... > > On Fri, Jul 22, 2011 at 11:05:01AM +0200, Jan Kiszka wrote: > > diff --git a/hw/pci_host.c b/hw/pci_host.c > > index 728e2d4..bfdc321 100644 > > --- a/hw/pci_host.c > > +++ b/hw/pci_host.c > > @@ -47,17 +47,33 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) > > return pci_find_device(bus, bus_num, devfn); > > } > > > > +void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr, > > + uint32_t limit, uint32_t val, uint32_t len) > > +{ > > + assert(len <= 4); > > + pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); > > +} > > + > > +uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr, > > + uint32_t limit, uint32_t len) > > +{ > > + assert(len <= 4); > > + return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); > > +} > > + > > Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit < addr. > So we need explicit "if (limit < addr) return;". > Here's the patch for pci branch. > > >From 75c1a2b47c93ad987cd7a37fb62bda9a59f27948 Mon Sep 17 00:00:00 2001 > Message-Id: <75c1a2b47c93ad987cd7a37fb62bda9a59f27948.1311837763.git.yamahata@valinux.co.jp> > From: Isaku Yamahata <yamahata@valinux.co.jp> > Date: Thu, 28 Jul 2011 16:20:28 +0900 > Subject: [PATCH] pci/host: limit check of pci_host_config_read/write_common > > This patch adds boundary check in pci_host_config_read/write_common() > Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit < addr. > So we need explicit "if (limit <= addr) return;" > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> I don't see a problem with this, but could you please clarify when does this happen? I think this is only possible for a pci device behind an express root. If so, this belongs in pcie_host.c I'd also like this info to be recorded in the commit log. > --- > hw/pci_host.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/hw/pci_host.c b/hw/pci_host.c > index 2e8a29f..71fd3a1 100644 > --- a/hw/pci_host.c > +++ b/hw/pci_host.c > @@ -51,6 +51,9 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, > uint32_t limit, uint32_t val, uint32_t len) > { > assert(len <= 4); > + if (limit <= addr) { > + return; > + } > pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); > } > > @@ -58,6 +61,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, > uint32_t limit, uint32_t len) > { > assert(len <= 4); > + if (limit <= addr) { > + return 0; > + } > return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); > } > > -- > 1.7.1.1 > > -- > yamahata
On Thu, Jul 28, 2011 at 11:40:21AM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 28, 2011 at 04:23:24PM +0900, Isaku Yamahata wrote: > > This might be a bit late comment... > > > > On Fri, Jul 22, 2011 at 11:05:01AM +0200, Jan Kiszka wrote: > > > diff --git a/hw/pci_host.c b/hw/pci_host.c > > > index 728e2d4..bfdc321 100644 > > > --- a/hw/pci_host.c > > > +++ b/hw/pci_host.c > > > @@ -47,17 +47,33 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) > > > return pci_find_device(bus, bus_num, devfn); > > > } > > > > > > +void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr, > > > + uint32_t limit, uint32_t val, uint32_t len) > > > +{ > > > + assert(len <= 4); > > > + pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); > > > +} > > > + > > > +uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr, > > > + uint32_t limit, uint32_t len) > > > +{ > > > + assert(len <= 4); > > > + return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); > > > +} > > > + > > > > Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit < addr. > > So we need explicit "if (limit < addr) return;". > > Here's the patch for pci branch. > > > > >From 75c1a2b47c93ad987cd7a37fb62bda9a59f27948 Mon Sep 17 00:00:00 2001 > > Message-Id: <75c1a2b47c93ad987cd7a37fb62bda9a59f27948.1311837763.git.yamahata@valinux.co.jp> > > From: Isaku Yamahata <yamahata@valinux.co.jp> > > Date: Thu, 28 Jul 2011 16:20:28 +0900 > > Subject: [PATCH] pci/host: limit check of pci_host_config_read/write_common > > > > This patch adds boundary check in pci_host_config_read/write_common() > > Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit < addr. > > So we need explicit "if (limit <= addr) return;" > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > I don't see a problem with this, but could you please clarify when does > this happen? I think this is only possible for a pci device > behind an express root. If so, this belongs in pcie_host.c Right. I'll move the check into pcie_host.c
diff --git a/hw/pci_host.c b/hw/pci_host.c index 2e8a29f..71fd3a1 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -51,6 +51,9 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, uint32_t limit, uint32_t val, uint32_t len) { assert(len <= 4); + if (limit <= addr) { + return; + } pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); } @@ -58,6 +61,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, uint32_t limit, uint32_t len) { assert(len <= 4); + if (limit <= addr) { + return 0; + } return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); }