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.yamah...@valinux.co.jp>
> From: Isaku Yamahata <yamah...@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 <yamah...@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
