PPC: Pseries: Check for PCI boundaries

We call pci_host_config_{read,write}_common() which perform PCI config
accesses. However they don't do all limit checking the way we expect
it to.

So let's introduce a small wrapper around them, making them behave the
way we would without touching generic code.

This patch is based on a patch by David Gibson which put this logic into
the generic code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/spapr_pci.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

On Wed, Jan 18, 2012 at 05:24:50PM +0100, Alexander Graf wrote:
> We call pci_host_config_{read,write}_common() which perform PCI config
> accesses. However they don't do all limit checking the way we expect
> it to.
> 
> So let's introduce a small wrapper around them, making them behave the
> way we would without touching generic code.
> 
> This patch is based on a patch by David Gibson which put this logic into
> the generic code.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Nod. Looks like a sensible way to fix pseries without
bothering about breaking other arches.

> ---
>  hw/spapr_pci.c |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index cf37628..2c95faa 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -67,6 +67,25 @@ static uint32_t rtas_pci_cfgaddr(uint32_t arg)
>      return ((arg >> 20) & 0xf00) | (arg & 0xff);
>  }
>  
> +static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
> +                                        uint32_t limit, uint32_t len)
> +{
> +    if ((addr + len) <= limit) {
> +        return pci_host_config_read_common(pci_dev, addr, limit, len);
> +    } else {
> +        return ~0x0;
> +    }
> +}
> +
> +static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
> +                                     uint32_t limit, uint32_t val,
> +                                     uint32_t len)
> +{
> +    if ((addr + len) <= limit) {
> +        pci_host_config_write_common(pci_dev, addr, limit, val, len);
> +    }
> +}
> +
>  static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
>                                       uint32_t token, uint32_t nargs,
>                                       target_ulong args,
> @@ -82,7 +101,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
>      }
>      size = rtas_ld(args, 3);
>      addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    val = pci_host_config_read_common(dev, addr, pci_config_size(dev), size);
> +    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
>      rtas_st(rets, 0, 0);
>      rtas_st(rets, 1, val);
>  }
> @@ -101,7 +120,7 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr,
>      }
>      size = rtas_ld(args, 1);
>      addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    val = pci_host_config_read_common(dev, addr, pci_config_size(dev), size);
> +    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
>      rtas_st(rets, 0, 0);
>      rtas_st(rets, 1, val);
>  }
> @@ -122,7 +141,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
>      val = rtas_ld(args, 4);
>      size = rtas_ld(args, 3);
>      addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    pci_host_config_write_common(dev, addr, pci_config_size(dev), val, size);
> +    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
>      rtas_st(rets, 0, 0);
>  }
>  
> @@ -141,7 +160,7 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
>      val = rtas_ld(args, 2);
>      size = rtas_ld(args, 1);
>      addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    pci_host_config_write_common(dev, addr, pci_config_size(dev), val, size);
> +    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
>      rtas_st(rets, 0, 0);
>  }
>  
> -- 
> 1.6.0.2

On Wed, Jan 18, 2012 at 07:28:33PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 18, 2012 at 05:24:50PM +0100, Alexander Graf wrote:
> > We call pci_host_config_{read,write}_common() which perform PCI config
> > accesses. However they don't do all limit checking the way we expect
> > it to.
> > 
> > So let's introduce a small wrapper around them, making them behave the
> > way we would without touching generic code.
> > 
> > This patch is based on a patch by David Gibson which put this logic into
> > the generic code.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Nod. Looks like a sensible way to fix pseries without
> bothering about breaking other arches.

Yeah.  I still think the bounds check should be done in common code,
but I'll make up another patch to push it there later.

Apologies for earlier comments on the matter, I've had a very
frustrating month :/