[Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Marcel Apfelbaum]

A bug reported by Luiz Capitulino let us to find
several bugs in memory address space setup.

One issue is that gdb stub can give us arbitrary addresses
and we'll try to access them.
Since our lookup ignored high bits in the address,
we hit a wrong section and got a crash.
In fact, PCI devices can access arbitrary addresses too,
so we should just make lookup robust against this case.

Another issue has to do with size of regions.
memory API uses UINT64_MAX so say "all 64 bit" but
some devices mistakenly used INT64_MAX.

It should not affect most systems in practice as
everything should be limited by address space size,
but it's an API misuse that we should not keep around,
and it will become a problem if a system with 64 bit
target address hits this path.

Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is
        the max size for memory regions rendered by exec. 
Patches 2-3 limits the size of memory regions used by exec.c.
Patch 4 fixes an actual bug.
The rest of patches make code cleaner and more robust.


Marcel Apfelbaum (3):
  exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions
    rendered by exec
  hw/alpha: limit iommu-typhoon memory size
  hw/ppc: limit iommu-spapr memory size

Michael S. Tsirkin (4):
  exec: don't ignore high address bits on lookup
  pci: fix address space size for bridge
  exec: don't ignore high address bits on set
  spapr_pci: s/INT64_MAX/UINT64_MAX/

Paolo Bonzini (1):
  pc: s/INT64_MAX/UINT64_MAX/

 exec.c                        | 18 ++++++++++++++++++
 hw/alpha/typhoon.c            |  2 +-
 hw/i386/pc_piix.c             |  2 +-
 hw/i386/pc_q35.c              |  2 +-
 hw/pci/pci_bridge.c           |  2 +-
 hw/ppc/spapr_iommu.c          |  2 +-
 hw/ppc/spapr_pci.c            |  2 +-
 include/exec/address-spaces.h |  4 ++++
 8 files changed, 28 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Marcel Apfelbaum]

The page table logic in exec.c assumes
that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
rendered by exec.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/exec/address-spaces.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
index 3d12cdd..174cc05 100644
--- a/include/exec/address-spaces.h
+++ b/include/exec/address-spaces.h
@@ -23,6 +23,10 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#define TARGET_PHYS_ADDR_SPACE_MAX                         \
+    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
+    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
+
 /* Get the root memory region.  This interface should only be used temporarily
  * until a proper bus interface is available.
  */
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 2/8] hw/alpha: limit iommu-typhoon memory size

[Marcel Apfelbaum]

The page table logic in exec.c assumes
that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
Limit iommu-typhoon accordingly.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/alpha/typhoon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 59e1bb8..ba88dd9 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -888,7 +888,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
 
     /* Host memory as seen from the PCI side, via the IOMMU.  */
     memory_region_init_iommu(&s->pchip.iommu, OBJECT(s), &typhoon_iommu_ops,
-                             "iommu-typhoon", UINT64_MAX);
+                             "iommu-typhoon", TARGET_PHYS_ADDR_SPACE_MAX);
     address_space_init(&s->pchip.iommu_as, &s->pchip.iommu, "pchip0-pci");
     pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 3/8] hw/ppc: limit iommu-spapr memory size

[Marcel Apfelbaum]

The page table logic in exec.c assumes
that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
Limit iommu-spapr accordingly.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/ppc/spapr_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index ef45f4f..3f3f6ea 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -136,7 +136,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
     trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
 
     memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
-                             "iommu-spapr", UINT64_MAX);
+                             "iommu-spapr", TARGET_PHYS_ADDR_SPACE_MAX);
 
     QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 4/8] exec: don't ignore high address bits on lookup

[Marcel Apfelbaum]

From: "Michael S. Tsirkin" <mst@redhat.com>

Lookup of address > target address space should
return an unassigned section, instead of silently
ignoring high bits.

Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 exec.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/exec.c b/exec.c
index 79610ce..aeeaf00 100644
--- a/exec.c
+++ b/exec.c
@@ -197,6 +197,9 @@ static void phys_page_set_level(PhysPageEntry *lp, hwaddr *index,
     }
 }
 
+#define TARGET_PHYS_MAX_ADDR  \
+        (0x1ULL << (TARGET_PHYS_ADDR_SPACE_BITS - TARGET_PAGE_BITS))
+
 static void phys_page_set(AddressSpaceDispatch *d,
                           hwaddr index, hwaddr nb,
                           uint16_t leaf)
@@ -213,6 +216,10 @@ static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr index,
     PhysPageEntry *p;
     int i;
 
+    if (index >= TARGET_PHYS_MAX_ADDR) {
+        return &sections[PHYS_SECTION_UNASSIGNED];
+    }
+
     for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
         if (lp.ptr == PHYS_MAP_NODE_NIL) {
             return &sections[PHYS_SECTION_UNASSIGNED];
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 5/8] pci: fix address space size for bridge

[Marcel Apfelbaum]

From: "Michael S. Tsirkin" <mst@redhat.com>

Address space size for bridge should be full 64 bit,
so we should use UINT64_MAX not INT64_MAX as it's size.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pci_bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 290abab..f72872e 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -372,7 +372,7 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename)
     sec_bus->parent_dev = dev;
     sec_bus->map_irq = br->map_irq ? br->map_irq : pci_swizzle_map_irq_fn;
     sec_bus->address_space_mem = &br->address_space_mem;
-    memory_region_init(&br->address_space_mem, OBJECT(br), "pci_bridge_pci", INT64_MAX);
+    memory_region_init(&br->address_space_mem, OBJECT(br), "pci_bridge_pci", UINT64_MAX);
     sec_bus->address_space_io = &br->address_space_io;
     memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io", 65536);
     br->windows = pci_bridge_region_init(br);
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 6/8] exec: don't ignore high address bits on set

[Marcel Apfelbaum]

From: "Michael S. Tsirkin" <mst@redhat.com>

We should never add pages outside target address space,
we had such bugs in the past so add debug message to catch
that quickly if we do, otherwise at least don't corrupt
the page tables.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 exec.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/exec.c b/exec.c
index aeeaf00..6c3196f 100644
--- a/exec.c
+++ b/exec.c
@@ -52,6 +52,7 @@
 #include "exec/memory-internal.h"
 
 //#define DEBUG_SUBPAGE
+/* #define DEBUG_PHYS_PAGE_SET */
 
 #if !defined(CONFIG_USER_ONLY)
 static int in_migration;
@@ -204,6 +205,16 @@ static void phys_page_set(AddressSpaceDispatch *d,
                           hwaddr index, hwaddr nb,
                           uint16_t leaf)
 {
+    if (nb && (index + nb - 1) >= TARGET_PHYS_MAX_ADDR) {
+#ifdef DEBUG_PHYS_PAGE_SET
+        printf("%s: page table corruption: page address " TARGET_FMT_plx
+               " > max supported address %llx\n", __func__, (index + nb - 1),
+               TARGET_PHYS_MAX_ADDR);
+#endif
+        /* at least don't corrupt the page tables */
+        return;
+    }
+
     /* Wildly overreserve - it doesn't matter much. */
     phys_map_node_reserve(3 * P_L2_LEVELS);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 7/8] pc: s/INT64_MAX/UINT64_MAX/

[Marcel Apfelbaum]

From: Paolo Bonzini <pbonzini@redhat.com>

It doesn't make sense for a region to be INT64_MAX in size:
memory core uses UINT64_MAX as a special value meaning
"all 64 bit" this is what was meant here.

While this should never affect the PC system which at the moment always
has < 63 bit size, this makes us hit all kind of corner case bugs with
sub-pages, so users are probably better off if we just use UINT64_MAX
instead.

Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
Tested-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 2 +-
 hw/i386/pc_q35.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4fdb7b6..8e8d354 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -115,7 +115,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
 
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
-        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
+        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
         rom_memory = pci_memory;
     } else {
         pci_memory = NULL;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4c191d3..ca44e05 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -102,7 +102,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     /* pci enabled */
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
-        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
+        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
         rom_memory = pci_memory;
     } else {
         pci_memory = NULL;
-- 
1.8.3.1

[Qemu-devel] [PATCH for-1.7 v2 8/8] spapr_pci: s/INT64_MAX/UINT64_MAX/

[Marcel Apfelbaum]

From: "Michael S. Tsirkin" <mst@redhat.com>

It doesn't make sense for a region to be INT64_MAX in size:
memory core uses UINT64_MAX as a special value meaning
"all 64 bit" this is what was meant here.

While this should never affect the spapr system which at the moment always
has < 63 bit size, this makes us hit all kind of corner case bugs with
sub-pages, so users are probably better off if we just use UINT64_MAX
instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ppc/spapr_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index edb4cb0..2beedd4 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -555,7 +555,7 @@ static int spapr_phb_init(SysBusDevice *s)
 
     /* Initialize memory regions */
     sprintf(namebuf, "%s.mmio", sphb->dtbusname);
-    memory_region_init(&sphb->memspace, OBJECT(sphb), namebuf, INT64_MAX);
+    memory_region_init(&sphb->memspace, OBJECT(sphb), namebuf, UINT64_MAX);
 
     sprintf(namebuf, "%s.mmio-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->memwindow, OBJECT(sphb),
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Peter Maydell]

On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> The page table logic in exec.c assumes
> that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> rendered by exec.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  include/exec/address-spaces.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index 3d12cdd..174cc05 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -23,6 +23,10 @@
>
>  #ifndef CONFIG_USER_ONLY
>
> +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> +

I think it's worth adding a comment that this is a
size intended for use in memory_region_init() calls and
so follows the odd convention used by that API that
it is a size in bytes with the exception that UINT64_MAX
represents 2^64.

(it follows from this that using the #define anywhere
except in a memory_region_init() call is probably a bug)

-- PMM

Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Alexander Graf]

On 07.11.2013, at 11:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:

> A bug reported by Luiz Capitulino let us to find
> several bugs in memory address space setup.
> 
> One issue is that gdb stub can give us arbitrary addresses
> and we'll try to access them.
> Since our lookup ignored high bits in the address,
> we hit a wrong section and got a crash.
> In fact, PCI devices can access arbitrary addresses too,
> so we should just make lookup robust against this case.
> 
> Another issue has to do with size of regions.
> memory API uses UINT64_MAX so say "all 64 bit" but
> some devices mistakenly used INT64_MAX.
> 
> It should not affect most systems in practice as
> everything should be limited by address space size,
> but it's an API misuse that we should not keep around,
> and it will become a problem if a system with 64 bit
> target address hits this path.
> 
> Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is
>        the max size for memory regions rendered by exec. 
> Patches 2-3 limits the size of memory regions used by exec.c.
> Patch 4 fixes an actual bug.
> The rest of patches make code cleaner and more robust.

ppc bits are:

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 10:49 +0000, Peter Maydell wrote:
> On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The page table logic in exec.c assumes
> > that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> > Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> > rendered by exec.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  include/exec/address-spaces.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> > index 3d12cdd..174cc05 100644
> > --- a/include/exec/address-spaces.h
> > +++ b/include/exec/address-spaces.h
> > @@ -23,6 +23,10 @@
> >
> >  #ifndef CONFIG_USER_ONLY
> >
> > +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> > +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> > +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> > +
> 
> I think it's worth adding a comment that this is a
> size intended for use in memory_region_init() calls and
> so follows the odd convention used by that API that
> it is a size in bytes with the exception that UINT64_MAX
> represents 2^64.
I will add the comment in v3.

Thanks,
Marcel

> 
> (it follows from this that using the #define anywhere
> except in a memory_region_init() call is probably a bug)
> 
> -- PMM

Re: [Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Michael S. Tsirkin]

On Thu, Nov 07, 2013 at 10:49:50AM +0000, Peter Maydell wrote:
> On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The page table logic in exec.c assumes
> > that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> > Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> > rendered by exec.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  include/exec/address-spaces.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> > index 3d12cdd..174cc05 100644
> > --- a/include/exec/address-spaces.h
> > +++ b/include/exec/address-spaces.h
> > @@ -23,6 +23,10 @@
> >
> >  #ifndef CONFIG_USER_ONLY
> >
> > +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> > +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> > +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> > +
> 
> I think it's worth adding a comment that this is a
> size intended for use in memory_region_init() calls and
> so follows the odd convention used by that API that
> it is a size in bytes with the exception that UINT64_MAX
> represents 2^64.
> 
> (it follows from this that using the #define anywhere
> except in a memory_region_init() call is probably a bug)
> 
> -- PMM

BTW how about we change the API to pass in int128?
Not for 1.7 of course.

This will help make sure it's only used for MRs.

-- 
MST

Re: [Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Paolo Bonzini]

Il 07/11/2013 13:04, Michael S. Tsirkin ha scritto:
>> > (it follows from this that using the #define anywhere
>> > except in a memory_region_init() call is probably a bug)
>> > 
>> > -- PMM
> 
> BTW how about we change the API to pass in int128?

The vast majority of memory regions have small sizes, it would add
boilerplate to wrap the size with an int128_make64 everywhere.

Paolo

> Not for 1.7 of course.
> 
> This will help make sure it's only used for MRs.

Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Paolo Bonzini]

Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto:
> A bug reported by Luiz Capitulino let us to find
> several bugs in memory address space setup.
> 
> One issue is that gdb stub can give us arbitrary addresses
> and we'll try to access them.
> Since our lookup ignored high bits in the address,
> we hit a wrong section and got a crash.
> In fact, PCI devices can access arbitrary addresses too,
> so we should just make lookup robust against this case.
> 
> Another issue has to do with size of regions.
> memory API uses UINT64_MAX so say "all 64 bit" but
> some devices mistakenly used INT64_MAX.
> 
> It should not affect most systems in practice as
> everything should be limited by address space size,
> but it's an API misuse that we should not keep around,
> and it will become a problem if a system with 64 bit
> target address hits this path.
> 
> Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is
>         the max size for memory regions rendered by exec. 
> Patches 2-3 limits the size of memory regions used by exec.c.
> Patch 4 fixes an actual bug.
> The rest of patches make code cleaner and more robust.

I think this is wrong.

There is no reason why boards should be using TARGET_PHYS_ADDR_SPACE_MAX
if the PCI address space is 64-bit.  Many files in hw/ do not even have
access to TARGET_PHYS_ADDR_SPACE_MAX, which is only available to files
that are compiled per-target.

The fact that you have to reduce the IOMMU address spaces from 64 bits
(UINT64_MAX, not the buggy INT64_MAX) to something else is a red flag.
If the IOMMU supports 64-bit addressing, the IOMMU regions should have
2^64 bits as their length.  Yes, it works by chance now, but it _does_
work for 2^64-bit size regions: you can do DMA to a high address (say
0xf << 60), the IOMMU tables will convert to a low address that fits
within TARGET_PHYS_ADDR_SPACE_BITS, and the everything will be fine.
Patches 4 and 6 change that.

So, ack for patch 5-7-8, which should also be enough to fix the problem
that Luiz reported.  For everything else the solution is to make memory
dispatch use the whole 64 bits, as in
http://permalink.gmane.org/gmane.comp.emulators.qemu/240229.  If you
want to delay that to 1.8 that's fine.

Paolo

> 
> Marcel Apfelbaum (3):
>   exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions
>     rendered by exec
>   hw/alpha: limit iommu-typhoon memory size
>   hw/ppc: limit iommu-spapr memory size
> 
> Michael S. Tsirkin (4):
>   exec: don't ignore high address bits on lookup
>   pci: fix address space size for bridge
>   exec: don't ignore high address bits on set
>   spapr_pci: s/INT64_MAX/UINT64_MAX/
> 
> Paolo Bonzini (1):
>   pc: s/INT64_MAX/UINT64_MAX/
> 
>  exec.c                        | 18 ++++++++++++++++++
>  hw/alpha/typhoon.c            |  2 +-
>  hw/i386/pc_piix.c             |  2 +-
>  hw/i386/pc_q35.c              |  2 +-
>  hw/pci/pci_bridge.c           |  2 +-
>  hw/ppc/spapr_iommu.c          |  2 +-
>  hw/ppc/spapr_pci.c            |  2 +-
>  include/exec/address-spaces.h |  4 ++++
>  8 files changed, 28 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Michael S. Tsirkin]

On Thu, Nov 07, 2013 at 03:08:35PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto:
> > A bug reported by Luiz Capitulino let us to find
> > several bugs in memory address space setup.
> > 
> > One issue is that gdb stub can give us arbitrary addresses
> > and we'll try to access them.
> > Since our lookup ignored high bits in the address,
> > we hit a wrong section and got a crash.
> > In fact, PCI devices can access arbitrary addresses too,
> > so we should just make lookup robust against this case.
> > 
> > Another issue has to do with size of regions.
> > memory API uses UINT64_MAX so say "all 64 bit" but
> > some devices mistakenly used INT64_MAX.
> > 
> > It should not affect most systems in practice as
> > everything should be limited by address space size,
> > but it's an API misuse that we should not keep around,
> > and it will become a problem if a system with 64 bit
> > target address hits this path.
> > 
> > Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is
> >         the max size for memory regions rendered by exec. 
> > Patches 2-3 limits the size of memory regions used by exec.c.
> > Patch 4 fixes an actual bug.
> > The rest of patches make code cleaner and more robust.
> 
> I think this is wrong.
> 
> There is no reason why boards should be using TARGET_PHYS_ADDR_SPACE_MAX
> if the PCI address space is 64-bit.  Many files in hw/ do not even have
> access to TARGET_PHYS_ADDR_SPACE_MAX, which is only available to files
> that are compiled per-target.
> 
> The fact that you have to reduce the IOMMU address spaces from 64 bits
> (UINT64_MAX, not the buggy INT64_MAX) to something else is a red flag.
> If the IOMMU supports 64-bit addressing, the IOMMU regions should have
> 2^64 bits as their length.  Yes, it works by chance now, but it _does_
> work for 2^64-bit size regions: you can do DMA to a high address (say
> 0xf << 60), the IOMMU tables will convert to a low address that fits
> within TARGET_PHYS_ADDR_SPACE_BITS, and the everything will be fine.
> Patches 4 and 6 change that.
> 
> So, ack for patch 5-7-8, which should also be enough to fix the problem
> that Luiz reported.

Not at all. As long as exec.c ignores high bits, any access
there will end up in the wrong region.
We might not get a crash but we'll get guest memory corruption.
Not good.

>  For everything else the solution is to make memory
> dispatch use the whole 64 bits, as in
> http://permalink.gmane.org/gmane.comp.emulators.qemu/240229.  If you
> want to delay that to 1.8 that's fine.
> 
> Paolo
> 
> > 
> > Marcel Apfelbaum (3):
> >   exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions
> >     rendered by exec
> >   hw/alpha: limit iommu-typhoon memory size
> >   hw/ppc: limit iommu-spapr memory size
> > 
> > Michael S. Tsirkin (4):
> >   exec: don't ignore high address bits on lookup
> >   pci: fix address space size for bridge
> >   exec: don't ignore high address bits on set
> >   spapr_pci: s/INT64_MAX/UINT64_MAX/
> > 
> > Paolo Bonzini (1):
> >   pc: s/INT64_MAX/UINT64_MAX/
> > 
> >  exec.c                        | 18 ++++++++++++++++++
> >  hw/alpha/typhoon.c            |  2 +-
> >  hw/i386/pc_piix.c             |  2 +-
> >  hw/i386/pc_q35.c              |  2 +-
> >  hw/pci/pci_bridge.c           |  2 +-
> >  hw/ppc/spapr_iommu.c          |  2 +-
> >  hw/ppc/spapr_pci.c            |  2 +-
> >  include/exec/address-spaces.h |  4 ++++
> >  8 files changed, 28 insertions(+), 6 deletions(-)
> > 

Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Paolo Bonzini]

Il 07/11/2013 16:05, Michael S. Tsirkin ha scritto:
>> > So, ack for patch 5-7-8, which should also be enough to fix the problem
>> > that Luiz reported.
> Not at all. As long as exec.c ignores high bits, any access
> there will end up in the wrong region.

... unless it happens to be the correct region because the whole
2^64-bit range is covered by a single region.  This is what happens for
IOMMUs.

Paolo

> We might not get a crash but we'll get guest memory corruption.

Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

[Michael S. Tsirkin]

On Thu, Nov 07, 2013 at 04:07:34PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 16:05, Michael S. Tsirkin ha scritto:
> >> > So, ack for patch 5-7-8, which should also be enough to fix the problem
> >> > that Luiz reported.
> > Not at all. As long as exec.c ignores high bits, any access
> > there will end up in the wrong region.
> 
> ... unless it happens to be the correct region because the whole
> 2^64-bit range is covered by a single region.  This is what happens for
> IOMMUs.
> 
> Paolo

Only if you are lucky.
What makes sure ignoring high bits does not hit the e1000 MMIO?
Nothing I think.

> > We might not get a crash but we'll get guest memory corruption.

Re: [Qemu-devel] [PATCH for-1.7 v2 1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

[Michael S. Tsirkin]

On Thu, Nov 07, 2013 at 02:49:06PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 13:04, Michael S. Tsirkin ha scritto:
> >> > (it follows from this that using the #define anywhere
> >> > except in a memory_region_init() call is probably a bug)
> >> > 
> >> > -- PMM
> > 
> > BTW how about we change the API to pass in int128?
> 
> The vast majority of memory regions have small sizes, it would add
> boilerplate to wrap the size with an int128_make64 everywhere.
> 
> Paolo

Well let's have two APIs for 64 bit and for 128 bit.

> > Not for 1.7 of course.
> > 
> > This will help make sure it's only used for MRs.

Re: [Qemu-devel] [PATCH for-1.7 v2 7/8] pc: s/INT64_MAX/UINT64_MAX/

[Paolo Bonzini]

Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> It doesn't make sense for a region to be INT64_MAX in size:
> memory core uses UINT64_MAX as a special value meaning
> "all 64 bit" this is what was meant here.
> 
> While this should never affect the PC system which at the moment always
> has < 63 bit size, this makes us hit all kind of corner case bugs with
> sub-pages, so users are probably better off if we just use UINT64_MAX
> instead.
> 
> Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
> Tested-by: Luiz Capitulino <lcapitulino@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Michael, is this patch (and 8/8: spapr_pci: s/INT64_MAX/UINT64_MAX/)
queued for 1.7 and/or 1.8?

Paolo

> ---
>  hw/i386/pc_piix.c | 2 +-
>  hw/i386/pc_q35.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 4fdb7b6..8e8d354 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -115,7 +115,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>  
>      if (pci_enabled) {
>          pci_memory = g_new(MemoryRegion, 1);
> -        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
> +        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>          rom_memory = pci_memory;
>      } else {
>          pci_memory = NULL;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4c191d3..ca44e05 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -102,7 +102,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      /* pci enabled */
>      if (pci_enabled) {
>          pci_memory = g_new(MemoryRegion, 1);
> -        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
> +        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>          rom_memory = pci_memory;
>      } else {
>          pci_memory = NULL;
> 

Re: [Qemu-devel] [PATCH for-1.7 v2 7/8] pc: s/INT64_MAX/UINT64_MAX/

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 12:44:42PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto:
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > It doesn't make sense for a region to be INT64_MAX in size:
> > memory core uses UINT64_MAX as a special value meaning
> > "all 64 bit" this is what was meant here.
> > 
> > While this should never affect the PC system which at the moment always
> > has < 63 bit size, this makes us hit all kind of corner case bugs with
> > sub-pages, so users are probably better off if we just use UINT64_MAX
> > instead.
> > 
> > Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
> > Tested-by: Luiz Capitulino <lcapitulino@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Michael, is this patch (and 8/8: spapr_pci: s/INT64_MAX/UINT64_MAX/)
> queued for 1.7 and/or 1.8?
> 
> Paolo

I queued this for 1.8 - I don't think it fixes any known specific bugs.
You can see my queue at
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci

> > ---
> >  hw/i386/pc_piix.c | 2 +-
> >  hw/i386/pc_q35.c  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 4fdb7b6..8e8d354 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -115,7 +115,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
> >  
> >      if (pci_enabled) {
> >          pci_memory = g_new(MemoryRegion, 1);
> > -        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
> > +        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> >          rom_memory = pci_memory;
> >      } else {
> >          pci_memory = NULL;
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 4c191d3..ca44e05 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -102,7 +102,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> >      /* pci enabled */
> >      if (pci_enabled) {
> >          pci_memory = g_new(MemoryRegion, 1);
> > -        memory_region_init(pci_memory, NULL, "pci", INT64_MAX);
> > +        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> >          rom_memory = pci_memory;
> >      } else {
> >          pci_memory = NULL;
> >