[Qemu-devel] [PATCH RFC v3 0/3] state loading security issues

[Qemu-devel] [PATCH RFC v3 0/3] 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.

Changes from v2:
address comments by dgilbert


Michael S. Tsirkin (3):
  vmstate: add VMS_MUST_EXIST
  vmstate: add VMSTATE_TEST
  hpet: fix buffer overrun on invalid state load

 include/migration/vmstate.h |  7 +++++++
 hw/timer/hpet.c             | 13 +++++++++++++
 vmstate.c                   | 10 ++++++++++
 3 files changed, 30 insertions(+)

-- 
MST

[Qemu-devel] [PATCH RFC v3 1/3] 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, 11 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index e7e1705..de970ab 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -100,6 +100,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 18b3732..236ce0b 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -103,6 +103,10 @@ 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++;
     }
@@ -146,6 +150,12 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                     field->info->put(f, addr, size);
                 }
             }
+        } else {
+            if (field->flags & VMS_MUST_EXIST) {
+                fprintf(stderr, "Output state validation failed: %s/%s\n",
+                        vmsd->name, field->name);
+                assert(!(field->flags & VMS_MUST_EXIST));
+            }
         }
         field++;
     }
-- 
MST

[Qemu-devel] [PATCH RFC v3 2/3] 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 de970ab..97629b7 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -204,6 +204,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_ARRAY | VMS_MUST_EXIST,                      \
+}
+
 #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
-- 
MST

[Qemu-devel] [PATCH RFC v3 3/3] 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>
---
 hw/timer/hpet.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1ee0640..7303ade 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -248,6 +248,18 @@ static int hpet_pre_load(void *opaque)
     return 0;
 }
 
+static 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 int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
@@ -318,6 +330,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] [PATCH RFC v3 1/3] 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, 11 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index e7e1705..de970ab 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -100,6 +100,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 18b3732..236ce0b 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -103,6 +103,10 @@ 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++;
>      }
> @@ -146,6 +150,12 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      field->info->put(f, addr, size);
>                  }
>              }
> +        } else {
> +            if (field->flags & VMS_MUST_EXIST) {
> +                fprintf(stderr, "Output state validation failed: %s/%s\n",
> +                        vmsd->name, field->name);
> +                assert(!(field->flags & VMS_MUST_EXIST));
> +            }
>          }
>          field++;
>      }
> -- 
> MST

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

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

Re: [Qemu-devel] [PATCH RFC v3 2/3] vmstate: add VMSTATE_TEST

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> Can validate state using VMS_NONE and VMS_MUST_EXIST

Old comment, VMS_NONE being dead.

> 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 de970ab..97629b7 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -204,6 +204,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_ARRAY | VMS_MUST_EXIST,                      \

Please comment this to say it's using the 0 sized array trick, (personally
I'd explicitly set .num = 0  as well).

Dave

> +}
> +
>  #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
>      .name       = (stringify(_field)),                               \
>      .version_id = (_version),                                        \
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC v3 3/3] hpet: fix buffer overrun on invalid state load

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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>

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


> ---
>  hw/timer/hpet.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1ee0640..7303ade 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -248,6 +248,18 @@ static int hpet_pre_load(void *opaque)
>      return 0;
>  }
>  
> +static 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 int hpet_post_load(void *opaque, int version_id)
>  {
>      HPETState *s = opaque;
> @@ -318,6 +330,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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK