[Qemu-devel] [RFC v2 0/5] state loading security issues

[Qemu-devel] [RFC v2 0/5] state loading security issues

[Michael S. Tsirkin]

In an attempt to provide a generic solution for this
set of issues, this adds a way to add validators
in the middle of the structure.

On failure, we assert on output (should never happen)
and fail migration on input.

The last patch in the series shows how the new
infrastructure is used.
I'll wait a bit for feedback, if there's none
I'll go ahead and use this to fix the state loading CVEs.

Michael S. Tsirkin (5):
  vmstate: reduce code duplication
  vmstate: add VMS_NONE
  vmstate: add VMS_MUST_EXIST
  vmstate: add VMSTATE_TEST
  hpet: fix buffer overrun on invalid state load

 include/migration/vmstate.h |   8 ++++
 hw/timer/hpet.c             |  17 +++++++
 vmstate.c                   | 107 +++++++++++++++++++++++++-------------------
 3 files changed, 87 insertions(+), 45 deletions(-)

-- 
MST

[Qemu-devel] [RFC v2 1/5] vmstate: reduce code duplication

[Michael S. Tsirkin]

move size offset and number of elements math out
to functions, to reduce code duplication.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 vmstate.c | 97 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/vmstate.c b/vmstate.c
index c61b0e5..18b3732 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -9,6 +9,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque);
 
+static int vmstate_n_elems(void *opaque, VMStateField *field)
+{
+    int n_elems = 1;
+
+    if (field->flags & VMS_ARRAY) {
+        n_elems = field->num;
+    } else if (field->flags & VMS_VARRAY_INT32) {
+        n_elems = *(int32_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT32) {
+        n_elems = *(uint32_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT16) {
+        n_elems = *(uint16_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT8) {
+        n_elems = *(uint8_t *)(opaque+field->num_offset);
+    }
+
+    return n_elems;
+}
+
+static int vmstate_size(void *opaque, VMStateField *field)
+{
+    int size = field->size;
+
+    if (field->flags & VMS_VBUFFER) {
+        size = *(int32_t *)(opaque+field->size_offset);
+        if (field->flags & VMS_MULTIPLY) {
+            size *= field->size;
+        }
+    }
+
+    return size;
+}
+
+static void *vmstate_base_addr(void *opaque, VMStateField *field)
+{
+    void *base_addr = opaque + field->offset;
+
+    if (field->flags & VMS_POINTER) {
+        base_addr = *(void **)base_addr + field->start;
+    }
+
+    return base_addr;
+}
+
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id)
 {
@@ -38,30 +82,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
              field->field_exists(opaque, version_id)) ||
             (!field->field_exists &&
              field->version_id <= version_id)) {
-            void *base_addr = opaque + field->offset;
-            int i, n_elems = 1;
-            int size = field->size;
-
-            if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
-                }
-            }
-            if (field->flags & VMS_ARRAY) {
-                n_elems = field->num;
-            } else if (field->flags & VMS_VARRAY_INT32) {
-                n_elems = *(int32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT32) {
-                n_elems = *(uint32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT16) {
-                n_elems = *(uint16_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT8) {
-                n_elems = *(uint8_t *)(opaque+field->num_offset);
-            }
-            if (field->flags & VMS_POINTER) {
-                base_addr = *(void **)base_addr + field->start;
-            }
+            void *base_addr = vmstate_base_addr(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field);
+            int size = vmstate_size(opaque, field);
+
             for (i = 0; i < n_elems; i++) {
                 void *addr = base_addr + size * i;
 
@@ -103,27 +127,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
     while (field->name) {
         if (!field->field_exists ||
             field->field_exists(opaque, vmsd->version_id)) {
-            void *base_addr = opaque + field->offset;
-            int i, n_elems = 1;
-            int size = field->size;
-
-            if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
-                }
-            }
-            if (field->flags & VMS_ARRAY) {
-                n_elems = field->num;
-            } else if (field->flags & VMS_VARRAY_INT32) {
-                n_elems = *(int32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT32) {
-                n_elems = *(uint32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT16) {
-                n_elems = *(uint16_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT8) {
-                n_elems = *(uint8_t *)(opaque+field->num_offset);
-            }
+            void *base_addr = vmstate_base_addr(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field);
+            int size = vmstate_size(opaque, field);
+
             if (field->flags & VMS_POINTER) {
                 base_addr = *(void **)base_addr + field->start;
             }
-- 
MST

Re: [Qemu-devel] [RFC v2 1/5] vmstate: reduce code duplication

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> move size offset and number of elements math out
> to functions, to reduce code duplication.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

If this was new code I would have rejected the use of signed 'int'
for something counting the number of elements and size; but this is just
a rearrange, so lets fight that another time.

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  vmstate.c | 97 ++++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 52 insertions(+), 45 deletions(-)
> 
> diff --git a/vmstate.c b/vmstate.c
> index c61b0e5..18b3732 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -9,6 +9,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque);
>  
> +static int vmstate_n_elems(void *opaque, VMStateField *field)
> +{
> +    int n_elems = 1;
> +
> +    if (field->flags & VMS_ARRAY) {
> +        n_elems = field->num;
> +    } else if (field->flags & VMS_VARRAY_INT32) {
> +        n_elems = *(int32_t *)(opaque+field->num_offset);
> +    } else if (field->flags & VMS_VARRAY_UINT32) {
> +        n_elems = *(uint32_t *)(opaque+field->num_offset);
> +    } else if (field->flags & VMS_VARRAY_UINT16) {
> +        n_elems = *(uint16_t *)(opaque+field->num_offset);
> +    } else if (field->flags & VMS_VARRAY_UINT8) {
> +        n_elems = *(uint8_t *)(opaque+field->num_offset);
> +    }
> +
> +    return n_elems;
> +}
> +
> +static int vmstate_size(void *opaque, VMStateField *field)
> +{
> +    int size = field->size;
> +
> +    if (field->flags & VMS_VBUFFER) {
> +        size = *(int32_t *)(opaque+field->size_offset);
> +        if (field->flags & VMS_MULTIPLY) {
> +            size *= field->size;
> +        }
> +    }
> +
> +    return size;
> +}
> +
> +static void *vmstate_base_addr(void *opaque, VMStateField *field)
> +{
> +    void *base_addr = opaque + field->offset;
> +
> +    if (field->flags & VMS_POINTER) {
> +        base_addr = *(void **)base_addr + field->start;
> +    }
> +
> +    return base_addr;
> +}
> +
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                         void *opaque, int version_id)
>  {
> @@ -38,30 +82,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>               field->field_exists(opaque, version_id)) ||
>              (!field->field_exists &&
>               field->version_id <= version_id)) {
> -            void *base_addr = opaque + field->offset;
> -            int i, n_elems = 1;
> -            int size = field->size;
> -
> -            if (field->flags & VMS_VBUFFER) {
> -                size = *(int32_t *)(opaque+field->size_offset);
> -                if (field->flags & VMS_MULTIPLY) {
> -                    size *= field->size;
> -                }
> -            }
> -            if (field->flags & VMS_ARRAY) {
> -                n_elems = field->num;
> -            } else if (field->flags & VMS_VARRAY_INT32) {
> -                n_elems = *(int32_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT32) {
> -                n_elems = *(uint32_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT16) {
> -                n_elems = *(uint16_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT8) {
> -                n_elems = *(uint8_t *)(opaque+field->num_offset);
> -            }
> -            if (field->flags & VMS_POINTER) {
> -                base_addr = *(void **)base_addr + field->start;
> -            }
> +            void *base_addr = vmstate_base_addr(opaque, field);
> +            int i, n_elems = vmstate_n_elems(opaque, field);
> +            int size = vmstate_size(opaque, field);
> +
>              for (i = 0; i < n_elems; i++) {
>                  void *addr = base_addr + size * i;
>  
> @@ -103,27 +127,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>      while (field->name) {
>          if (!field->field_exists ||
>              field->field_exists(opaque, vmsd->version_id)) {
> -            void *base_addr = opaque + field->offset;
> -            int i, n_elems = 1;
> -            int size = field->size;
> -
> -            if (field->flags & VMS_VBUFFER) {
> -                size = *(int32_t *)(opaque+field->size_offset);
> -                if (field->flags & VMS_MULTIPLY) {
> -                    size *= field->size;
> -                }
> -            }
> -            if (field->flags & VMS_ARRAY) {
> -                n_elems = field->num;
> -            } else if (field->flags & VMS_VARRAY_INT32) {
> -                n_elems = *(int32_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT32) {
> -                n_elems = *(uint32_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT16) {
> -                n_elems = *(uint16_t *)(opaque+field->num_offset);
> -            } else if (field->flags & VMS_VARRAY_UINT8) {
> -                n_elems = *(uint8_t *)(opaque+field->num_offset);
> -            }
> +            void *base_addr = vmstate_base_addr(opaque, field);
> +            int i, n_elems = vmstate_n_elems(opaque, field);
> +            int size = vmstate_size(opaque, field);
> +
>              if (field->flags & VMS_POINTER) {
>                  base_addr = *(void **)base_addr + field->start;
>              }
> -- 
> MST
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [RFC v2 2/5] vmstate: add VMS_NONE

[Michael S. Tsirkin]

The element with this flags value is skipped.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h | 1 +
 vmstate.c                   | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index e7e1705..3a1587e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -88,6 +88,7 @@ struct VMStateInfo {
 };
 
 enum VMStateFlags {
+    VMS_NONE             = 0x000,
     VMS_SINGLE           = 0x001,
     VMS_POINTER          = 0x002,
     VMS_ARRAY            = 0x004,
diff --git a/vmstate.c b/vmstate.c
index 18b3732..fe53735 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -13,7 +13,9 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
 {
     int n_elems = 1;
 
-    if (field->flags & VMS_ARRAY) {
+    if (!(field->flags & ~VMS_NONE)) {
+        n_elems = 0;
+    } else if (field->flags & VMS_ARRAY) {
         n_elems = field->num;
     } else if (field->flags & VMS_VARRAY_INT32) {
         n_elems = *(int32_t *)(opaque+field->num_offset);
-- 
MST

Re: [Qemu-devel] [RFC v2 2/5] vmstate: add VMS_NONE

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> The element with this flags value is skipped.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/migration/vmstate.h | 1 +
>  vmstate.c                   | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index e7e1705..3a1587e 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -88,6 +88,7 @@ struct VMStateInfo {
>  };
>  
>  enum VMStateFlags {
> +    VMS_NONE             = 0x000,
>      VMS_SINGLE           = 0x001,
>      VMS_POINTER          = 0x002,
>      VMS_ARRAY            = 0x004,

I think this would be simpler if you just gave it it's own
bit, the next patch already makes the mask more complicated.

However, do you need it?  Why not just declare a VMS_ARRAY with field->num=0 ?

Dave

> diff --git a/vmstate.c b/vmstate.c
> index 18b3732..fe53735 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -13,7 +13,9 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
>  {
>      int n_elems = 1;
>  
> -    if (field->flags & VMS_ARRAY) {
> +    if (!(field->flags & ~VMS_NONE)) {
> +        n_elems = 0;
> +    } else if (field->flags & VMS_ARRAY) {
>          n_elems = field->num;
>      } else if (field->flags & VMS_VARRAY_INT32) {
>          n_elems = *(int32_t *)(opaque+field->num_offset);
> -- 
> MST
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC v2 2/5] vmstate: add VMS_NONE

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 05:07:39PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > The element with this flags value is skipped.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/migration/vmstate.h | 1 +
> >  vmstate.c                   | 4 +++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index e7e1705..3a1587e 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -88,6 +88,7 @@ struct VMStateInfo {
> >  };
> >  
> >  enum VMStateFlags {
> > +    VMS_NONE             = 0x000,
> >      VMS_SINGLE           = 0x001,
> >      VMS_POINTER          = 0x002,
> >      VMS_ARRAY            = 0x004,
> 
> I think this would be simpler if you just gave it it's own
> bit, the next patch already makes the mask more complicated.
> 
> However, do you need it?  Why not just declare a VMS_ARRAY with field->num=0 ?
> 
> Dave

Good idea, I'll do that.

> > diff --git a/vmstate.c b/vmstate.c
> > index 18b3732..fe53735 100644
> > --- a/vmstate.c
> > +++ b/vmstate.c
> > @@ -13,7 +13,9 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
> >  {
> >      int n_elems = 1;
> >  
> > -    if (field->flags & VMS_ARRAY) {
> > +    if (!(field->flags & ~VMS_NONE)) {
> > +        n_elems = 0;
> > +    } else if (field->flags & VMS_ARRAY) {
> >          n_elems = field->num;
> >      } else if (field->flags & VMS_VARRAY_INT32) {
> >          n_elems = *(int32_t *)(opaque+field->num_offset);
> > -- 
> > MST
> > 
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

Can be used to verify a required field exists or validate
state in some other way.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h |  1 +
 vmstate.c                   | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 3a1587e..eb90cef 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -101,6 +101,7 @@ enum VMStateFlags {
     VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
     VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
     VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
+    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
 };
 
 typedef struct {
diff --git a/vmstate.c b/vmstate.c
index fe53735..4943b83 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -13,7 +13,7 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
 {
     int n_elems = 1;
 
-    if (!(field->flags & ~VMS_NONE)) {
+    if (!(field->flags & ~(VMS_NONE | VMS_MUST_EXIST))) {
         n_elems = 0;
     } else if (field->flags & VMS_ARRAY) {
         n_elems = field->num;
@@ -107,6 +107,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             }
         }
         field++;
+    } else if (field->flags & VMS_MUST_EXIST) {
+        fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
+        return -1;
     }
     ret = vmstate_subsection_load(f, vmsd, opaque);
     if (ret != 0) {
@@ -148,6 +151,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                     field->info->put(f, addr, size);
                 }
             }
+        } else {
+            if (field->flags & VMS_MUST_EXIST) {
+                fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
+                assert(!(field->flags & VMS_MUST_EXIST));
+            }
         }
         field++;
     }
-- 
MST

Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 04:38:01PM +0200, Michael S. Tsirkin wrote:
> Can be used to verify a required field exists or validate
> state in some other way.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sent a wrong patch. this is RFC in any case so not resending
everything, but this is needed on top both to build and make the intent
clear:


diff --git a/vmstate.c b/vmstate.c
index 4943b83..974c618 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -105,11 +105,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
             }
+        } else if (field->flags & VMS_MUST_EXIST) {
+            fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
+            return -1;
         }
         field++;
-    } else if (field->flags & VMS_MUST_EXIST) {
-        fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
-        return -1;
     }
     ret = vmstate_subsection_load(f, vmsd, opaque);
     if (ret != 0) {

Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> Can be used to verify a required field exists or validate
> state in some other way.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/migration/vmstate.h |  1 +
>  vmstate.c                   | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 3a1587e..eb90cef 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -101,6 +101,7 @@ enum VMStateFlags {
>      VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> +    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
>  };
>  
>  typedef struct {
> diff --git a/vmstate.c b/vmstate.c
> index fe53735..4943b83 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -13,7 +13,7 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
>  {
>      int n_elems = 1;
>  
> -    if (!(field->flags & ~VMS_NONE)) {
> +    if (!(field->flags & ~(VMS_NONE | VMS_MUST_EXIST))) {
>          n_elems = 0;
>      } else if (field->flags & VMS_ARRAY) {
>          n_elems = field->num;
> @@ -107,6 +107,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>              }
>          }
>          field++;
> +    } else if (field->flags & VMS_MUST_EXIST) {
> +        fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
> +        return -1;

I think your intent here is just to misuse the field_exist function pointer 
as a call for a different reason as a hook for a validator; is it really worth
misusing it like that or is something more explicit worth it?
Perhaps something passed an Error** so it could pass back what was wrong?

>      }
>      ret = vmstate_subsection_load(f, vmsd, opaque);
>      if (ret != 0) {
> @@ -148,6 +151,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      field->info->put(f, addr, size);
>                  }
>              }
> +        } else {
> +            if (field->flags & VMS_MUST_EXIST) {
> +                fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
> +                assert(!(field->flags & VMS_MUST_EXIST));

Wrong message for the save side.

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

Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > Can be used to verify a required field exists or validate
> > state in some other way.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/migration/vmstate.h |  1 +
> >  vmstate.c                   | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 3a1587e..eb90cef 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -101,6 +101,7 @@ enum VMStateFlags {
> >      VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
> >      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
> >      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> > +    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
> >  };
> >  
> >  typedef struct {
> > diff --git a/vmstate.c b/vmstate.c
> > index fe53735..4943b83 100644
> > --- a/vmstate.c
> > +++ b/vmstate.c
> > @@ -13,7 +13,7 @@ static int vmstate_n_elems(void *opaque, VMStateField *field)
> >  {
> >      int n_elems = 1;
> >  
> > -    if (!(field->flags & ~VMS_NONE)) {
> > +    if (!(field->flags & ~(VMS_NONE | VMS_MUST_EXIST))) {
> >          n_elems = 0;
> >      } else if (field->flags & VMS_ARRAY) {
> >          n_elems = field->num;
> > @@ -107,6 +107,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >              }
> >          }
> >          field++;
> > +    } else if (field->flags & VMS_MUST_EXIST) {
> > +        fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
> > +        return -1;
> 
> I think your intent here is just to misuse the field_exist function pointer 
> as a call for a different reason as a hook for a validator; is it really worth
> misusing it like that or is something more explicit worth it?
> Perhaps something passed an Error** so it could pass back what was wrong?


Well adding a required field seems valuable by itself, does it not?

And there's no way to pass in Error** since none of the callers
has Error**: all of migration still uses stderr to pass
errors.

So we could add an API but it doesn't seem too valuable.

Since all callers will use this through a wrapper like VMSTATE_TEST,
it will be easy to change our mind later.


> >      }
> >      ret = vmstate_subsection_load(f, vmsd, opaque);
> >      if (ret != 0) {
> > @@ -148,6 +151,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >                      field->info->put(f, addr, size);
> >                  }
> >              }
> > +        } else {
> > +            if (field->flags & VMS_MUST_EXIST) {
> > +                fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, field->name);
> > +                assert(!(field->flags & VMS_MUST_EXIST));
> 
> Wrong message for the save side.


Thanks!

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

Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:

<snip>

> > I think your intent here is just to misuse the field_exist function pointer 
> > as a call for a different reason as a hook for a validator; is it really worth
> > misusing it like that or is something more explicit worth it?
> > Perhaps something passed an Error** so it could pass back what was wrong?
> 
> 
> Well adding a required field seems valuable by itself, does it not?

Maybe; however most fields are always-present, unless they have a test
function or minimum version, so it's a little weird to add a 'required'
when that's the default.

> And there's no way to pass in Error** since none of the callers
> has Error**: all of migration still uses stderr to pass
> errors.
> 
> So we could add an API but it doesn't seem too valuable.
> 
> Since all callers will use this through a wrapper like VMSTATE_TEST,
> it will be easy to change our mind later.

Yep, that's fine - was just an idea.

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

Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

On Tue, Mar 25, 2014 at 09:23:13AM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:
> 
> <snip>
> 
> > > I think your intent here is just to misuse the field_exist function pointer 
> > > as a call for a different reason as a hook for a validator; is it really worth
> > > misusing it like that or is something more explicit worth it?
> > > Perhaps something passed an Error** so it could pass back what was wrong?
> > 
> > 
> > Well adding a required field seems valuable by itself, does it not?
> 
> Maybe; however most fields are always-present, unless they have a test
> function or minimum version, so it's a little weird to add a 'required'
> when that's the default.

Right - here we say "there is a test function but it must return true".

I considered adding a separate callback but it worried me
that it's not clear how would it interact with the exist
flag or the version flag. Ideas?

> > And there's no way to pass in Error** since none of the callers
> > has Error**: all of migration still uses stderr to pass
> > errors.
> > 
> > So we could add an API but it doesn't seem too valuable.
> > 
> > Since all callers will use this through a wrapper like VMSTATE_TEST,
> > it will be easy to change our mind later.
> 
> Yep, that's fine - was just an idea.
> 
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [RFC v2 4/5] vmstate: add VMSTATE_TEST

[Michael S. Tsirkin]

Can validate state using VMS_NONE and VMS_MUST_EXIST

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index eb90cef..e220f09 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -205,6 +205,12 @@ extern const VMStateInfo vmstate_info_bitmap;
     .offset       = vmstate_offset_value(_state, _field, _type),     \
 }
 
+#define VMSTATE_TEST(_name, _test) { \
+    .name         = (_name),                                         \
+    .field_exists = (_test),                                         \
+    .flags        = VMS_NONE | VMS_MUST_EXIST,                       \
+}
+
 #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
-- 
MST

[Qemu-devel] [RFC v2 5/5] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

CVE-2013-4527 hw/timer/hpet.c buffer overrun

hpet is a VARRAY with a uint8 size but static array of 32

To fix, make sure num_timers is valid using VMSTATE_TEST hook.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/timer/hpet.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1ee0640..67671bc 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -248,6 +248,22 @@ static int hpet_pre_load(void *opaque)
     return 0;
 }
 
+bool hpet_validate_num_timers(void *opaque, int version_id)
+{
+    HPETState *s = opaque;
+
+    if (s->num_timers < HPET_MIN_TIMERS) {
+        return false;
+    } else if (s->num_timers > HPET_MAX_TIMERS) {
+        return false;
+    }
+    return true;
+}
+
+static bool hpet_fix_num_timers(HPETState *s)
+{
+}
+
 static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
@@ -318,6 +334,7 @@ static const VMStateDescription vmstate_hpet = {
         VMSTATE_UINT64(isr, HPETState),
         VMSTATE_UINT64(hpet_counter, HPETState),
         VMSTATE_UINT8_V(num_timers, HPETState, 2),
+        VMSTATE_TEST("num_timers in range", hpet_validate_num_timers);
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
                                     vmstate_hpet_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
-- 
MST

Re: [Qemu-devel] [RFC v2 0/5] state loading security issues

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 04:37:43PM +0200, Michael S. Tsirkin wrote:
> In an attempt to provide a generic solution for this
> set of issues, this adds a way to add validators
> in the middle of the structure.
> 
> On failure, we assert on output (should never happen)
> and fail migration on input.
> 
> The last patch in the series shows how the new
> infrastructure is used.
> I'll wait a bit for feedback, if there's none
> I'll go ahead and use this to fix the state loading CVEs.

Forgot to commit some fixes so this doesn't
really work - but this is hopefully enough for people to
get the general idea and comment before I build more
code on top of this.

Please consider this pseudo-code :)

> Michael S. Tsirkin (5):
>   vmstate: reduce code duplication
>   vmstate: add VMS_NONE
>   vmstate: add VMS_MUST_EXIST
>   vmstate: add VMSTATE_TEST
>   hpet: fix buffer overrun on invalid state load
> 
>  include/migration/vmstate.h |   8 ++++
>  hw/timer/hpet.c             |  17 +++++++
>  vmstate.c                   | 107 +++++++++++++++++++++++++-------------------
>  3 files changed, 87 insertions(+), 45 deletions(-)
> 
> -- 
> MST
>