[Qemu-devel] [PATCH] migration: don't segfault on invalid input

[Qemu-devel] [PATCH] migration: don't segfault on invalid input

[Michael S. Tsirkin]

host_from_stream_offset returns NULL on error,
return error instead of trying to use that address,
to avoid segfault on invalid stream.

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

diff --git a/arch_init.c b/arch_init.c
index e468c0c..bc7528d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -116,6 +116,8 @@ static int ram_save_block(QEMUFile *f)
 
     if (!block)
         block = QLIST_FIRST(&ram_list.blocks);
+    if (!last_block)
+	last_block = block;
 
     current_addr = block->offset + offset;
 
@@ -390,6 +392,9 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
                 host = qemu_get_ram_ptr(addr);
             else
                 host = host_from_stream_offset(f, addr, flags);
+            if (!host) {
+                return -EINVAL;
+            }
 
             ch = qemu_get_byte(f);
             memset(host, ch, TARGET_PAGE_SIZE);
-- 
1.7.3-rc1

[Qemu-devel] Re: [PATCH] migration: don't segfault on invalid input

[Alex Williamson]

On Sun, 2010-10-17 at 20:43 +0200, Michael S. Tsirkin wrote:
> host_from_stream_offset returns NULL on error,
> return error instead of trying to use that address,
> to avoid segfault on invalid stream.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch_init.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index e468c0c..bc7528d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -116,6 +116,8 @@ static int ram_save_block(QEMUFile *f)
>  
>      if (!block)
>          block = QLIST_FIRST(&ram_list.blocks);
> +    if (!last_block)
> +	last_block = block;
>  
>      current_addr = block->offset + offset;

NAK, last_block == block will cause us to set the continue flag on the
first block.  I assume you're trying to prevent the last_block->offset
segv in the while test, but that should never happen.  The only time
last_block is NULL is at the beginning of stage 1, where all pages are
dirty.  At that point we should always enter the if {} block at the
beginning of the do {} while, which breaks out rather than hitting the
segv.  We'll then set last_block for the next pass.

> @@ -390,6 +392,9 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>                  host = qemu_get_ram_ptr(addr);
>              else
>                  host = host_from_stream_offset(f, addr, flags);
> +            if (!host) {
> +                return -EINVAL;
> +            }
>  

This should also never happen since we've synchronized ramblocks at the
beginning of migration, but probably a good idea to return an error.
Thanks,

Alex

[Qemu-devel] Re: [PATCH] migration: don't segfault on invalid input

[Michael S. Tsirkin]

On Sun, Oct 17, 2010 at 03:01:45PM -0600, Alex Williamson wrote:
> On Sun, 2010-10-17 at 20:43 +0200, Michael S. Tsirkin wrote:
> > host_from_stream_offset returns NULL on error,
> > return error instead of trying to use that address,
> > to avoid segfault on invalid stream.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  arch_init.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index e468c0c..bc7528d 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -116,6 +116,8 @@ static int ram_save_block(QEMUFile *f)
> >  
> >      if (!block)
> >          block = QLIST_FIRST(&ram_list.blocks);
> > +    if (!last_block)
> > +	last_block = block;
> >  
> >      current_addr = block->offset + offset;
> 
> NAK, last_block == block will cause us to set the continue flag on the
> first block.  I assume you're trying to prevent the last_block->offset
> segv in the while test, but that should never happen.  The only time
> last_block is NULL is at the beginning of stage 1, where all pages are
> dirty.  At that point we should always enter the if {} block at the
> beginning of the do {} while, which breaks out rather than hitting the
> segv.  We'll then set last_block for the next pass.

Sorry, sent this chunk in error.

> > @@ -390,6 +392,9 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> >                  host = qemu_get_ram_ptr(addr);
> >              else
> >                  host = host_from_stream_offset(f, addr, flags);
> > +            if (!host) {
> > +                return -EINVAL;
> > +            }
> >  
> 
> This should also never happen since we've synchronized ramblocks at the
> beginning of migration, but probably a good idea to return an error.
> Thanks,
> 
> Alex