Patchwork [v2] pci: Common overflow prevention

login
register
mail settings
Submitter Isaku Yamahata
Date July 28, 2011, 7:23 a.m.
Message ID <20110728072324.GF14976@valinux.co.jp>
Download mbox | patch
Permalink /patch/107188/
State New
Headers show

Comments

Isaku Yamahata - July 28, 2011, 7:23 a.m.
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>
---
 hw/pci_host.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Michael S. Tsirkin - July 28, 2011, 8:40 a.m.
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
Isaku Yamahata - July 28, 2011, 12:50 p.m.
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

Patch

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));
 }