Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] pseries: Fix RTAS based config access
Am 15.04.2012 12:16, schrieb 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 m...@redhat.com
No objections from me :) But pls note I have no idea about RTAS.
Noted a couple of apparent typos below.
Thanks, applied (with typos fixed) to ppc-next:
http://repo.or.cz/w/qemu/agraf.git/shortlog/refs/heads/ppc-next
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] pseries: Fix RTAS based config access
Am 02.04.2012 06:17, schrieb David Gibson:
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
mst, are you planning to review these two patches? The code movements
and RTAS error handling looks okay to me on brief sight, but I'm no PCI
expert and the two of you were having discussions as to where to do such
checks.
Thanks,
Andreas
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 */
+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,
+
Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] pseries: Fix RTAS based config access
On Thu, Apr 12, 2012 at 02:29:33PM +0200, Andreas Färber wrote:
Am 02.04.2012 06:17, schrieb David Gibson:
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
mst, are you planning to review these two patches? The code movements
and RTAS error handling looks okay to me on brief sight, but I'm no PCI
expert and the two of you were having discussions as to where to do such
checks.
Thanks,
Andreas
I saw a long argument so I was waiting for dust to settle :)
Will try to review next week.
--
MST
Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] pseries: Fix RTAS based config access
On Thu, Apr 12, 2012 at 04:50:46PM +0300, Michael S. Tsirkin wrote:
On Thu, Apr 12, 2012 at 02:29:33PM +0200, Andreas Färber wrote:
Am 02.04.2012 06:17, schrieb David Gibson:
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
mst, are you planning to review these two patches? The code movements
and RTAS error handling looks okay to me on brief sight, but I'm no PCI
expert and the two of you were having discussions as to where to do such
checks.
Thanks,
Andreas
I saw a long argument so I was waiting for dust to settle :)
Well.. this patch is basically my capitulation on that argument. It's
just the minimal bugfix, minus any cleanup / refactoring of the
checking code as I was doing before.
Will try to review next week.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
