Patchwork pci: fix pci_default_read_config().

login
register
mail settings
Submitter Isaku Yamahata
Date May 27, 2010, 5:44 a.m.
Message ID <20100527054442.GI31807@valinux.co.jp>
Download mbox | patch
Permalink /patch/53683/
State New
Headers show

Comments

Isaku Yamahata - May 27, 2010, 5:44 a.m.
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>
---
 hw/pci.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Michael S. Tsirkin - May 27, 2010, 2:13 p.m.
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?

> ---
>  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
>
Isaku Yamahata - May 28, 2010, 9:41 a.m.
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
> > 
>

Patch

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