[Qemu-devel] [PATCH] pci: fix pci_default_read_config().

[Qemu-devel] [PATCH] pci: fix pci_default_read_config().

[Isaku Yamahata]

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(-)

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

[Qemu-devel] Re: [PATCH] pci: fix pci_default_read_config().

[Michael S. Tsirkin]

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
> 

[Qemu-devel] Re: [PATCH] pci: fix pci_default_read_config().

[Isaku Yamahata]

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