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 <m...@redhat.com>
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 <da...@gibson.dropbear.id.au>
> ---
> 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
