[PATCH 0/2] cpu_register_physical_memory() is completely broken.

[PATCH 0/2] cpu_register_physical_memory() is completely broken.

[Gleb Natapov]

Or just a little bit?

Nothing prevents guest from configuring pci mmio bar to overlap system
memory region and the physical memory address will became mmio, but
when guest will change pci bar mapping the physical address location
will not become memory again, but instead it becomes unassigned. Yes,
guest can only hurt itself by doing this, but real HW works different,
so things that may work on real HW will break in qemu.

Anyway attached are two patches that fix more pressing issues: segfault and
abourt() that can be triggered by a guest.

To trigger segfaul run Linux in qemu tcg (or apply patch 2 and then kvm
can be used too) with standard config. In the guest do the following:
# setpci -s 00:03.0 0x14.L=0xc000
# dd if=/dev/zero of=/dev/mem bs=4096 count=1 seek=12


To trigger abort run Linux in qemu with kvm and do:
# setpci -s 00:03.0 0x14.L=0xc000

Gleb Natapov (2):
  Fix segfault in mmio subpage handling code.
  Remove guest triggerable abort()

 exec.c    |    2 ++
 kvm-all.c |   16 ++++------------
 2 files changed, 6 insertions(+), 12 deletions(-)

[PATCH 1/2] Fix segfault in mmio subpage handling code.

[Gleb Natapov]

It is possible that subpage mmio is registered over existing memory
page. When this happens "memory" will have real memory address and not
index into io_mem array so next access to the page will generate
segfault. It is uncommon to have some part of a page to be accessed as
memory and some as mmio, but qemu shouldn't crash even when guest does
stupid things. So lets just pretend that the rest of the page is
unassigned if guest configure part of the memory page as mmio.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 exec.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 5e9a5b7..5945496 100644
--- a/exec.c
+++ b/exec.c
@@ -3363,6 +3363,8 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
            mmio, start, end, idx, eidx, memory);
 #endif
     memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
+    if ((memory & ~TARGET_PAGE_MASK) == IO_MEM_RAM)
+        memory = IO_MEM_UNASSIGNED;
     for (; idx <= eidx; idx++) {
         mmio->sub_io_index[idx] = memory;
         mmio->region_offset[idx] = region_offset;
-- 
1.7.1

[PATCH 2/2] Remove guest triggerable abort()

[Gleb Natapov]

This abort() condition is easily triggerable by a guest if it configures
pci bar with unaligned address that overlaps main memory.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 kvm-all.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index fec6d05..ad46b10 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -437,18 +437,10 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr,
     KVMSlot *mem, old;
     int err;
 
-    if (start_addr & ~TARGET_PAGE_MASK) {
-        if (flags >= IO_MEM_UNASSIGNED) {
-            if (!kvm_lookup_overlapping_slot(s, start_addr,
-                                             start_addr + size)) {
-                return;
-            }
-            fprintf(stderr, "Unaligned split of a KVM memory slot\n");
-        } else {
-            fprintf(stderr, "Only page-aligned memory slots supported\n");
-        }
-        abort();
-    }
+    /* kvm works in page size chunks, but the function may be called
+       with sub-page size and analigned start address. */
+    size = TARGET_PAGE_ALIGN(size);
+    start_addr = TARGET_PAGE_ALIGN(start_addr);
 
     /* KVM does not support read-only slots */
     phys_offset &= ~IO_MEM_ROM;
-- 
1.7.1

Re: [PATCH 1/2] Fix segfault in mmio subpage handling code.

[Gleb Natapov]

Use this one instead.

On Wed, Jul 28, 2010 at 06:13:22PM +0300, Gleb Natapov wrote:
> It is possible that subpage mmio is registered over existing memory
> page. When this happens "memory" will have real memory address and not
> index into io_mem array so next access to the page will generate
> segfault. It is uncommon to have some part of a page to be accessed as
> memory and some as mmio, but qemu shouldn't crash even when guest does
> stupid things. So lets just pretend that the rest of the page is
> unassigned if guest configure part of the memory page as mmio.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>

diff --git a/exec.c b/exec.c
index 5e9a5b7..53483bc 100644
--- a/exec.c
+++ b/exec.c
@@ -3362,6 +3362,8 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
     printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__,
            mmio, start, end, idx, eidx, memory);
 #endif
+    if ((memory & ~TARGET_PAGE_MASK) == IO_MEM_RAM)
+        memory = IO_MEM_UNASSIGNED;
     memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
     for (; idx <= eidx; idx++) {
         mmio->sub_io_index[idx] = memory;
--
			Gleb.

Re: [PATCH 1/2] Fix segfault in mmio subpage handling code.

[Marcelo Tosatti]

On Thu, Jul 29, 2010 at 01:41:45PM +0300, Gleb Natapov wrote:
> Use this one instead.
> 
> On Wed, Jul 28, 2010 at 06:13:22PM +0300, Gleb Natapov wrote:
> > It is possible that subpage mmio is registered over existing memory
> > page. When this happens "memory" will have real memory address and not
> > index into io_mem array so next access to the page will generate
> > segfault. It is uncommon to have some part of a page to be accessed as
> > memory and some as mmio, but qemu shouldn't crash even when guest does
> > stupid things. So lets just pretend that the rest of the page is
> > unassigned if guest configure part of the memory page as mmio.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> 
> diff --git a/exec.c b/exec.c
> index 5e9a5b7..53483bc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3362,6 +3362,8 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>      printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__,
>             mmio, start, end, idx, eidx, memory);
>  #endif
> +    if ((memory & ~TARGET_PAGE_MASK) == IO_MEM_RAM)
> +        memory = IO_MEM_UNASSIGNED;
>      memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
>      for (; idx <= eidx; idx++) {
>          mmio->sub_io_index[idx] = memory;

Looks good to me.

Re: [PATCH 2/2] Remove guest triggerable abort()

[Marcelo Tosatti]

On Wed, Jul 28, 2010 at 06:13:23PM +0300, Gleb Natapov wrote:
> This abort() condition is easily triggerable by a guest if it configures
> pci bar with unaligned address that overlaps main memory.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  kvm-all.c |   16 ++++------------
>  1 files changed, 4 insertions(+), 12 deletions(-)

Applied to uq/master, thanks.

Re: [Qemu-devel] Re: [PATCH 1/2] Fix segfault in mmio subpage handling code.

[Blue Swirl]

Thanks, applied.

On Thu, Jul 29, 2010 at 9:16 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Thu, Jul 29, 2010 at 01:41:45PM +0300, Gleb Natapov wrote:
>> Use this one instead.
>>
>> On Wed, Jul 28, 2010 at 06:13:22PM +0300, Gleb Natapov wrote:
>> > It is possible that subpage mmio is registered over existing memory
>> > page. When this happens "memory" will have real memory address and not
>> > index into io_mem array so next access to the page will generate
>> > segfault. It is uncommon to have some part of a page to be accessed as
>> > memory and some as mmio, but qemu shouldn't crash even when guest does
>> > stupid things. So lets just pretend that the rest of the page is
>> > unassigned if guest configure part of the memory page as mmio.
>> >
>> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>
>> diff --git a/exec.c b/exec.c
>> index 5e9a5b7..53483bc 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3362,6 +3362,8 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>>      printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__,
>>             mmio, start, end, idx, eidx, memory);
>>  #endif
>> +    if ((memory & ~TARGET_PAGE_MASK) == IO_MEM_RAM)
>> +        memory = IO_MEM_UNASSIGNED;
>>      memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
>>      for (; idx <= eidx; idx++) {
>>          mmio->sub_io_index[idx] = memory;
>
> Looks good to me.
>
>
>