Re: [Qemu-ppc] [PATCH 1/3] pseries: Fix RTAS based config access

[Michael S. Tsirkin]

On Mon, Apr 02, 2012 at 02:17:35PM +1000, David Gibson wrote:
> On the pseries platform, access to PCI config space is via RTAS calls(
> which go to the hypervisor) rather than MMIO.  This means we don't use
> the same code path as nearly everyone else which goes through pci_host.c
> and we're missing some of the parameter checking along the way.
> 
> We do have some parameter checking in the RTAS calls, but it's not enough.
> It checks for overruns, but does not check for unaligned accesses,
> oversized accesses (which means the guest could trigger an assertion
> failure from pci_host_config_{read,write}_common().  Worse it doesn't do
> the basic checking for the number of RTAS arguments and results before
> accessing them.
> 
> This patch fixes these bugs.
> 
> Cc: Michael S. Tsirkin <address@hidden>

No objections from me :) But pls note I have no idea about RTAS.

Noted a couple of apparent typos below.

> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/spapr_pci.c |  117 +++++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 79 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index e7ef551..1cf84e7 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -57,26 +57,38 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr,
>  
>  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
>  {
> +    /* This handles the encoding of extended config space addresses */
>      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)
> +static void finish_read_pci_config(sPAPREnvironment *spapr, uint64_t buid,
> +                                   uint32_t addr, uint32_t size,
> +                                   target_ulong rets)
>  {
> -    if ((addr + len) <= limit) {
> -        return pci_host_config_read_common(pci_dev, addr, limit, len);
> -    } else {
> -        return ~0x0;
> +    PCIDevice *pci_dev;
> +    uint32_t val;
> +
> +    if ((size != 1) && (size != 2) && (size != 4)) {
> +        /* access must be 1, 2 oe 4 bytes */

oe -> or?

> +        rtas_st(rets, 0, -1);
> +        return;
>      }
> -}
>  
> -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);
> +    pci_dev = find_dev(spapr, buid, addr);
> +    addr = rtas_pci_cfgaddr(addr);
> +
> +    if (!pci_dev || (addr % size) || (addr >= pci_config_size(pci_dev))) {
> +        /* Access must be to a valid device, within bounds and
> +         * naturally aligned */
> +        rtas_st(rets, 0, -1);
> +        return;
>      }
> +
> +    val = pci_host_config_read_common(pci_dev, addr,
> +                                      pci_config_size(pci_dev), size);
> +
> +    rtas_st(rets, 0, 0);
> +    rtas_st(rets, 1, val);
>  }
>  
>  static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
> @@ -84,19 +96,19 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment 
> *spapr,
>                                       target_ulong args,
>                                       uint32_t nret, target_ulong rets)
>  {
> -    uint32_t val, size, addr;
> -    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> -    PCIDevice *dev = find_dev(spapr, buid, rtas_ld(args, 0));
> +    uint64_t buid;
> +    uint32_t size, addr;
>  
> -    if (!dev) {
> +    if ((nargs != 4) || (nret != 2)) {
>          rtas_st(rets, 0, -1);
>          return;
>      }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>      size = rtas_ld(args, 3);
> -    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
> -    rtas_st(rets, 0, 0);
> -    rtas_st(rets, 1, val);
> +    addr = rtas_ld(args, 0);
> +
> +    finish_read_pci_config(spapr, buid, addr, size, rets);
>  }
>  
>  static void rtas_read_pci_config(sPAPREnvironment *spapr,
> @@ -104,18 +116,45 @@ static void rtas_read_pci_config(sPAPREnvironment 
> *spapr,
>                                   target_ulong args,
>                                   uint32_t nret, target_ulong rets)
>  {
> -    uint32_t val, size, addr;
> -    PCIDevice *dev = find_dev(spapr, 0, rtas_ld(args, 0));
> +    uint32_t size, addr;
>  
> -    if (!dev) {
> +    if ((nargs != 2) || (nret != 2)) {
>          rtas_st(rets, 0, -1);
>          return;
>      }
> +
>      size = rtas_ld(args, 1);
> -    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
> +    addr = rtas_ld(args, 0);
> +
> +    finish_read_pci_config(spapr, 0, addr, size, rets);
> +}
> +
> +static void finish_write_pci_config(sPAPREnvironment *spapr, uint64_t buid,
> +                                    uint32_t addr, uint32_t size,
> +                                    uint32_t val, target_ulong rets)
> +{
> +    PCIDevice *pci_dev;
> +
> +    if ((size != 1) && (size != 2) && (size != 4)) {
> +        /* access must be 1, 2 oe 4 bytes */

oe -> or?

> +        rtas_st(rets, 0, -1);
> +        return;
> +    }
> +
> +    pci_dev = find_dev(spapr, buid, addr);
> +    addr = rtas_pci_cfgaddr(addr);
> +
> +    if (!pci_dev || (addr % size) || (addr >= pci_config_size(pci_dev))) {
> +        /* Access must be to a valid device, within bounds and
> +         * naturally aligned */
> +        rtas_st(rets, 0, -1);
> +        return;
> +    }
> +
> +    pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
> +                                 val, size);
> +
>      rtas_st(rets, 0, 0);
> -    rtas_st(rets, 1, val);
>  }
>  
>  static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
> @@ -123,19 +162,20 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment 
> *spapr,
>                                        target_ulong args,
>                                        uint32_t nret, target_ulong rets)
>  {
> +    uint64_t buid;
>      uint32_t val, size, addr;
> -    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> -    PCIDevice *dev = find_dev(spapr, buid, rtas_ld(args, 0));
>  
> -    if (!dev) {
> +    if ((nargs != 5) || (nret != 1)) {
>          rtas_st(rets, 0, -1);
>          return;
>      }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>      val = rtas_ld(args, 4);
>      size = rtas_ld(args, 3);
> -    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
> -    rtas_st(rets, 0, 0);
> +    addr = rtas_ld(args, 0);
> +
> +    finish_write_pci_config(spapr, buid, addr, size, val, rets);
>  }
>  
>  static void rtas_write_pci_config(sPAPREnvironment *spapr,
> @@ -144,17 +184,18 @@ static void rtas_write_pci_config(sPAPREnvironment 
> *spapr,
>                                    uint32_t nret, target_ulong rets)
>  {
>      uint32_t val, size, addr;
> -    PCIDevice *dev = find_dev(spapr, 0, rtas_ld(args, 0));
>  
> -    if (!dev) {
> +    if ((nargs != 3) || (nret != 1)) {
>          rtas_st(rets, 0, -1);
>          return;
>      }
> +
> +
>      val = rtas_ld(args, 2);
>      size = rtas_ld(args, 1);
> -    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
> -    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
> -    rtas_st(rets, 0, 0);
> +    addr = rtas_ld(args, 0);
> +
> +    finish_write_pci_config(spapr, 0, addr, size, val, rets);
>  }
>  
>  static int pci_spapr_map_irq(PCIDevice *pci_dev, int irq_num)
> -- 
> 1.7.9.1