[Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

This patchset fixes CVE-2014-7840: invalid
migration stream can cause arbitrary qemu memory
overwrite.
First patch includes the minimal fix for the issue.
Follow-up patches on top add extra checking to reduce the
chance this kind of bug recurs.

Note: these are already (tentatively-pending review)
queued in my tree, so only review/ack
is necessary.

Michael S. Tsirkin (4):
  migration: fix parameter validation on ram load
  exec: add wrapper for host pointer access
  cpu: assert host pointer offset within block
  cpu: verify that block->host is set

 include/exec/cpu-all.h |  7 +++++++
 arch_init.c            |  5 +++--
 exec.c                 | 10 +++++-----
 3 files changed, 15 insertions(+), 7 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/4] migration: fix parameter validation on ram load

[Michael S. Tsirkin]

During migration, the values read from migration stream during ram load
are not validated. Especially offset in host_from_stream_offset() and
also the length of the writes in the callers of said function.

To fix this, we need to make sure that the [offset, offset + length]
range fits into one of the allocated memory regions.

Validating addr < len should be sufficient since data seems to always be
managed in TARGET_PAGE_SIZE chunks.

Fixes: CVE-2014-7840

Note: follow-up patches add extra checks on each block->host access.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch_init.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 88a5ba0..593a990 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1006,7 +1006,7 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     uint8_t len;
 
     if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || block->length <= offset) {
             error_report("Ack, bad migration stream!");
             return NULL;
         }
@@ -1019,8 +1019,9 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     id[len] = 0;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
+        if (!strncmp(id, block->idstr, sizeof(id)) && block->length > offset) {
             return memory_region_get_ram_ptr(block->mr) + offset;
+        }
     }
 
     error_report("Can't find block %s!", id);
-- 
MST

Re: [Qemu-devel] [PATCH 1/4] migration: fix parameter validation on ram load

[Paolo Bonzini]

On 12/11/2014 10:44, Michael S. Tsirkin wrote:
> During migration, the values read from migration stream during ram load
> are not validated. Especially offset in host_from_stream_offset() and
> also the length of the writes in the callers of said function.
> 
> To fix this, we need to make sure that the [offset, offset + length]
> range fits into one of the allocated memory regions.
> 
> Validating addr < len should be sufficient since data seems to always be
> managed in TARGET_PAGE_SIZE chunks.
> 
> Fixes: CVE-2014-7840
> 
> Note: follow-up patches add extra checks on each block->host access.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch_init.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 88a5ba0..593a990 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1006,7 +1006,7 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      uint8_t len;
>  
>      if (flags & RAM_SAVE_FLAG_CONTINUE) {
> -        if (!block) {
> +        if (!block || block->length <= offset) {
>              error_report("Ack, bad migration stream!");
>              return NULL;
>          }
> @@ -1019,8 +1019,9 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      id[len] = 0;
>  
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> -        if (!strncmp(id, block->idstr, sizeof(id)))
> +        if (!strncmp(id, block->idstr, sizeof(id)) && block->length > offset) {
>              return memory_region_get_ram_ptr(block->mr) + offset;
> +        }
>      }
>  
>      error_report("Can't find block %s!", id);
> 

Sort-of Yoda conditionals, i.e. I think "offset >= block->length" and
"offset < block->length" would have been more common and indeed that's
what you use in patch 3.  That's the only comment I have.

Series

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 2/4] exec: add wrapper for host pointer access

[Michael S. Tsirkin]

host pointer accesses force pointer math, let's
add a wrapper to make them safer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/exec/cpu-all.h |  5 +++++
 exec.c                 | 10 +++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index c085804..9d8d408 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -313,6 +313,11 @@ typedef struct RAMBlock {
     int fd;
 } RAMBlock;
 
+static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
+{
+    return (char *)block->host + offset;
+}
+
 typedef struct RAMList {
     QemuMutex mutex;
     /* Protected by the iothread lock.  */
diff --git a/exec.c b/exec.c
index ad5cf12..9648669 100644
--- a/exec.c
+++ b/exec.c
@@ -840,7 +840,7 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
 
     block = qemu_get_ram_block(start);
     assert(block == qemu_get_ram_block(end - 1));
-    start1 = (uintptr_t)block->host + (start - block->offset);
+    start1 = (uintptr_t)ramblock_ptr(block, start - block->offset);
     cpu_tlb_reset_dirty_all(start1, length);
 }
 
@@ -1500,7 +1500,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
         offset = addr - block->offset;
         if (offset < block->length) {
-            vaddr = block->host + offset;
+            vaddr = ramblock_ptr(block, offset);
             if (block->flags & RAM_PREALLOC) {
                 ;
             } else if (xen_enabled()) {
@@ -1551,7 +1551,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
 {
     RAMBlock *block = qemu_get_ram_block(addr);
 
-    return block->host;
+    return ramblock_ptr(block, 0);
 }
 
 /* Return a host pointer to ram allocated with qemu_ram_alloc.
@@ -1578,7 +1578,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
                 xen_map_cache(block->offset, block->length, 1);
         }
     }
-    return block->host + (addr - block->offset);
+    return ramblock_ptr(block, addr - block->offset);
 }
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
@@ -1597,7 +1597,7 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
             if (addr - block->offset < block->length) {
                 if (addr - block->offset + *size > block->length)
                     *size = block->length - addr + block->offset;
-                return block->host + (addr - block->offset);
+                return ramblock_ptr(block, addr - block->offset);
             }
         }
 
-- 
MST

Re: [Qemu-devel] [PATCH 2/4] exec: add wrapper for host pointer access

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> host pointer accesses force pointer math, let's
> add a wrapper to make them safer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/exec/cpu-all.h |  5 +++++
>  exec.c                 | 10 +++++-----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index c085804..9d8d408 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -313,6 +313,11 @@ typedef struct RAMBlock {
>      int fd;
>  } RAMBlock;
>  
> +static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
> +{
> +    return (char *)block->host + offset;
> +}

I'm a bit surprised you don't need to pass a length to this to be able
to tell how much you can access.

>  typedef struct RAMList {
>      QemuMutex mutex;
>      /* Protected by the iothread lock.  */
> diff --git a/exec.c b/exec.c
> index ad5cf12..9648669 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -840,7 +840,7 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
>  
>      block = qemu_get_ram_block(start);
>      assert(block == qemu_get_ram_block(end - 1));
> -    start1 = (uintptr_t)block->host + (start - block->offset);
> +    start1 = (uintptr_t)ramblock_ptr(block, start - block->offset);
>      cpu_tlb_reset_dirty_all(start1, length);
>  }
>  
> @@ -1500,7 +1500,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>          offset = addr - block->offset;
>          if (offset < block->length) {
> -            vaddr = block->host + offset;
> +            vaddr = ramblock_ptr(block, offset);
>              if (block->flags & RAM_PREALLOC) {
>                  ;
>              } else if (xen_enabled()) {
> @@ -1551,7 +1551,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
>  {
>      RAMBlock *block = qemu_get_ram_block(addr);
>  
> -    return block->host;
> +    return ramblock_ptr(block, 0);
>  }
>  
>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
> @@ -1578,7 +1578,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>                  xen_map_cache(block->offset, block->length, 1);
>          }
>      }
> -    return block->host + (addr - block->offset);
> +    return ramblock_ptr(block, addr - block->offset);
>  }

which then makes me wonder if all the uses of this are safe near the
end of the block.

>  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> @@ -1597,7 +1597,7 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
>              if (addr - block->offset < block->length) {
>                  if (addr - block->offset + *size > block->length)
>                      *size = block->length - addr + block->offset;
> -                return block->host + (addr - block->offset);
> +                return ramblock_ptr(block, addr - block->offset);
>              }

but then this sounds like it's going to have partial duplication, it already looks
like it's only going to succeed if it finds itself a block that the access fits
in.


Dave

>          }
>  
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] exec: add wrapper for host pointer access

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 10:58:53AM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > host pointer accesses force pointer math, let's
> > add a wrapper to make them safer.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/exec/cpu-all.h |  5 +++++
> >  exec.c                 | 10 +++++-----
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > index c085804..9d8d408 100644
> > --- a/include/exec/cpu-all.h
> > +++ b/include/exec/cpu-all.h
> > @@ -313,6 +313,11 @@ typedef struct RAMBlock {
> >      int fd;
> >  } RAMBlock;
> >  
> > +static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
> > +{
> > +    return (char *)block->host + offset;
> > +}
> 
> I'm a bit surprised you don't need to pass a length to this to be able
> to tell how much you can access.

This is because at the moment all accesses only touch a single page.
Said assumption seems to be made all over the code, and won't
be easy to remove.

> >  typedef struct RAMList {
> >      QemuMutex mutex;
> >      /* Protected by the iothread lock.  */
> > diff --git a/exec.c b/exec.c
> > index ad5cf12..9648669 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -840,7 +840,7 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
> >  
> >      block = qemu_get_ram_block(start);
> >      assert(block == qemu_get_ram_block(end - 1));
> > -    start1 = (uintptr_t)block->host + (start - block->offset);
> > +    start1 = (uintptr_t)ramblock_ptr(block, start - block->offset);
> >      cpu_tlb_reset_dirty_all(start1, length);
> >  }
> >  
> > @@ -1500,7 +1500,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
> >      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> >          offset = addr - block->offset;
> >          if (offset < block->length) {
> > -            vaddr = block->host + offset;
> > +            vaddr = ramblock_ptr(block, offset);
> >              if (block->flags & RAM_PREALLOC) {
> >                  ;
> >              } else if (xen_enabled()) {
> > @@ -1551,7 +1551,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> >  {
> >      RAMBlock *block = qemu_get_ram_block(addr);
> >  
> > -    return block->host;
> > +    return ramblock_ptr(block, 0);
> >  }
> >  
> >  /* Return a host pointer to ram allocated with qemu_ram_alloc.
> > @@ -1578,7 +1578,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
> >                  xen_map_cache(block->offset, block->length, 1);
> >          }
> >      }
> > -    return block->host + (addr - block->offset);
> > +    return ramblock_ptr(block, addr - block->offset);
> >  }
> 
> which then makes me wonder if all the uses of this are safe near the
> end of the block.
> 
> >  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> > @@ -1597,7 +1597,7 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
> >              if (addr - block->offset < block->length) {
> >                  if (addr - block->offset + *size > block->length)
> >                      *size = block->length - addr + block->offset;
> > -                return block->host + (addr - block->offset);
> > +                return ramblock_ptr(block, addr - block->offset);
> >              }
> 
> but then this sounds like it's going to have partial duplication, it already looks
> like it's only going to succeed if it finds itself a block that the access fits
> in.
> 
> 
> Dave

Sorry, I don't really understand what you are saying here.

> >          }
> >  
> > -- 
> > MST
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] exec: add wrapper for host pointer access

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Mon, Nov 17, 2014 at 10:58:53AM +0000, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > host pointer accesses force pointer math, let's
> > > add a wrapper to make them safer.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/exec/cpu-all.h |  5 +++++
> > >  exec.c                 | 10 +++++-----
> > >  2 files changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > > index c085804..9d8d408 100644
> > > --- a/include/exec/cpu-all.h
> > > +++ b/include/exec/cpu-all.h
> > > @@ -313,6 +313,11 @@ typedef struct RAMBlock {
> > >      int fd;
> > >  } RAMBlock;
> > >  
> > > +static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
> > > +{
> > > +    return (char *)block->host + offset;
> > > +}
> > 
> > I'm a bit surprised you don't need to pass a length to this to be able
> > to tell how much you can access.
> 
> This is because at the moment all accesses only touch a single page.
> Said assumption seems to be made all over the code, and won't
> be easy to remove.
> 
> > >  typedef struct RAMList {
> > >      QemuMutex mutex;
> > >      /* Protected by the iothread lock.  */
> > > diff --git a/exec.c b/exec.c
> > > index ad5cf12..9648669 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -840,7 +840,7 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
> > >  
> > >      block = qemu_get_ram_block(start);
> > >      assert(block == qemu_get_ram_block(end - 1));
> > > -    start1 = (uintptr_t)block->host + (start - block->offset);
> > > +    start1 = (uintptr_t)ramblock_ptr(block, start - block->offset);
> > >      cpu_tlb_reset_dirty_all(start1, length);
> > >  }
> > >  
> > > @@ -1500,7 +1500,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
> > >      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > >          offset = addr - block->offset;
> > >          if (offset < block->length) {
> > > -            vaddr = block->host + offset;
> > > +            vaddr = ramblock_ptr(block, offset);
> > >              if (block->flags & RAM_PREALLOC) {
> > >                  ;
> > >              } else if (xen_enabled()) {
> > > @@ -1551,7 +1551,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> > >  {
> > >      RAMBlock *block = qemu_get_ram_block(addr);
> > >  
> > > -    return block->host;
> > > +    return ramblock_ptr(block, 0);
> > >  }
> > >  
> > >  /* Return a host pointer to ram allocated with qemu_ram_alloc.
> > > @@ -1578,7 +1578,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
> > >                  xen_map_cache(block->offset, block->length, 1);
> > >          }
> > >      }
> > > -    return block->host + (addr - block->offset);
> > > +    return ramblock_ptr(block, addr - block->offset);
> > >  }
> > 
> > which then makes me wonder if all the uses of this are safe near the
> > end of the block.
> > 
> > >  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> > > @@ -1597,7 +1597,7 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
> > >              if (addr - block->offset < block->length) {
> > >                  if (addr - block->offset + *size > block->length)
> > >                      *size = block->length - addr + block->offset;
> > > -                return block->host + (addr - block->offset);
> > > +                return ramblock_ptr(block, addr - block->offset);
> > >              }
> > 
> > but then this sounds like it's going to have partial duplication, it already looks
> > like it's only going to succeed if it finds itself a block that the access fits
> > in.
> > 
> > 
> > Dave
> 
> Sorry, I don't really understand what you are saying here.

qemu_ram_ptr_length already does some checks, so using ramblock_ptr is duplicating
some of that; not a big issue.

Dave

> 
> > >          }
> > >  
> > > -- 
> > > MST
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] exec: add wrapper for host pointer access

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 12:59:44PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Mon, Nov 17, 2014 at 10:58:53AM +0000, Dr. David Alan Gilbert wrote:
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > > host pointer accesses force pointer math, let's
> > > > add a wrapper to make them safer.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/exec/cpu-all.h |  5 +++++
> > > >  exec.c                 | 10 +++++-----
> > > >  2 files changed, 10 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > > > index c085804..9d8d408 100644
> > > > --- a/include/exec/cpu-all.h
> > > > +++ b/include/exec/cpu-all.h
> > > > @@ -313,6 +313,11 @@ typedef struct RAMBlock {
> > > >      int fd;
> > > >  } RAMBlock;
> > > >  
> > > > +static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
> > > > +{
> > > > +    return (char *)block->host + offset;
> > > > +}
> > > 
> > > I'm a bit surprised you don't need to pass a length to this to be able
> > > to tell how much you can access.
> > 
> > This is because at the moment all accesses only touch a single page.
> > Said assumption seems to be made all over the code, and won't
> > be easy to remove.
> > 
> > > >  typedef struct RAMList {
> > > >      QemuMutex mutex;
> > > >      /* Protected by the iothread lock.  */
> > > > diff --git a/exec.c b/exec.c
> > > > index ad5cf12..9648669 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -840,7 +840,7 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
> > > >  
> > > >      block = qemu_get_ram_block(start);
> > > >      assert(block == qemu_get_ram_block(end - 1));
> > > > -    start1 = (uintptr_t)block->host + (start - block->offset);
> > > > +    start1 = (uintptr_t)ramblock_ptr(block, start - block->offset);
> > > >      cpu_tlb_reset_dirty_all(start1, length);
> > > >  }
> > > >  
> > > > @@ -1500,7 +1500,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
> > > >      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > > >          offset = addr - block->offset;
> > > >          if (offset < block->length) {
> > > > -            vaddr = block->host + offset;
> > > > +            vaddr = ramblock_ptr(block, offset);
> > > >              if (block->flags & RAM_PREALLOC) {
> > > >                  ;
> > > >              } else if (xen_enabled()) {
> > > > @@ -1551,7 +1551,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> > > >  {
> > > >      RAMBlock *block = qemu_get_ram_block(addr);
> > > >  
> > > > -    return block->host;
> > > > +    return ramblock_ptr(block, 0);
> > > >  }
> > > >  
> > > >  /* Return a host pointer to ram allocated with qemu_ram_alloc.
> > > > @@ -1578,7 +1578,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
> > > >                  xen_map_cache(block->offset, block->length, 1);
> > > >          }
> > > >      }
> > > > -    return block->host + (addr - block->offset);
> > > > +    return ramblock_ptr(block, addr - block->offset);
> > > >  }
> > > 
> > > which then makes me wonder if all the uses of this are safe near the
> > > end of the block.
> > > 
> > > >  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> > > > @@ -1597,7 +1597,7 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
> > > >              if (addr - block->offset < block->length) {
> > > >                  if (addr - block->offset + *size > block->length)
> > > >                      *size = block->length - addr + block->offset;
> > > > -                return block->host + (addr - block->offset);
> > > > +                return ramblock_ptr(block, addr - block->offset);
> > > >              }
> > > 
> > > but then this sounds like it's going to have partial duplication, it already looks
> > > like it's only going to succeed if it finds itself a block that the access fits
> > > in.
> > > 
> > > 
> > > Dave
> > 
> > Sorry, I don't really understand what you are saying here.
> 
> qemu_ram_ptr_length already does some checks, so using ramblock_ptr is duplicating
> some of that; not a big issue.
> 
> Dave

Yep.
Since the point is hardening, it's probably a good idea
to keep it simple - and data path shouldn't use ram_addr_t.


> > 
> > > >          }
> > > >  
> > > > -- 
> > > > MST
> > > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 3/4] cpu: assert host pointer offset within block

[Michael S. Tsirkin]

Make accesses safer in case we missed some
check somewhere.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/exec/cpu-all.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 9d8d408..7c3a5e7 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -315,6 +315,7 @@ typedef struct RAMBlock {
 
 static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
 {
+    assert(offset < block->length);
     return (char *)block->host + offset;
 }
 
-- 
MST

[Qemu-devel] [PATCH 4/4] cpu: verify that block->host is set

[Michael S. Tsirkin]

If it isn't, access at an offset will cause memory corruption.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/exec/cpu-all.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 7c3a5e7..62f5581 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -316,6 +316,7 @@ typedef struct RAMBlock {
 static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
 {
     assert(offset < block->length);
+    assert(block->host);
     return (char *)block->host + offset;
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> This patchset fixes CVE-2014-7840: invalid
> migration stream can cause arbitrary qemu memory
> overwrite.
> First patch includes the minimal fix for the issue.
> Follow-up patches on top add extra checking to reduce the
> chance this kind of bug recurs.
> 
> Note: these are already (tentatively-pending review)
> queued in my tree, so only review/ack
> is necessary.

Why not let this go in via the migration tree?


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > This patchset fixes CVE-2014-7840: invalid
> > migration stream can cause arbitrary qemu memory
> > overwrite.
> > First patch includes the minimal fix for the issue.
> > Follow-up patches on top add extra checking to reduce the
> > chance this kind of bug recurs.
> > 
> > Note: these are already (tentatively-pending review)
> > queued in my tree, so only review/ack
> > is necessary.
> 
> Why not let this go in via the migration tree?
> 
> 
> 		Amit

Well I Cc'd Juan and David, so if they had a problem with this, I expect
they'd complain.  David acked so I assume it's ok.  Since I wasted time
testing this and have it on my tree already, might as well just merge.

Which reminds me: we really should have someone in MAINTAINERS
for migration-related files.

-- 
MST

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > This patchset fixes CVE-2014-7840: invalid
> > > migration stream can cause arbitrary qemu memory
> > > overwrite.
> > > First patch includes the minimal fix for the issue.
> > > Follow-up patches on top add extra checking to reduce the
> > > chance this kind of bug recurs.
> > > 
> > > Note: these are already (tentatively-pending review)
> > > queued in my tree, so only review/ack
> > > is necessary.
> > 
> > Why not let this go in via the migration tree?
> 
> Well I Cc'd Juan and David, so if they had a problem with this, I expect
> they'd complain.  David acked so I assume it's ok.  Since I wasted time
> testing this and have it on my tree already, might as well just merge.

IMO asking as a courtesy would've been better than just stating it.

> Which reminds me: we really should have someone in MAINTAINERS
> for migration-related files.

There is, since last week.


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > This patchset fixes CVE-2014-7840: invalid
> > > > migration stream can cause arbitrary qemu memory
> > > > overwrite.
> > > > First patch includes the minimal fix for the issue.
> > > > Follow-up patches on top add extra checking to reduce the
> > > > chance this kind of bug recurs.
> > > > 
> > > > Note: these are already (tentatively-pending review)
> > > > queued in my tree, so only review/ack
> > > > is necessary.
> > > 
> > > Why not let this go in via the migration tree?
> > 
> > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > testing this and have it on my tree already, might as well just merge.
> 
> IMO asking as a courtesy would've been better than just stating it.

Right, thanks for reminding me.

BTW, there is actually a good reason to special-case it: it's a CVE fix,
which I handle.  So they stay on my private queue and are passed
to vendors so vendors can fix downstreams, until making fix public is
cleared with all reporters and vendors.
After reporting is cleared, I try to collect acks but don't normally route
patches through separate queues - that would make it harder to
track the status which we need for CVEs.

I guess this specific one actually is well contained, so it could go in
through a specific tree if it had to.  In fact, it is still possible if
Juan says he prefers it so: I only expect to send pull request around
tomorrow or the day after that.

> > Which reminds me: we really should have someone in MAINTAINERS
> > for migration-related files.
> 
> There is, since last week.
> 
> 
> 		Amit

That's good. I see Juan is listed there now, so all's well.

-- 
MST

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > migration stream can cause arbitrary qemu memory
> > > > > overwrite.
> > > > > First patch includes the minimal fix for the issue.
> > > > > Follow-up patches on top add extra checking to reduce the
> > > > > chance this kind of bug recurs.
> > > > > 
> > > > > Note: these are already (tentatively-pending review)
> > > > > queued in my tree, so only review/ack
> > > > > is necessary.
> > > > 
> > > > Why not let this go in via the migration tree?
> > > 
> > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > testing this and have it on my tree already, might as well just merge.
> > 
> > IMO asking as a courtesy would've been better than just stating it.
> 
> Right, thanks for reminding me.
> 
> BTW, there is actually a good reason to special-case it: it's a CVE fix,
> which I handle.  So they stay on my private queue and are passed
> to vendors so vendors can fix downstreams, until making fix public is
> cleared with all reporters and vendors.
> After reporting is cleared, I try to collect acks but don't normally route
> patches through separate queues - that would make it harder to
> track the status which we need for CVEs.

Patch is public, so all of this doesn't really matter.

But: involving maintainers in their areas, even if the patch is
embargoed, should be a pre-requisite.  I'm not sure if we're doing
that, but please do that so patches get a proper review from the
maintainers.

> I guess this specific one actually is well contained, so it could go in
> through a specific tree if it had to.  In fact, it is still possible if
> Juan says he prefers it so: I only expect to send pull request around
> tomorrow or the day after that.

I'm sure we prefer migration patches go through the migration tree.

Also, this week I'm looking at the migration queue -- it's an
unofficial split of maintenance duties between Juan and me while we're
still trying to find out what works best.

> > > Which reminds me: we really should have someone in MAINTAINERS
> > > for migration-related files.
> > 
> > There is, since last week.
> 
> That's good. I see Juan is listed there now, so all's well.

But that was well-known anyway :-)


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 04:37:50PM +0530, Amit Shah wrote:
> On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> > On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > > migration stream can cause arbitrary qemu memory
> > > > > > overwrite.
> > > > > > First patch includes the minimal fix for the issue.
> > > > > > Follow-up patches on top add extra checking to reduce the
> > > > > > chance this kind of bug recurs.
> > > > > > 
> > > > > > Note: these are already (tentatively-pending review)
> > > > > > queued in my tree, so only review/ack
> > > > > > is necessary.
> > > > > 
> > > > > Why not let this go in via the migration tree?
> > > > 
> > > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > > testing this and have it on my tree already, might as well just merge.
> > > 
> > > IMO asking as a courtesy would've been better than just stating it.
> > 
> > Right, thanks for reminding me.
> > 
> > BTW, there is actually a good reason to special-case it: it's a CVE fix,
> > which I handle.  So they stay on my private queue and are passed
> > to vendors so vendors can fix downstreams, until making fix public is
> > cleared with all reporters and vendors.
> > After reporting is cleared, I try to collect acks but don't normally route
> > patches through separate queues - that would make it harder to
> > track the status which we need for CVEs.
> 
> Patch is public, so all of this doesn't really matter.
> 
> But: involving maintainers in their areas, even if the patch is
> embargoed, should be a pre-requisite.  I'm not sure if we're doing
> that, but please do that so patches get a proper review from the
> maintainers.

Involving more people means more back and forth with reporters which
must approve any disclosure.  If the issue isn't clear, I do involve
maintainers.  I send patches on list and try to merge them only after
they get ack from relevant people. I'm sorry, but this is as far as I
have the time to go.

> > I guess this specific one actually is well contained, so it could go in
> > through a specific tree if it had to.  In fact, it is still possible if
> > Juan says he prefers it so: I only expect to send pull request around
> > tomorrow or the day after that.
> 
> I'm sure we prefer migration patches go through the migration tree.

I'm sorry - I disagree. maintainer boundaries in qemu are quite flexible
because code is somewhat monolithic.  We prefer that patches are
reviewed by people that are familiar with it, that is for sure.  Which
tree is definitely secondary.  If there's a conflict, we can resolve it.
I doubt it will happen here.

> Also, this week I'm looking at the migration queue -- it's an
> unofficial split of maintenance duties between Juan and me while we're
> still trying to find out what works best.
> 

I don't know how am I supposed to know that.
Send patch to MAINTAINERS or something?

> > > > Which reminds me: we really should have someone in MAINTAINERS
> > > > for migration-related files.
> > > 
> > > There is, since last week.
> > 
> > That's good. I see Juan is listed there now, so all's well.
> 
> But that was well-known anyway :-)
> 
> 
> 		Amit

-- 
MST

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 04:37:50PM +0530, Amit Shah wrote:
> > On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> > > On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > > > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > > > migration stream can cause arbitrary qemu memory
> > > > > > > overwrite.
> > > > > > > First patch includes the minimal fix for the issue.
> > > > > > > Follow-up patches on top add extra checking to reduce the
> > > > > > > chance this kind of bug recurs.
> > > > > > > 
> > > > > > > Note: these are already (tentatively-pending review)
> > > > > > > queued in my tree, so only review/ack
> > > > > > > is necessary.
> > > > > > 
> > > > > > Why not let this go in via the migration tree?
> > > > > 
> > > > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > > > testing this and have it on my tree already, might as well just merge.
> > > > 
> > > > IMO asking as a courtesy would've been better than just stating it.
> > > 
> > > Right, thanks for reminding me.
> > > 
> > > BTW, there is actually a good reason to special-case it: it's a CVE fix,
> > > which I handle.  So they stay on my private queue and are passed
> > > to vendors so vendors can fix downstreams, until making fix public is
> > > cleared with all reporters and vendors.
> > > After reporting is cleared, I try to collect acks but don't normally route
> > > patches through separate queues - that would make it harder to
> > > track the status which we need for CVEs.
> > 
> > Patch is public, so all of this doesn't really matter.
> > 
> > But: involving maintainers in their areas, even if the patch is
> > embargoed, should be a pre-requisite.  I'm not sure if we're doing
> > that, but please do that so patches get a proper review from the
> > maintainers.
> 
> Involving more people means more back and forth with reporters which
> must approve any disclosure.  If the issue isn't clear, I do involve
> maintainers.  I send patches on list and try to merge them only after
> they get ack from relevant people. I'm sorry, but this is as far as I
> have the time to go.

The other aspect of the thing is sub-optimal, or patches with bugs,
get pushed in, because the maintainers didn't get involved.  Also,
maintainers don't like code being changed behind their backs...

But if you're overloaded with handling security issues, we can help
identify a co-maintainer as well.

> > > I guess this specific one actually is well contained, so it could go in
> > > through a specific tree if it had to.  In fact, it is still possible if
> > > Juan says he prefers it so: I only expect to send pull request around
> > > tomorrow or the day after that.
> > 
> > I'm sure we prefer migration patches go through the migration tree.
> 
> I'm sorry - I disagree. maintainer boundaries in qemu are quite flexible
> because code is somewhat monolithic.  We prefer that patches are
> reviewed by people that are familiar with it, that is for sure.  Which
> tree is definitely secondary.  If there's a conflict, we can resolve it.
> I doubt it will happen here.

For well-maintained areas, I'm not sure I agree with the flexibility
argument.  Juan has been maintaining the migration tree for a long
while now.

> > Also, this week I'm looking at the migration queue -- it's an
> > unofficial split of maintenance duties between Juan and me while we're
> > still trying to find out what works best.
> > 
> 
> I don't know how am I supposed to know that.

Right now you don't need to -- I just pick patches up and pass them on
to Juan, who does the pull req.

> Send patch to MAINTAINERS or something?

I'm going to add myself to migration maintainers, yes.  But that's
secondary; the pull reqs still go via Juan.


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Mon, Nov 17, 2014 at 05:50:34PM +0530, Amit Shah wrote:
> On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote:
> > On Mon, Nov 17, 2014 at 04:37:50PM +0530, Amit Shah wrote:
> > > On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> > > > On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > > > > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > > > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > > > > migration stream can cause arbitrary qemu memory
> > > > > > > > overwrite.
> > > > > > > > First patch includes the minimal fix for the issue.
> > > > > > > > Follow-up patches on top add extra checking to reduce the
> > > > > > > > chance this kind of bug recurs.
> > > > > > > > 
> > > > > > > > Note: these are already (tentatively-pending review)
> > > > > > > > queued in my tree, so only review/ack
> > > > > > > > is necessary.
> > > > > > > 
> > > > > > > Why not let this go in via the migration tree?
> > > > > > 
> > > > > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > > > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > > > > testing this and have it on my tree already, might as well just merge.
> > > > > 
> > > > > IMO asking as a courtesy would've been better than just stating it.
> > > > 
> > > > Right, thanks for reminding me.
> > > > 
> > > > BTW, there is actually a good reason to special-case it: it's a CVE fix,
> > > > which I handle.  So they stay on my private queue and are passed
> > > > to vendors so vendors can fix downstreams, until making fix public is
> > > > cleared with all reporters and vendors.
> > > > After reporting is cleared, I try to collect acks but don't normally route
> > > > patches through separate queues - that would make it harder to
> > > > track the status which we need for CVEs.
> > > 
> > > Patch is public, so all of this doesn't really matter.
> > > 
> > > But: involving maintainers in their areas, even if the patch is
> > > embargoed, should be a pre-requisite.  I'm not sure if we're doing
> > > that, but please do that so patches get a proper review from the
> > > maintainers.
> > 
> > Involving more people means more back and forth with reporters which
> > must approve any disclosure.  If the issue isn't clear, I do involve
> > maintainers.  I send patches on list and try to merge them only after
> > they get ack from relevant people. I'm sorry, but this is as far as I
> > have the time to go.
> 
> The other aspect of the thing is sub-optimal, or patches with bugs,
> get pushed in, because the maintainers didn't get involved.  

Patches don't get merged before they are on list for a while.
I typically ping people if I don't get acks.

> Also,
> maintainers don't like code being changed behind their backs...

no one is changing code in secret here.
So don't turn your back - review patches :0

FYI I'm sometimes merging patches on list that don't get reviews for
weeks - if they seem to make sense.

> But if you're overloaded with handling security issues, we can help
> identify a co-maintainer as well.

Sure. I don't think we need more process to slow down the flow of
patches even more.

> > > > I guess this specific one actually is well contained, so it could go in
> > > > through a specific tree if it had to.  In fact, it is still possible if
> > > > Juan says he prefers it so: I only expect to send pull request around
> > > > tomorrow or the day after that.
> > > 
> > > I'm sure we prefer migration patches go through the migration tree.
> > 
> > I'm sorry - I disagree. maintainer boundaries in qemu are quite flexible
> > because code is somewhat monolithic.  We prefer that patches are
> > reviewed by people that are familiar with it, that is for sure.  Which
> > tree is definitely secondary.  If there's a conflict, we can resolve it.
> > I doubt it will happen here.
> 
> For well-maintained areas, I'm not sure I agree with the flexibility
> argument.  Juan has been maintaining the migration tree for a long
> while now.

Did you look at the patches?
Most of them touch files like exec.c which is all over
the place.

> > > Also, this week I'm looking at the migration queue -- it's an
> > > unofficial split of maintenance duties between Juan and me while we're
> > > still trying to find out what works best.
> > > 
> > 
> > I don't know how am I supposed to know that.
> 
> Right now you don't need to -- I just pick patches up and pass them on
> to Juan, who does the pull req.
> 
> > Send patch to MAINTAINERS or something?
> 
> I'm going to add myself to migration maintainers, yes.  But that's
> secondary; the pull reqs still go via Juan.
> 
> 
> 		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Mon) 17 Nov 2014 [14:36:45], Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 05:50:34PM +0530, Amit Shah wrote:
> > On (Mon) 17 Nov 2014 [13:48:58], Michael S. Tsirkin wrote:
> > > On Mon, Nov 17, 2014 at 04:37:50PM +0530, Amit Shah wrote:
> > > > On (Mon) 17 Nov 2014 [12:52:59], Michael S. Tsirkin wrote:
> > > > > On Mon, Nov 17, 2014 at 04:08:58PM +0530, Amit Shah wrote:
> > > > > > On (Mon) 17 Nov 2014 [12:32:57], Michael S. Tsirkin wrote:
> > > > > > > On Mon, Nov 17, 2014 at 12:06:38PM +0530, Amit Shah wrote:
> > > > > > > > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > > > > > > > This patchset fixes CVE-2014-7840: invalid
> > > > > > > > > migration stream can cause arbitrary qemu memory
> > > > > > > > > overwrite.
> > > > > > > > > First patch includes the minimal fix for the issue.
> > > > > > > > > Follow-up patches on top add extra checking to reduce the
> > > > > > > > > chance this kind of bug recurs.
> > > > > > > > > 
> > > > > > > > > Note: these are already (tentatively-pending review)
> > > > > > > > > queued in my tree, so only review/ack
> > > > > > > > > is necessary.
> > > > > > > > 
> > > > > > > > Why not let this go in via the migration tree?
> > > > > > > 
> > > > > > > Well I Cc'd Juan and David, so if they had a problem with this, I expect
> > > > > > > they'd complain.  David acked so I assume it's ok.  Since I wasted time
> > > > > > > testing this and have it on my tree already, might as well just merge.
> > > > > > 
> > > > > > IMO asking as a courtesy would've been better than just stating it.
> > > > > 
> > > > > Right, thanks for reminding me.
> > > > > 
> > > > > BTW, there is actually a good reason to special-case it: it's a CVE fix,
> > > > > which I handle.  So they stay on my private queue and are passed
> > > > > to vendors so vendors can fix downstreams, until making fix public is
> > > > > cleared with all reporters and vendors.
> > > > > After reporting is cleared, I try to collect acks but don't normally route
> > > > > patches through separate queues - that would make it harder to
> > > > > track the status which we need for CVEs.
> > > > 
> > > > Patch is public, so all of this doesn't really matter.
> > > > 
> > > > But: involving maintainers in their areas, even if the patch is
> > > > embargoed, should be a pre-requisite.  I'm not sure if we're doing
> > > > that, but please do that so patches get a proper review from the
> > > > maintainers.
> > > 
> > > Involving more people means more back and forth with reporters which
> > > must approve any disclosure.  If the issue isn't clear, I do involve
> > > maintainers.  I send patches on list and try to merge them only after
> > > they get ack from relevant people. I'm sorry, but this is as far as I
> > > have the time to go.
> > 
> > The other aspect of the thing is sub-optimal, or patches with bugs,
> > get pushed in, because the maintainers didn't get involved.  
> 
> Patches don't get merged before they are on list for a while.
> I typically ping people if I don't get acks.

BTW I was talking about embargoed bugs / patches.  That's not relevant
for this discussion.  I'll create a new thread to discuss qemu's
security policy for embargoed bugs.


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> This patchset fixes CVE-2014-7840: invalid
> migration stream can cause arbitrary qemu memory
> overwrite.
> First patch includes the minimal fix for the issue.
> Follow-up patches on top add extra checking to reduce the
> chance this kind of bug recurs.
> 
> Note: these are already (tentatively-pending review)
> queued in my tree, so only review/ack
> is necessary.

Any more acks for this?  Dave?

Also, Michael agreed to just push patch 1 for now, and 2-4 can go in
later (2.3)?  Michael, is that fine with you?


		Amit

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Dr. David Alan Gilbert]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > This patchset fixes CVE-2014-7840: invalid
> > migration stream can cause arbitrary qemu memory
> > overwrite.
> > First patch includes the minimal fix for the issue.
> > Follow-up patches on top add extra checking to reduce the
> > chance this kind of bug recurs.
> > 
> > Note: these are already (tentatively-pending review)
> > queued in my tree, so only review/ack
> > is necessary.
> 
> Any more acks for this?  Dave?

Yep,
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
(for the set)

> Also, Michael agreed to just push patch 1 for now, and 2-4 can go in
> later (2.3)?  Michael, is that fine with you?
> 
> 
> 		Amit

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Michael S. Tsirkin]

On Tue, Nov 18, 2014 at 02:31:34PM +0530, Amit Shah wrote:
> On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > This patchset fixes CVE-2014-7840: invalid
> > migration stream can cause arbitrary qemu memory
> > overwrite.
> > First patch includes the minimal fix for the issue.
> > Follow-up patches on top add extra checking to reduce the
> > chance this kind of bug recurs.
> > 
> > Note: these are already (tentatively-pending review)
> > queued in my tree, so only review/ack
> > is necessary.
> 
> Any more acks for this?  Dave?
> 
> Also, Michael agreed to just push patch 1 for now, and 2-4 can go in
> later (2.3)?  Michael, is that fine with you?
> 
> 
> 		Amit

I'm fine with putting them off.
Someone wants to take them or should I?


-- 
MST

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Nov 18, 2014 at 02:31:34PM +0530, Amit Shah wrote:
> > On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> > > This patchset fixes CVE-2014-7840: invalid
> > > migration stream can cause arbitrary qemu memory
> > > overwrite.
> > > First patch includes the minimal fix for the issue.
> > > Follow-up patches on top add extra checking to reduce the
> > > chance this kind of bug recurs.
> > > 
> > > Note: these are already (tentatively-pending review)
> > > queued in my tree, so only review/ack
> > > is necessary.
> > 
> > Any more acks for this?  Dave?
> > 
> > Also, Michael agreed to just push patch 1 for now, and 2-4 can go in
> > later (2.3)?  Michael, is that fine with you?
> > 
> > 
> > 		Amit
> 
> I'm fine with putting them off.
> Someone wants to take them or should I?

exec.c stuff seems to be pretty much all over, so I think you're
as good as anyone.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Wed, Nov 12, 2014 at 11:44:35AM +0200, Michael S. Tsirkin wrote:
> This patchset fixes CVE-2014-7840: invalid
> migration stream can cause arbitrary qemu memory
> overwrite.
> First patch includes the minimal fix for the issue.
> Follow-up patches on top add extra checking to reduce the
> chance this kind of bug recurs.
> 
> Note: these are already (tentatively-pending review)
> queued in my tree, so only review/ack
> is necessary.
> 
> Michael S. Tsirkin (4):

Reviewed-by: Amos Kong <akong@redhat.com>

>   migration: fix parameter validation on ram load
>   exec: add wrapper for host pointer access
>   cpu: assert host pointer offset within block
>   cpu: verify that block->host is set
> 
>  include/exec/cpu-all.h |  7 +++++++
>  arch_init.c            |  5 +++--
>  exec.c                 | 10 +++++-----
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> -- 
> MST
> 

-- 
			Amos.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/4] migration: fix CVE-2014-7840

[Amit Shah]

On (Wed) 12 Nov 2014 [11:44:35], Michael S. Tsirkin wrote:
> This patchset fixes CVE-2014-7840: invalid
> migration stream can cause arbitrary qemu memory
> overwrite.
> First patch includes the minimal fix for the issue.
> Follow-up patches on top add extra checking to reduce the
> chance this kind of bug recurs.

Queued up patches 2-4 in my tree, thanks.

		Amit