* [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
@ 2014-03-31 14:15 ` Michael S. Tsirkin
2014-03-31 15:01 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 02/30] vmstate: add VMS_MUST_EXIST Michael S. Tsirkin
` (28 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
dgilbert, =?UTF-8?q?Andreas=20F=C3=A4rber?=
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 b689f2f..dc99e1a 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -10,6 +10,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)
{
@@ -36,30 +80,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;
@@ -102,27 +126,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
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication
2014-03-31 14:15 ` [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication Michael S. Tsirkin
@ 2014-03-31 15:01 ` Dr. David Alan Gilbert
2014-03-31 15:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-03-31 15:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Juan Quintela, qemu-devel, mdroth, qemu-stable,
Alexey Kardashevskiy, =?UTF-8?q?Andreas=20F=C3=A4rber?=,
dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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 b689f2f..dc99e1a 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -10,6 +10,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)
> {
> @@ -36,30 +80,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;
>
> @@ -102,27 +126,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;
> }
Hmm, shouldn't those last 3 lines be deleted as well - the logic is
now in vmstate_base_addr?
Dave
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication
2014-03-31 15:01 ` Dr. David Alan Gilbert
@ 2014-03-31 15:27 ` Michael S. Tsirkin
0 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 15:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: mdroth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
qemu-devel, =?UTF-8?q?Andreas=20F=C3=A4rber?=
On Mon, Mar 31, 2014 at 04:01:34PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > 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 b689f2f..dc99e1a 100644
> > --- a/vmstate.c
> > +++ b/vmstate.c
> > @@ -10,6 +10,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)
> > {
> > @@ -36,30 +80,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;
> >
> > @@ -102,27 +126,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;
> > }
>
> Hmm, shouldn't those last 3 lines be deleted as well - the logic is
> now in vmstate_base_addr?
>
>
> Dave
So it should, thanks.
> > --
> > MST
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 02/30] vmstate: add VMS_MUST_EXIST
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
2014-03-31 14:15 ` [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE Michael S. Tsirkin
` (27 subsequent siblings)
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Fam Zheng, mdroth, Juan Quintela, Igor Mitsyanko,
qemu-stable, dgilbert, Orit Wasserman,
=?UTF-8?q?Andreas=20F=C3=A4rber?=
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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@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 dc99e1a..02d5d94 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -102,6 +102,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++;
}
@@ -145,6 +149,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
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
2014-03-31 14:15 ` [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication Michael S. Tsirkin
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 02/30] vmstate: add VMS_MUST_EXIST Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-04-01 10:39 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load Michael S. Tsirkin
` (26 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Fam Zheng, mdroth, Juan Quintela, Igor Mitsyanko,
qemu-stable, dgilbert
Validate state using VMS_ARRAY with num = 0 and VMS_MUST_EXIST
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/migration/vmstate.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index de970ab..5b71370 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -204,6 +204,14 @@ extern const VMStateInfo vmstate_info_bitmap;
.offset = vmstate_offset_value(_state, _field, _type), \
}
+/* Validate state using a boolean predicate. */
+#define VMSTATE_VALIDATE(_name, _test) { \
+ .name = (_name), \
+ .field_exists = (_test), \
+ .flags = VMS_ARRAY | VMS_MUST_EXIST, \
+ .num = 0, /* 0 elements: no data, only run _test */ \
+}
+
#define VMSTATE_POINTER(_field, _state, _version, _info, _type) { \
.name = (stringify(_field)), \
.version_id = (_version), \
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE Michael S. Tsirkin
@ 2014-04-01 10:39 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 10:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Fam Zheng, mdroth, Juan Quintela, Igor Mitsyanko,
qemu-stable, qemu-devel
* Michael S. Tsirkin (mst@redhat.com) wrote:
> Validate state using VMS_ARRAY with num = 0 and VMS_MUST_EXIST
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> include/migration/vmstate.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index de970ab..5b71370 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -204,6 +204,14 @@ extern const VMStateInfo vmstate_info_bitmap;
> .offset = vmstate_offset_value(_state, _field, _type), \
> }
>
> +/* Validate state using a boolean predicate. */
> +#define VMSTATE_VALIDATE(_name, _test) { \
> + .name = (_name), \
> + .field_exists = (_test), \
> + .flags = VMS_ARRAY | VMS_MUST_EXIST, \
> + .num = 0, /* 0 elements: no data, only run _test */ \
> +}
> +
> #define VMSTATE_POINTER(_field, _state, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (2 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 17:21 ` Laszlo Ersek
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load Michael S. Tsirkin
` (25 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Anthony Liguori, Laszlo Ersek,
KONRAD Frederic
CVE-2013-4148 QEMU 1.0 integer conversion in
virtio_net_load()@hw/net/virtio-net.c
Deals with loading a corrupted savevm image.
> n->mac_table.in_use = qemu_get_be32(f);
in_use is int so it can get negative when assigned 32bit unsigned value.
> /* MAC_TABLE_ENTRIES may be different from the saved image */
> if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
passing this check ^^^
> qemu_get_buffer(f, n->mac_table.macs,
> n->mac_table.in_use * ETH_ALEN);
with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
positive and bigger than mac_table.macs. For example 0x81000000
satisfies this condition when ETH_ALEN is 6.
A similar problem exists with is_multi.
Fix both by making the value unsigned.
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Edit: "for consistency, change first_multi as well".
Note: all call sites were audited to confirm that
making them unsigned didn't cause any issues:
it turns out we actually never do math on them,
so it's easy to validate because both values are
always <= MAC_TABLE_ENTRIES.
---
include/hw/virtio/virtio-net.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index df60f16..4b32440 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -176,8 +176,8 @@ typedef struct VirtIONet {
uint8_t nobcast;
uint8_t vhost_started;
struct {
- int in_use;
- int first_multi;
+ uint32_t in_use;
+ uint32_t first_multi;
uint8_t multi_overflow;
uint8_t uni_overflow;
uint8_t *macs;
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load Michael S. Tsirkin
@ 2014-03-31 17:21 ` Laszlo Ersek
2014-03-31 19:34 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Laszlo Ersek @ 2014-03-31 17:21 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Anthony Liguori, KONRAD Frederic, mdroth, dgilbert, qemu-stable
On 03/31/14 16:16, Michael S. Tsirkin wrote:
> CVE-2013-4148 QEMU 1.0 integer conversion in
> virtio_net_load()@hw/net/virtio-net.c
>
> Deals with loading a corrupted savevm image.
>
>> n->mac_table.in_use = qemu_get_be32(f);
>
> in_use is int so it can get negative when assigned 32bit unsigned value.
>
>> /* MAC_TABLE_ENTRIES may be different from the saved image */
>> if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
>
> passing this check ^^^
>
>> qemu_get_buffer(f, n->mac_table.macs,
>> n->mac_table.in_use * ETH_ALEN);
>
> with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
> positive and bigger than mac_table.macs. For example 0x81000000
> satisfies this condition when ETH_ALEN is 6.
>
> A similar problem exists with is_multi.
("first_multi")
>
> Fix both by making the value unsigned.
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Edit: "for consistency, change first_multi as well".
>
> Note: all call sites were audited to confirm that
> making them unsigned didn't cause any issues:
> it turns out we actually never do math on them,
> so it's easy to validate because both values are
> always <= MAC_TABLE_ENTRIES.
> ---
> include/hw/virtio/virtio-net.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index df60f16..4b32440 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -176,8 +176,8 @@ typedef struct VirtIONet {
> uint8_t nobcast;
> uint8_t vhost_started;
> struct {
> - int in_use;
> - int first_multi;
> + uint32_t in_use;
> + uint32_t first_multi;
> uint8_t multi_overflow;
> uint8_t uni_overflow;
> uint8_t *macs;
>
I ran
git grep -EHn '\<(in_use|first_multi)\>'
Many hits, hard to audit (esp. because I'm unfamiliar with the code).
Several loops with signed int loop variables. I checked cursorily.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load
2014-03-31 17:21 ` Laszlo Ersek
@ 2014-03-31 19:34 ` Michael S. Tsirkin
0 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 19:34 UTC (permalink / raw)
To: Laszlo Ersek
Cc: mdroth, qemu-stable, qemu-devel, Anthony Liguori, dgilbert,
KONRAD Frederic
On Mon, Mar 31, 2014 at 07:21:30PM +0200, Laszlo Ersek wrote:
> On 03/31/14 16:16, Michael S. Tsirkin wrote:
> > CVE-2013-4148 QEMU 1.0 integer conversion in
> > virtio_net_load()@hw/net/virtio-net.c
> >
> > Deals with loading a corrupted savevm image.
> >
> >> n->mac_table.in_use = qemu_get_be32(f);
> >
> > in_use is int so it can get negative when assigned 32bit unsigned value.
> >
> >> /* MAC_TABLE_ENTRIES may be different from the saved image */
> >> if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
> >
> > passing this check ^^^
> >
> >> qemu_get_buffer(f, n->mac_table.macs,
> >> n->mac_table.in_use * ETH_ALEN);
> >
> > with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
> > positive and bigger than mac_table.macs. For example 0x81000000
> > satisfies this condition when ETH_ALEN is 6.
> >
> > A similar problem exists with is_multi.
>
> ("first_multi")
Actually it doesn't even exist: Peter commented on this in v1
and I forgot to fix up the commit log :(
I really wanted to fix the commit log to say
"make first_multi unsigned for consistency" but
forgot.
> >
> > Fix both by making the value unsigned.
> >
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Edit: "for consistency, change first_multi as well".
> >
> > Note: all call sites were audited to confirm that
> > making them unsigned didn't cause any issues:
> > it turns out we actually never do math on them,
> > so it's easy to validate because both values are
> > always <= MAC_TABLE_ENTRIES.
> > ---
> > include/hw/virtio/virtio-net.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index df60f16..4b32440 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -176,8 +176,8 @@ typedef struct VirtIONet {
> > uint8_t nobcast;
> > uint8_t vhost_started;
> > struct {
> > - int in_use;
> > - int first_multi;
> > + uint32_t in_use;
> > + uint32_t first_multi;
> > uint8_t multi_overflow;
> > uint8_t uni_overflow;
> > uint8_t *macs;
> >
>
> I ran
>
> git grep -EHn '\<(in_use|first_multi)\>'
>
> Many hits, hard to audit (esp. because I'm unfamiliar with the code).
> Several loops with signed int loop variables. I checked cursorily.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (3 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-04-01 8:45 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 06/30] virtio-net: out-of-bounds buffer write on invalid state load Michael S. Tsirkin
` (24 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, dgilbert, Anthony Liguori, mdroth
CVE-2013-4149 QEMU 1.3.0 out-of-bounds buffer write in
virtio_net_load()@hw/net/virtio-net.c
> } else if (n->mac_table.in_use) {
> uint8_t *buf = g_malloc0(n->mac_table.in_use);
We are allocating buffer of size n->mac_table.in_use
> qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
and read to the n->mac_table.in_use size buffer n->mac_table.in_use *
ETH_ALEN bytes, corrupting memory.
If adversary controls state then memory written there is controlled
by adversary.
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/virtio-net.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 439477b..8d037b1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1363,9 +1363,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
qemu_get_buffer(f, n->mac_table.macs,
n->mac_table.in_use * ETH_ALEN);
} else if (n->mac_table.in_use) {
- uint8_t *buf = g_malloc0(n->mac_table.in_use);
- qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
- g_free(buf);
+ int i;
+
+ /* Overflow detected - can happen if source has a larger MAC table.
+ * We simply set overflow flag so there's no need to maintain the
+ * table of addresses, discard them all.
+ */
+ for (i = 0; i < n->mac_table.in_use * ETH_ALEN; ++i) {
+ qemu_get_byte(f);
+ }
n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
n->mac_table.in_use = 0;
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load Michael S. Tsirkin
@ 2014-04-01 8:45 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 8:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-stable, qemu-devel, Anthony Liguori, mdroth
* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4149 QEMU 1.3.0 out-of-bounds buffer write in
> virtio_net_load()@hw/net/virtio-net.c
>
> > } else if (n->mac_table.in_use) {
> > uint8_t *buf = g_malloc0(n->mac_table.in_use);
>
> We are allocating buffer of size n->mac_table.in_use
>
> > qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
>
> and read to the n->mac_table.in_use size buffer n->mac_table.in_use *
> ETH_ALEN bytes, corrupting memory.
>
> If adversary controls state then memory written there is controlled
> by adversary.
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/virtio-net.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 439477b..8d037b1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1363,9 +1363,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> qemu_get_buffer(f, n->mac_table.macs,
> n->mac_table.in_use * ETH_ALEN);
> } else if (n->mac_table.in_use) {
You can lose that 'else if' test;
if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
qemu_get_buffer(f, n->mac_table.macs,
n->mac_table.in_use * ETH_ALEN);
} else if (n->mac_table.in_use) {
to get to the else in_use > MAC_TABLE_ENTRIES.
Dave
> - uint8_t *buf = g_malloc0(n->mac_table.in_use);
> - qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN);
> - g_free(buf);
> + int i;
> +
> + /* Overflow detected - can happen if source has a larger MAC table.
> + * We simply set overflow flag so there's no need to maintain the
> + * table of addresses, discard them all.
> + */
> + for (i = 0; i < n->mac_table.in_use * ETH_ALEN; ++i) {
> + qemu_get_byte(f);
> + }
> n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
> n->mac_table.in_use = 0;
> }
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 06/30] virtio-net: out-of-bounds buffer write on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (4 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 07/30] virtio: " Michael S. Tsirkin
` (23 subsequent siblings)
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, qemu-stable, dgilbert, Anthony Liguori, mdroth
CVE-2013-4150 QEMU 1.5.0 out-of-bounds buffer write in
virtio_net_load()@hw/net/virtio-net.c
This code is in hw/net/virtio-net.c:
if (n->max_queues > 1) {
if (n->max_queues != qemu_get_be16(f)) {
error_report("virtio-net: different max_queues ");
return -1;
}
n->curr_queues = qemu_get_be16(f);
for (i = 1; i < n->curr_queues; i++) {
n->vqs[i].tx_waiting = qemu_get_be32(f);
}
}
Number of vqs is max_queues, so if we get invalid input here,
for example if max_queues = 2, curr_queues = 3, we get
write beyond end of the buffer, with data that comes from
wire.
This might be used to corrupt qemu memory in hard to predict ways.
Since we have lots of function pointers around, RCE might be possible.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/net/virtio-net.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8d037b1..c811fbd 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1413,6 +1413,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
}
n->curr_queues = qemu_get_be16(f);
+ if (n->curr_queues > n->max_queues) {
+ error_report("virtio-net: curr_queues %x > max_queues %x",
+ n->curr_queues, n->max_queues);
+ return -1;
+ }
for (i = 1; i < n->curr_queues; i++) {
n->vqs[i].tx_waiting = qemu_get_be32(f);
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH v4 07/30] virtio: out-of-bounds buffer write on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (5 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 06/30] virtio-net: out-of-bounds buffer write on invalid state load Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun " Michael S. Tsirkin
` (22 subsequent siblings)
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, dgilbert, Anthony Liguori, mdroth
CVE-2013-4151 QEMU 1.0 out-of-bounds buffer write in
virtio_load@hw/virtio/virtio.c
So we have this code since way back when:
num = qemu_get_be32(f);
for (i = 0; i < num; i++) {
vdev->vq[i].vring.num = qemu_get_be32(f);
array of vqs has size VIRTIO_PCI_QUEUE_MAX, so
on invalid input this will write beyond end of buffer.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/virtio/virtio.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..9008430 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -891,7 +891,8 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val)
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
- int num, i, ret;
+ int i, ret;
+ uint32_t num;
uint32_t features;
uint32_t supported_features;
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
@@ -919,6 +920,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
num = qemu_get_be32(f);
+ if (num > VIRTIO_PCI_QUEUE_MAX) {
+ error_report("Invalid number of PCI queues: 0x%x", num);
+ return -1;
+ }
+
for (i = 0; i < num; i++) {
vdev->vq[i].vring.num = qemu_get_be32(f);
if (k->has_variable_vring_alignment) {
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (6 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 07/30] virtio: " Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 15:31 ` Peter Maydell
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 09/30] hpet: " Michael S. Tsirkin
` (21 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, mdroth, qemu-stable, dgilbert, Anthony Liguori,
Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4526
Within hw/ide/ahci.c, VARRAY refers to ports which is also loaded. So
we use the old version of ports to read the array but then allow any
value for ports. This can cause the code to overflow.
There's no reason to migrate ports - it never changes.
So just make sure it matches.
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/ide/ahci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bfe633f..457a7a1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1293,7 +1293,7 @@ const VMStateDescription vmstate_ahci = {
VMSTATE_UINT32(control_regs.impl, AHCIState),
VMSTATE_UINT32(control_regs.version, AHCIState),
VMSTATE_UINT32(idp_index, AHCIState),
- VMSTATE_INT32(ports, AHCIState),
+ VMSTATE_INT32_EQUAL(ports, AHCIState),
VMSTATE_END_OF_LIST()
},
};
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun on invalid state load
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun " Michael S. Tsirkin
@ 2014-03-31 15:31 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Michael Roth, qemu-stable, QEMU Developers,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Dave Gilbert
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4526
>
> Within hw/ide/ahci.c, VARRAY refers to ports which is also loaded. So
> we use the old version of ports to read the array but then allow any
> value for ports. This can cause the code to overflow.
>
> There's no reason to migrate ports - it never changes.
> So just make sure it matches.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 09/30] hpet: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (7 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun " Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns " Michael S. Tsirkin
` (20 subsequent siblings)
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Ping Fan, mdroth, Markus Armbruster, qemu-stable, dgilbert,
Anthony Liguori, Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=
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_VALID 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 e15d6bc..2792f89 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -239,6 +239,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;
@@ -307,6 +319,7 @@ static const VMStateDescription vmstate_hpet = {
VMSTATE_UINT64(isr, HPETState),
VMSTATE_UINT64(hpet_counter, HPETState),
VMSTATE_UINT8_V(num_timers, HPETState, 2),
+ VMSTATE_VALIDATE("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
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (8 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 09/30] hpet: " Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-04-01 10:56 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun " Michael S. Tsirkin
` (19 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, dgilbert, Anthony Liguori, mdroth
4) CVE-2013-4529
hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is
too large
There are two issues in this file:
1. log_max from remote can be larger than on local
then buffer will overrun with data coming from state file.
2. log_num can be larger then we get data corruption
again with an overflow but not adversary controlled.
Fix both issues.
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/pci/pcie_aer.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 991502e..535be2c 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -795,6 +795,13 @@ static const VMStateDescription vmstate_pcie_aer_err = {
}
};
+static bool pcie_aer_state_log_num_valid(void *opaque, int version_id)
+{
+ PCIEAERLog *s = opaque;
+
+ return s->log_num <= s->log_max;
+}
+
const VMStateDescription vmstate_pcie_aer_log = {
.name = "PCIE_AER_ERROR_LOG",
.version_id = 1,
@@ -802,7 +809,8 @@ const VMStateDescription vmstate_pcie_aer_log = {
.minimum_version_id_old = 1,
.fields = (VMStateField[]) {
VMSTATE_UINT16(log_num, PCIEAERLog),
- VMSTATE_UINT16(log_max, PCIEAERLog),
+ VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
+ VMSTATE_VALIDATE("log_num <= log_max", pcie_aer_state_log_num_valid),
VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
vmstate_pcie_aer_err, PCIEAERErr),
VMSTATE_END_OF_LIST()
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns " Michael S. Tsirkin
@ 2014-04-01 10:56 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 10:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: mdroth, qemu-devel, Anthony Liguori, qemu-stable
* Michael S. Tsirkin (mst@redhat.com) wrote:
> 4) CVE-2013-4529
> hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is
> too large
>
> There are two issues in this file:
> 1. log_max from remote can be larger than on local
> then buffer will overrun with data coming from state file.
> 2. log_num can be larger then we get data corruption
> again with an overflow but not adversary controlled.
>
> Fix both issues.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/pci/pcie_aer.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 991502e..535be2c 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -795,6 +795,13 @@ static const VMStateDescription vmstate_pcie_aer_err = {
> }
> };
>
> +static bool pcie_aer_state_log_num_valid(void *opaque, int version_id)
> +{
> + PCIEAERLog *s = opaque;
> +
> + return s->log_num <= s->log_max;
> +}
> +
> const VMStateDescription vmstate_pcie_aer_log = {
> .name = "PCIE_AER_ERROR_LOG",
> .version_id = 1,
> @@ -802,7 +809,8 @@ const VMStateDescription vmstate_pcie_aer_log = {
> .minimum_version_id_old = 1,
> .fields = (VMStateField[]) {
> VMSTATE_UINT16(log_num, PCIEAERLog),
> - VMSTATE_UINT16(log_max, PCIEAERLog),
> + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
> + VMSTATE_VALIDATE("log_num <= log_max", pcie_aer_state_log_num_valid),
> VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
> vmstate_pcie_aer_err, PCIEAERErr),
> VMSTATE_END_OF_LIST()
> --
> MST
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (9 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns " Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 15:04 ` Peter Maydell
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c Michael S. Tsirkin
` (18 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
CVE-2013-4530
pl022.c did not bounds check tx_fifo_head and
rx_fifo_head after loading them from file and
before they are used to dereference array.
Reported-by: Michael S. Tsirkin <mst@redhat.com
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/ssi/pl022.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
index fd479ef..49b3f61 100644
--- a/hw/ssi/pl022.c
+++ b/hw/ssi/pl022.c
@@ -240,11 +240,23 @@ static const MemoryRegionOps pl022_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
+static int pl022_post_load(void *opaque, int version_id)
+{
+ PL022State *s = opaque;
+
+ if (s->tx_fifo_head > ARRAY_SIZE(s->tx_fifo) ||
+ s->rx_fifo_head > ARRAY_SIZE(s->rx_fifo)) {
+ return -1;
+ }
+ return 0;
+}
+
static const VMStateDescription vmstate_pl022 = {
.name = "pl022_ssp",
.version_id = 1,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
+ .post_load = pl022_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT32(cr0, PL022State),
VMSTATE_UINT32(cr1, PL022State),
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun on invalid state load
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun " Michael S. Tsirkin
@ 2014-03-31 15:04 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4530
>
> pl022.c did not bounds check tx_fifo_head and
> rx_fifo_head after loading them from file and
> before they are used to dereference array.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/ssi/pl022.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
> index fd479ef..49b3f61 100644
> --- a/hw/ssi/pl022.c
> +++ b/hw/ssi/pl022.c
> @@ -240,11 +240,23 @@ static const MemoryRegionOps pl022_ops = {
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> +static int pl022_post_load(void *opaque, int version_id)
> +{
> + PL022State *s = opaque;
> +
> + if (s->tx_fifo_head > ARRAY_SIZE(s->tx_fifo) ||
> + s->rx_fifo_head > ARRAY_SIZE(s->rx_fifo)) {
> + return -1;
Shouldn't these be '>=' checks? Also if the
incoming values are negative then we'll go
wrong in the other direction.
Given the way we do calculations involving *_fifo_len
as well, it might be best to also sanitize those,
though I think it's not possible currently to provoke
a buffer overrun with them. (NB that the _fifo_len
fields can validly be equal to the ARRAY_SIZE, unlike
the _fifo_head fields.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (10 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun " Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 15:40 ` Peter Maydell
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration Michael S. Tsirkin
` (17 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
dgilbert, Anthony Liguori, =?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4531
cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
cpreg_vmstate_array_len will cause a buffer overflow.
VMSTATE_INT32_LE was supposed to protect against this
but doesn't because it doesn't validate that input is
non-negative.
Fix this macro to valide the value appropriately.
The only other user of VMSTATE_INT32_LE doesn't
ever use negative numbers so it doesn't care.
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
vmstate.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/vmstate.c b/vmstate.c
index 02d5d94..e1e9cae 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -340,8 +340,9 @@ const VMStateInfo vmstate_info_int32_equal = {
.put = put_int32,
};
-/* 32 bit int. Check that the received value is less than or equal to
- the one in the field */
+/* 32 bit int. Check that the received value is non-negative
+ * and less than or equal to the one in the field.
+ */
static int get_int32_le(QEMUFile *f, void *pv, size_t size)
{
@@ -349,7 +350,7 @@ static int get_int32_le(QEMUFile *f, void *pv, size_t size)
int32_t loaded;
qemu_get_sbe32s(f, &loaded);
- if (loaded <= *cur) {
+ if (loaded >= 0 && loaded <= *cur) {
*cur = loaded;
return 0;
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c Michael S. Tsirkin
@ 2014-03-31 15:40 ` Peter Maydell
2014-04-01 15:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
QEMU Developers, Anthony Liguori, Andreas Färber,
Dave Gilbert
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4531
>
> cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
> cpreg_vmstate_array_len will cause a buffer overflow.
>
> VMSTATE_INT32_LE was supposed to protect against this
> but doesn't because it doesn't validate that input is
> non-negative.
>
> Fix this macro to valide the value appropriately.
>
> The only other user of VMSTATE_INT32_LE doesn't
> ever use negative numbers so it doesn't care.
This fixes the bug, but it's rather unintuitive semantics
for an INT32_LE not to simply do a signed comparison...
Maybe rename the macro?
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c
2014-03-31 15:40 ` Peter Maydell
@ 2014-04-01 15:12 ` Michael S. Tsirkin
0 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-04-01 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Michael Roth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
QEMU Developers, Anthony Liguori, Andreas Färber,
Dave Gilbert
On Mon, Mar 31, 2014 at 04:40:41PM +0100, Peter Maydell wrote:
> On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> > CVE-2013-4531
> >
> > cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
> > cpreg_vmstate_array_len will cause a buffer overflow.
> >
> > VMSTATE_INT32_LE was supposed to protect against this
> > but doesn't because it doesn't validate that input is
> > non-negative.
> >
> > Fix this macro to valide the value appropriately.
> >
> > The only other user of VMSTATE_INT32_LE doesn't
> > ever use negative numbers so it doesn't care.
>
> This fixes the bug, but it's rather unintuitive semantics
> for an INT32_LE not to simply do a signed comparison...
> Maybe rename the macro?
>
> thanks
> -- PMM
Sure. I'll do it in a separate patch though.
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (11 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 17:11 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2) Michael S. Tsirkin
` (16 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-stable, dgilbert, mdroth
CVE-2013-4532
s->next_packet is read from wire as an index into s->rx[]. If
s->next_packet exceeds the length of s->rx[], the buffer can be
subsequently overrun with arbitrary data from the wire.
Fix this by failing migration if s->next_packet we read from
the wire exceeds this.
Similarly, validate rx_fifo against sizeof(s->rx[].data).
Finally, constrain rx len to a sensibly small positive
value, to avoid integer overruns when data model
later uses this value.
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d04e6a4..182fd3e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
{
stellaris_enet_state *s = (stellaris_enet_state *)opaque;
- int i;
+ int i, v;
if (version_id != 1)
return -EINVAL;
@@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
}
- s->next_packet = qemu_get_be32(f);
- s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
- s->rx_fifo_len = qemu_get_be32(f);
+ v = qemu_get_be32(f);
+ if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
+ return -EINVAL;
+ }
+ s->next_packet = v;
+ v = qemu_get_be32(f);
+ if (v < 0 || v >= sizeof(s->rx[i].data)) {
+ return -EINVAL;
+ }
+ s->rx_fifo = s->rx[s->next_packet].data + v;
+ v = qemu_get_be32(f);
+ /* Set limit low enough to avoid integer overflow when
+ * we do math on len later, but high enough to avoid
+ * truncating any packets.
+ */
+ if (v < 0 || v >= 0x100000) {
+ return -EINVAL;
+ }
+ s->rx_fifo_len = v;
return 0;
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 17:11 ` Dr. David Alan Gilbert
2014-03-31 20:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-03-31 17:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, Peter Maydell, qemu-stable, qemu-devel, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4532
>
> s->next_packet is read from wire as an index into s->rx[]. If
> s->next_packet exceeds the length of s->rx[], the buffer can be
> subsequently overrun with arbitrary data from the wire.
>
> Fix this by failing migration if s->next_packet we read from
> the wire exceeds this.
>
> Similarly, validate rx_fifo against sizeof(s->rx[].data).
>
> Finally, constrain rx len to a sensibly small positive
> value, to avoid integer overruns when data model
> later uses this value.
>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d04e6a4..182fd3e 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> {
> stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> - int i;
> + int i, v;
>
> if (version_id != 1)
> return -EINVAL;
> @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>
> }
The loop that's just off the top here is:
for (i = 0; i < 31; i++) {
s->rx[i].len = qemu_get_be32(f);
qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
}
Doesn't that 'len' need validating? I assume it's the size of the
packet in the fixed sized buffer? (??)
> - s->next_packet = qemu_get_be32(f);
> - s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> - s->rx_fifo_len = qemu_get_be32(f);
> + v = qemu_get_be32(f);
> + if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> + return -EINVAL;
> + }
> + s->next_packet = v;
> + v = qemu_get_be32(f);
> + if (v < 0 || v >= sizeof(s->rx[i].data)) {
> + return -EINVAL;
> + }
> + s->rx_fifo = s->rx[s->next_packet].data + v;
> + v = qemu_get_be32(f);
> + /* Set limit low enough to avoid integer overflow when
> + * we do math on len later, but high enough to avoid
> + * truncating any packets.
> + */
> + if (v < 0 || v >= 0x100000) {
> + return -EINVAL;
> + }
> + s->rx_fifo_len = v;
I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
the buffer (or I think it might be legal for the sum to point to the byte after the
buffer)?
My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
related to the current packet in-flight; although I've not quite convinced myself
about what is supposed to happen at the end of the packet (which is why
I say rx_fifo might point just at? the end.
Dave
>
> return 0;
> }
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration
2014-03-31 17:11 ` Dr. David Alan Gilbert
@ 2014-03-31 20:49 ` Michael S. Tsirkin
2014-03-31 21:13 ` Peter Maydell
0 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 20:49 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Peter Maydell, qemu-stable, qemu-devel, mdroth
On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > CVE-2013-4532
> >
> > s->next_packet is read from wire as an index into s->rx[]. If
> > s->next_packet exceeds the length of s->rx[], the buffer can be
> > subsequently overrun with arbitrary data from the wire.
> >
> > Fix this by failing migration if s->next_packet we read from
> > the wire exceeds this.
> >
> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
> >
> > Finally, constrain rx len to a sensibly small positive
> > value, to avoid integer overruns when data model
> > later uses this value.
> >
> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> > 1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> > index d04e6a4..182fd3e 100644
> > --- a/hw/net/stellaris_enet.c
> > +++ b/hw/net/stellaris_enet.c
> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> > static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> > {
> > stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> > - int i;
> > + int i, v;
> >
> > if (version_id != 1)
> > return -EINVAL;
> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> > qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >
> > }
>
> The loop that's just off the top here is:
> for (i = 0; i < 31; i++) {
> s->rx[i].len = qemu_get_be32(f);
> qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>
> }
>
> Doesn't that 'len' need validating? I assume it's the size of the
> packet in the fixed sized buffer? (??)
Not that I see where it's used as such.
> > - s->next_packet = qemu_get_be32(f);
> > - s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> > - s->rx_fifo_len = qemu_get_be32(f);
> > + v = qemu_get_be32(f);
> > + if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> > + return -EINVAL;
> > + }
> > + s->next_packet = v;
> > + v = qemu_get_be32(f);
> > + if (v < 0 || v >= sizeof(s->rx[i].data)) {
> > + return -EINVAL;
> > + }
> > + s->rx_fifo = s->rx[s->next_packet].data + v;
> > + v = qemu_get_be32(f);
> > + /* Set limit low enough to avoid integer overflow when
> > + * we do math on len later, but high enough to avoid
> > + * truncating any packets.
> > + */
> > + if (v < 0 || v >= 0x100000) {
> > + return -EINVAL;
> > + }
> > + s->rx_fifo_len = v;
>
> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
> the buffer (or I think it might be legal for the sum to point to the byte after the
> buffer)?
> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
> related to the current packet in-flight; although I've not quite convinced myself
> about what is supposed to happen at the end of the packet (which is why
> I say rx_fifo might point just at? the end.
>
> Dave
Actually I forgot why I wrote this last check.
Peter said we should and I thought I see the issue ...
But I no longer see what kind of damage can rx_fifo_len cause
unless validated.
I never see it used as a length - merely a counter to
detect when to increment next_packet.
And that seems to be done in a safe way ...
Peter, Dave, maybe one of you can confirm what's the
possible attack here?
> >
> > return 0;
> > }
> > --
> > MST
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration
2014-03-31 20:49 ` Michael S. Tsirkin
@ 2014-03-31 21:13 ` Peter Maydell
2014-04-01 15:19 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 21:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, qemu-stable, Dr. David Alan Gilbert, QEMU Developers
On 31 March 2014 21:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> > CVE-2013-4532
>> >
>> > s->next_packet is read from wire as an index into s->rx[]. If
>> > s->next_packet exceeds the length of s->rx[], the buffer can be
>> > subsequently overrun with arbitrary data from the wire.
>> >
>> > Fix this by failing migration if s->next_packet we read from
>> > the wire exceeds this.
>> >
>> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
>> >
>> > Finally, constrain rx len to a sensibly small positive
>> > value, to avoid integer overruns when data model
>> > later uses this value.
>> >
>> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> > hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
>> > 1 file changed, 20 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
>> > index d04e6a4..182fd3e 100644
>> > --- a/hw/net/stellaris_enet.c
>> > +++ b/hw/net/stellaris_enet.c
>> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>> > static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>> > {
>> > stellaris_enet_state *s = (stellaris_enet_state *)opaque;
>> > - int i;
>> > + int i, v;
>> >
>> > if (version_id != 1)
>> > return -EINVAL;
>> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>> > qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>> >
>> > }
>>
>> The loop that's just off the top here is:
>> for (i = 0; i < 31; i++) {
>> s->rx[i].len = qemu_get_be32(f);
>> qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>>
>> }
>>
>> Doesn't that 'len' need validating? I assume it's the size of the
>> packet in the fixed sized buffer? (??)
>
> Not that I see where it's used as such.
In the DATA case of stellaris_enet_read() -- when the current
rx_fifo_len goes to zero we will uncritically set rx_fifo_len to
s->rx[s->next_packet].len. So we must validate that it's between
0 and 2048 (the size of the rx[].data array), otherwise further
reads from DATA will be able to run off the end of the data array
for the following packet.
>> > - s->next_packet = qemu_get_be32(f);
>> > - s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
>> > - s->rx_fifo_len = qemu_get_be32(f);
>> > + v = qemu_get_be32(f);
>> > + if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
>> > + return -EINVAL;
>> > + }
>> > + s->next_packet = v;
>> > + v = qemu_get_be32(f);
>> > + if (v < 0 || v >= sizeof(s->rx[i].data)) {
>> > + return -EINVAL;
>> > + }
>> > + s->rx_fifo = s->rx[s->next_packet].data + v;
>> > + v = qemu_get_be32(f);
>> > + /* Set limit low enough to avoid integer overflow when
>> > + * we do math on len later, but high enough to avoid
>> > + * truncating any packets.
>> > + */
>> > + if (v < 0 || v >= 0x100000) {
>> > + return -EINVAL;
>> > + }
>> > + s->rx_fifo_len = v;
>>
>> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
>> the buffer (or I think it might be legal for the sum to point to the byte after the
>> buffer)?
>> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
>> related to the current packet in-flight; although I've not quite convinced myself
>> about what is supposed to happen at the end of the packet (which is why
>> I say rx_fifo might point just at? the end.
> Actually I forgot why I wrote this last check.
> Peter said we should and I thought I see the issue ...
> But I no longer see what kind of damage can rx_fifo_len cause
> unless validated.
Again, look at the DATA read logic. Every time the guest does a
DATA read, we read from the four bytes at s->rx_fifo, increment
rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len
eventually goes to zero we will (on the subsequent read) reset
both rx_fifo and rx_fifo_len from the next packet in the rx queue.
So if the incoming data sets rx_fifo_len to (let us say) 0x10000,
then the guest can cause us to read well off the end of the rx data
array. This means your check isn't tight enough -- we need to
ensure that rx_fifo and rx_fifo_len between them define a window
into the rx data and nowhere else. As David says this means you
need:
v1 = qemu_get_be32(f);
if (v1 < 0 || v1 > sizeof(s->rx[i].data)) {
return -EINVAL;
}
s->rx_fifo = s->rx[s->next_packet].data + v1;
v2 = qemu_get_be32(f);
if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) {
return -EINVAL;
}
s->rx_fifo_len = v2;
The max check on v1 is actually only there to ensure that we
don't have to think about integer overflow when we do the
upper-bound check on v1 + v2. Note that v1 == sizeof(array)
is OK if (and only if) v2 == 0.
An assert in stellaris_enet_receive() that the net code never
hands us a packet we can't fit in the array wouldn't go amiss
either, but that's a separate issue.
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration
2014-03-31 21:13 ` Peter Maydell
@ 2014-04-01 15:19 ` Michael S. Tsirkin
0 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-04-01 15:19 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Michael Roth, Dr. David Alan Gilbert, qemu-stable
On Mon, Mar 31, 2014 at 10:13:04PM +0100, Peter Maydell wrote:
> On 31 March 2014 21:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
> >> * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> > CVE-2013-4532
> >> >
> >> > s->next_packet is read from wire as an index into s->rx[]. If
> >> > s->next_packet exceeds the length of s->rx[], the buffer can be
> >> > subsequently overrun with arbitrary data from the wire.
> >> >
> >> > Fix this by failing migration if s->next_packet we read from
> >> > the wire exceeds this.
> >> >
> >> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
> >> >
> >> > Finally, constrain rx len to a sensibly small positive
> >> > value, to avoid integer overruns when data model
> >> > later uses this value.
> >> >
> >> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >> > hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> >> > 1 file changed, 20 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> >> > index d04e6a4..182fd3e 100644
> >> > --- a/hw/net/stellaris_enet.c
> >> > +++ b/hw/net/stellaris_enet.c
> >> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> >> > static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >> > {
> >> > stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> >> > - int i;
> >> > + int i, v;
> >> >
> >> > if (version_id != 1)
> >> > return -EINVAL;
> >> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >> > qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >> >
> >> > }
> >>
> >> The loop that's just off the top here is:
> >> for (i = 0; i < 31; i++) {
> >> s->rx[i].len = qemu_get_be32(f);
> >> qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >>
> >> }
> >>
> >> Doesn't that 'len' need validating? I assume it's the size of the
> >> packet in the fixed sized buffer? (??)
> >
> > Not that I see where it's used as such.
>
> In the DATA case of stellaris_enet_read() -- when the current
> rx_fifo_len goes to zero we will uncritically set rx_fifo_len to
> s->rx[s->next_packet].len. So we must validate that it's between
> 0 and 2048 (the size of the rx[].data array), otherwise further
> reads from DATA will be able to run off the end of the data array
> for the following packet.
>
> >> > - s->next_packet = qemu_get_be32(f);
> >> > - s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> >> > - s->rx_fifo_len = qemu_get_be32(f);
> >> > + v = qemu_get_be32(f);
> >> > + if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> >> > + return -EINVAL;
> >> > + }
> >> > + s->next_packet = v;
> >> > + v = qemu_get_be32(f);
> >> > + if (v < 0 || v >= sizeof(s->rx[i].data)) {
> >> > + return -EINVAL;
> >> > + }
> >> > + s->rx_fifo = s->rx[s->next_packet].data + v;
> >> > + v = qemu_get_be32(f);
> >> > + /* Set limit low enough to avoid integer overflow when
> >> > + * we do math on len later, but high enough to avoid
> >> > + * truncating any packets.
> >> > + */
> >> > + if (v < 0 || v >= 0x100000) {
> >> > + return -EINVAL;
> >> > + }
> >> > + s->rx_fifo_len = v;
> >>
> >> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
> >> the buffer (or I think it might be legal for the sum to point to the byte after the
> >> buffer)?
> >> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
> >> related to the current packet in-flight; although I've not quite convinced myself
> >> about what is supposed to happen at the end of the packet (which is why
> >> I say rx_fifo might point just at? the end.
>
> > Actually I forgot why I wrote this last check.
> > Peter said we should and I thought I see the issue ...
> > But I no longer see what kind of damage can rx_fifo_len cause
> > unless validated.
>
> Again, look at the DATA read logic. Every time the guest does a
> DATA read, we read from the four bytes at s->rx_fifo, increment
> rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len
> eventually goes to zero we will (on the subsequent read) reset
> both rx_fifo and rx_fifo_len from the next packet in the rx queue.
> So if the incoming data sets rx_fifo_len to (let us say) 0x10000,
> then the guest can cause us to read well off the end of the rx data
> array. This means your check isn't tight enough -- we need to
> ensure that rx_fifo and rx_fifo_len between them define a window
> into the rx data and nowhere else. As David says this means you
> need:
>
> v1 = qemu_get_be32(f);
> if (v1 < 0 || v1 > sizeof(s->rx[i].data)) {
> return -EINVAL;
> }
> s->rx_fifo = s->rx[s->next_packet].data + v1;
> v2 = qemu_get_be32(f);
> if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) {
> return -EINVAL;
> }
> s->rx_fifo_len = v2;
>
> The max check on v1 is actually only there to ensure that we
> don't have to think about integer overflow when we do the
> upper-bound check on v1 + v2. Note that v1 == sizeof(array)
> is OK if (and only if) v2 == 0.
>
> An assert in stellaris_enet_receive() that the net code never
> hands us a packet we can't fit in the array wouldn't go amiss
> either, but that's a separate issue.
>
> thanks
> -- PMM
Got it now, thanks a lot for the explanation.
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (12 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-04-01 9:43 ` Dr. David Alan Gilbert
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3) Michael S. Tsirkin
` (15 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-stable, dgilbert, mdroth
CVE-2013-4532
s->tx_fifo_len is read from the wire and later used as an index into
s->tx_fifo[] when a DATA command is issued by the guest. If
s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
than 0, the buffer can be overrun/underrun by arbitrary data written out
by the guest upon resuming its execution.
Fix this by failing migration if the value from the wire would make
guest access the array out of bounds.
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/stellaris_enet.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 182fd3e..aed00fd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
{
stellaris_enet_state *s = (stellaris_enet_state *)opaque;
- int i, v;
+ int i, v, sz;
if (version_id != 1)
return -EINVAL;
@@ -374,7 +374,13 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
s->mrxd = qemu_get_be32(f);
s->np = qemu_get_be32(f);
s->tx_frame_len = qemu_get_be32(f);
- s->tx_fifo_len = qemu_get_be32(f);
+ v = qemu_get_be32(f);
+ /* How many bytes does data use in tx fifo. */
+ sz = s->tx_frame_len == -1 ? 2 : 4;
+ if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
+ return -EINVAL;
+ }
+ s->tx_fifo_len = v;
qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
for (i = 0; i < 31; i++) {
s->rx[i].len = qemu_get_be32(f);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2) Michael S. Tsirkin
@ 2014-04-01 9:43 ` Dr. David Alan Gilbert
2014-04-01 10:05 ` Peter Maydell
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 9:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, Peter Maydell, qemu-stable, qemu-devel, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4532
>
> s->tx_fifo_len is read from the wire and later used as an index into
> s->tx_fifo[] when a DATA command is issued by the guest. If
> s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
> than 0, the buffer can be overrun/underrun by arbitrary data written out
> by the guest upon resuming its execution.
>
> Fix this by failing migration if the value from the wire would make
> guest access the array out of bounds.
>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/stellaris_enet.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 182fd3e..aed00fd 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> {
> stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> - int i, v;
> + int i, v, sz;
>
> if (version_id != 1)
> return -EINVAL;
> @@ -374,7 +374,13 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> s->mrxd = qemu_get_be32(f);
> s->np = qemu_get_be32(f);
> s->tx_frame_len = qemu_get_be32(f);
> - s->tx_fifo_len = qemu_get_be32(f);
> + v = qemu_get_be32(f);
> + /* How many bytes does data use in tx fifo. */
> + sz = s->tx_frame_len == -1 ? 2 : 4;
> + if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
> + return -EINVAL;
> + }
> + s->tx_fifo_len = v;
> qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' write
case is pretty hairy.
Dave
> for (i = 0; i < 31; i++) {
> s->rx[i].len = qemu_get_be32(f);
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)
2014-04-01 9:43 ` Dr. David Alan Gilbert
@ 2014-04-01 10:05 ` Peter Maydell
2014-04-01 11:52 ` Peter Maydell
0 siblings, 1 reply; 81+ messages in thread
From: Peter Maydell @ 2014-04-01 10:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Michael Roth, qemu-stable, QEMU Developers, Michael S. Tsirkin
On 1 April 2014 10:43, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> CVE-2013-4532
>> @@ -374,7 +374,13 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>> s->mrxd = qemu_get_be32(f);
>> s->np = qemu_get_be32(f);
>> s->tx_frame_len = qemu_get_be32(f);
>> - s->tx_fifo_len = qemu_get_be32(f);
>> + v = qemu_get_be32(f);
>> + /* How many bytes does data use in tx fifo. */
>> + sz = s->tx_frame_len == -1 ? 2 : 4;
>> + if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
>> + return -EINVAL;
>> + }
>> + s->tx_fifo_len = v;
>> qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
>
> I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' write
> case is pretty hairy.
It's not that complicated. The first word of DATA has
the frame length in the bottom 16 bits, and the first
16 bits of actual data. All the subsequent words written
to DATA are part of the actual data.
However, I think this checking code is wrong. We
need to check tx_frame_len as well -- otherwise the
incoming data can simply set a hugely oversize
frame length and then have the guest stuff data
into DATA until we overrun the tx_fifo. As with the
rx fifo, I think the right approach here is to actually
check that the incoming migration data satisfies the
invariants:
tx_frame_len == -1
OR
tx_frame_len >= 0 && tx_frame_len <= ARRAY_SIZE(tx_fifo)
&& tx_fifo_len >= 0 && tx_fifo_len <= tx_frame_len - 4
(We don't have to have funny ?2:4 logic because the
case where we only write 2 bytes is when tx_frame_len
is -1, which will also reinitialize tx_fifo_len and
write strictly to the start of the fifo.)
But note that there seems to be a bug or two in
the DATA read logic: our cutoff for tx frame too
long is tx_frame_len > 2032, but for the limit
case of 2032, if we add 14 for the ethernet header
and 4 for explicit CRC then we get 2050, which is
slightly larger than the tx_fifo buffer... I think
we either need to wind that 2032 value down or
enlarge the tx_fifo buffer, or possibly just tweak
the logic not to try to actually write the CRC bytes
into the FIFO since we promptly ignore them -- need
to check the h/w datasheet to find out which.
Also a bug I think is that the PADEN handling code
is setting tx_fifo_len to 60 for short packets, which
will have no effect; I'm pretty sure it should be
setting tx_frame_len instead. That's just wrong
behaviour rather than an overrun issue though.
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)
2014-04-01 10:05 ` Peter Maydell
@ 2014-04-01 11:52 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-04-01 11:52 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Michael Roth, qemu-stable, QEMU Developers, Michael S. Tsirkin
On 1 April 2014 11:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> But note that there seems to be a bug or two in
> the DATA read logic: our cutoff for tx frame too
> long is tx_frame_len > 2032, but for the limit
> case of 2032, if we add 14 for the ethernet header
> and 4 for explicit CRC then we get 2050, which is
> slightly larger than the tx_fifo buffer... I think
> we either need to wind that 2032 value down or
> enlarge the tx_fifo buffer, or possibly just tweak
> the logic not to try to actually write the CRC bytes
> into the FIFO since we promptly ignore them -- need
> to check the h/w datasheet to find out which.
The datasheet isn't entirely clear on this point.
It is fairly definite that the TX overrun check is
done for tx_frame_len written as >2032, but it
doesn't really say what happens if there's not
enough space for the CRC. I rather suspect everybody
is using this device with auto-CRC enabled anyway,
and I have neither the hardware nor the time to
experiment with it. I'll put together a patch that
does something plausible and doesn't allow data
buffer overruns; roughly, we separate the check for
"can we put this word into the FIFO?" from the
one for "should we transmit the packet now?". This
is actually closer to the h/w behaviour, because the
h/w doesn't auto-transmit the packet on a full FIFO
in all cases -- unless you set the transmit-start
threshold you have to explicitly write to a different
register to say "now start transmitting".
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (13 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2) Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-04-01 9:51 ` Dr. David Alan Gilbert
2014-04-01 14:42 ` Eric Blake
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration Michael S. Tsirkin
` (14 subsequent siblings)
29 siblings, 2 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-stable, dgilbert, mdroth
CVE-2013-4532
s->tx_frame_len is read from the wire and can later used as an index
into s->tx_fifo[] for memset() when a DATA command is issued by the guest.
In this case s->tx_frame_len is checked to avoid an overrun, but if the
value is negative a subsequently executed guest can underrun the buffer
with zeros via the memset() call.
Additionally, tx_frame_len is used to validate that tx_fifo_len
doesn't exceed the fifo bounds - the assumption being that data model
never makes it exceed 2032.
Fix this by failing migration if the incoming value of s->tx_frame_len
is less than -1 (the emulation code allows from -1 as a special case)
or if it exceeds 2032.
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/stellaris_enet.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index aed00fd..90ff950 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
s->mtxd = qemu_get_be32(f);
s->mrxd = qemu_get_be32(f);
s->np = qemu_get_be32(f);
- s->tx_frame_len = qemu_get_be32(f);
+ v = qemu_get_be32(f);
+ if (v < -1 || s->tx_frame_len > 2032) {
+ return -EINVAL;
+ }
+ s->tx_frame_len = v;
v = qemu_get_be32(f);
/* How many bytes does data use in tx fifo. */
sz = s->tx_frame_len == -1 ? 2 : 4;
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3) Michael S. Tsirkin
@ 2014-04-01 9:51 ` Dr. David Alan Gilbert
2014-04-01 10:06 ` Peter Maydell
2014-04-01 14:42 ` Eric Blake
1 sibling, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 9:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, Peter Maydell, qemu-stable, qemu-devel, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4532
>
> s->tx_frame_len is read from the wire and can later used as an index
> into s->tx_fifo[] for memset() when a DATA command is issued by the guest.
>
> In this case s->tx_frame_len is checked to avoid an overrun, but if the
> value is negative a subsequently executed guest can underrun the buffer
> with zeros via the memset() call.
>
> Additionally, tx_frame_len is used to validate that tx_fifo_len
> doesn't exceed the fifo bounds - the assumption being that data model
> never makes it exceed 2032.
>
> Fix this by failing migration if the incoming value of s->tx_frame_len
> is less than -1 (the emulation code allows from -1 as a special case)
> or if it exceeds 2032.
>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/stellaris_enet.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index aed00fd..90ff950 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> s->mtxd = qemu_get_be32(f);
> s->mrxd = qemu_get_be32(f);
> s->np = qemu_get_be32(f);
> - s->tx_frame_len = qemu_get_be32(f);
> + v = qemu_get_be32(f);
> + if (v < -1 || s->tx_frame_len > 2032) {
> + return -EINVAL;
> + }
> + s->tx_frame_len = v;
I think that's wrong; 'stellaris_enet_write' does the following:
s->tx_frame_len = value & 0xffff;
if (s->tx_frame_len > 2032) {
DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
s->tx_frame_len = 0;
s->ris |= SE_INT_TXER;
stellaris_enet_update(s);
} else {
DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
/* The value written does not include the ethernet header. */
s->tx_frame_len += 14;
if ((s->tctl & SE_TCTL_CRC) == 0)
s->tx_frame_len += 4;
So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
at this point, and if the CRC flag is set then another 4. Thus it seems a user
can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying
given the buffer is only 2048 bytes.
Dave
> v = qemu_get_be32(f);
> /* How many bytes does data use in tx fifo. */
> sz = s->tx_frame_len == -1 ? 2 : 4;
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-04-01 9:51 ` Dr. David Alan Gilbert
@ 2014-04-01 10:06 ` Peter Maydell
2014-04-01 15:22 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Peter Maydell @ 2014-04-01 10:06 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Michael Roth, qemu-stable, QEMU Developers, Michael S. Tsirkin
On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
> at this point, and if the CRC flag is set then another 4. Thus it seems a user
> can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying
> given the buffer is only 2048 bytes.
Yep, see my equivalent remarks in the other patch.
Michael -- can we please squash these two patches into one?
It's really hard to review the code for correctness when
half the logic for dealing with the tx fifo is in a
different patch...
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-04-01 10:06 ` Peter Maydell
@ 2014-04-01 15:22 ` Michael S. Tsirkin
2014-04-01 15:56 ` Peter Maydell
0 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-04-01 15:22 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, qemu-stable, Dr. David Alan Gilbert, Michael Roth
On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote:
> On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
> > at this point, and if the CRC flag is set then another 4. Thus it seems a user
> > can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying
> > given the buffer is only 2048 bytes.
>
>
> Yep, see my equivalent remarks in the other patch.
>
> Michael -- can we please squash these two patches into one?
> It's really hard to review the code for correctness when
> half the logic for dealing with the tx fifo is in a
> different patch...
>
> thanks
> -- PMM
Will do - part 1 as well?
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-04-01 15:22 ` Michael S. Tsirkin
@ 2014-04-01 15:56 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-04-01 15:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: QEMU Developers, qemu-stable, Dr. David Alan Gilbert, Michael Roth
On 1 April 2014 16:22, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote:
>> Michael -- can we please squash these two patches into one?
>> It's really hard to review the code for correctness when
>> half the logic for dealing with the tx fifo is in a
>> different patch...
> Will do - part 1 as well?
No, that's OK, it's dealing with the rx logic rather than
the tx logic.
However, I've been writing some patches this afternoon
which get rid of tx_frame_len completely, so you might
want to wait for those...
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3) Michael S. Tsirkin
2014-04-01 9:51 ` Dr. David Alan Gilbert
@ 2014-04-01 14:42 ` Eric Blake
1 sibling, 0 replies; 81+ messages in thread
From: Eric Blake @ 2014-04-01 14:42 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: mdroth, Peter Maydell, qemu-stable, dgilbert
[-- Attachment #1: Type: text/plain, Size: 216 bytes --]
On 03/31/2014 08:16 AM, Michael S. Tsirkin wrote:
> CVE-2013-4532
s/orerrun/overrun/ in the subject
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (14 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3) Michael S. Tsirkin
@ 2014-03-31 14:16 ` Michael S. Tsirkin
2014-03-31 16:09 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 17/30] openpic: " Michael S. Tsirkin
` (13 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, dgilbert, Anthony Liguori, mdroth
From: Michael Roth <mdroth@linux.vnet.ibm.com>
CVE-2013-6399
vdev->queue_sel is read from the wire, and later used in the
emulation code as an index into vdev->vq[]. If the value of
vdev->queue_sel exceeds the length of vdev->vq[], currently
allocated to be VIRTIO_PCI_QUEUE_MAX elements, subsequent PIO
operations such as VIRTIO_PCI_QUEUE_PFN can be used to overrun
the buffer with arbitrary data originating from the source.
Fix this by failing migration if the value from the wire exceeds
VIRTIO_PCI_QUEUE_MAX.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9008430..bcbfbb2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -907,6 +907,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
qemu_get_8s(f, &vdev->status);
qemu_get_8s(f, &vdev->isr);
qemu_get_be16s(f, &vdev->queue_sel);
+ if (vdev->queue_sel >= VIRTIO_PCI_QUEUE_MAX) {
+ return -1;
+ }
qemu_get_be32s(f, &features);
if (virtio_set_features(vdev, features) < 0) {
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 16:09 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 16:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, Anthony Liguori, QEMU Developers, Dave Gilbert,
qemu-stable
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-6399
>
> vdev->queue_sel is read from the wire, and later used in the
> emulation code as an index into vdev->vq[]. If the value of
> vdev->queue_sel exceeds the length of vdev->vq[], currently
> allocated to be VIRTIO_PCI_QUEUE_MAX elements, subsequent PIO
> operations such as VIRTIO_PCI_QUEUE_PFN can be used to overrun
> the buffer with arbitrary data originating from the source.
>
> Fix this by failing migration if the value from the wire exceeds
> VIRTIO_PCI_QUEUE_MAX.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/virtio/virtio.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9008430..bcbfbb2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -907,6 +907,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> qemu_get_8s(f, &vdev->status);
> qemu_get_8s(f, &vdev->isr);
> qemu_get_be16s(f, &vdev->queue_sel);
> + if (vdev->queue_sel >= VIRTIO_PCI_QUEUE_MAX) {
> + return -1;
> + }
> qemu_get_be32s(f, &features);
>
> if (virtio_set_features(vdev, features) < 0) {
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
We should probably rename VIRTIO_PCI_QUEUE_MAX at some
point -- it's not PCI transport specific.
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 17/30] openpic: avoid buffer overrun on incoming migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (15 preceding siblings ...)
2014-03-31 14:16 ` [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:55 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping Michael S. Tsirkin
` (12 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Crosthwaite, mdroth, qemu-stable, Alexander Graf,
Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=,
dgilbert
From: Michael Roth <mdroth@linux.vnet.ibm.com>
CVE-2013-4534
opp->nb_cpus is read from the wire and used to determine how many
IRQDest elements to read into opp->dst[]. If the value exceeds the
length of opp->dst[], MAX_CPU, opp->dst[] can be overrun with arbitrary
data from the wire.
Fix this by failing migration if the value read from the wire exceeds
MAX_CPU.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/intc/openpic.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index be76fbd..8cb16da 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1429,6 +1429,9 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
qemu_get_be32s(f, &opp->tfrr);
qemu_get_be32s(f, &opp->nb_cpus);
+ if (opp->nb_cpus > MAX_CPU) {
+ return -EINVAL;
+ }
for (i = 0; i < opp->nb_cpus; i++) {
qemu_get_sbe32s(f, &opp->dst[i].ctpr);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 17/30] openpic: avoid buffer overrun on incoming migration
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 17/30] openpic: " Michael S. Tsirkin
@ 2014-03-31 15:55 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Crosthwaite, Michael Roth, QEMU Developers, Alexander Graf,
qemu-stable, Paolo Bonzini, Andreas Färber, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-4534
>
> opp->nb_cpus is read from the wire and used to determine how many
> IRQDest elements to read into opp->dst[]. If the value exceeds the
> length of opp->dst[], MAX_CPU, opp->dst[] can be overrun with arbitrary
> data from the wire.
>
> Fix this by failing migration if the value read from the wire exceeds
> MAX_CPU.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/intc/openpic.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index be76fbd..8cb16da 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1429,6 +1429,9 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
> qemu_get_be32s(f, &opp->tfrr);
>
> qemu_get_be32s(f, &opp->nb_cpus);
> + if (opp->nb_cpus > MAX_CPU) {
> + return -EINVAL;
> + }
nb_cpus is a device property so we shouldn't be reading
it off the wire at all. I would suggest treating it as
a "read but ignore value", assuming we care about migration
back-compatibility for this device, or possibly "read and
confirm it matches the value we already have".
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (16 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 17/30] openpic: " Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-04-01 9:10 ` Amit Shah
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration Michael S. Tsirkin
` (11 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, dgilbert, Anthony Liguori, mdroth
CVE-2013-4535
CVE-2013-4536
Both virtio-block and virtio-serial read,
VirtQueueElements are read in as buffers, and passed to
virtqueue_map_sg(), where num_sg is taken from the wire and can force
writes to indicies beyond VIRTQUEUE_MAX_SIZE.
To fix, validate num_sg.
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bcbfbb2..003b6ad 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -430,6 +430,12 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
unsigned int i;
hwaddr len;
+ if (num_sg >= VIRTQUEUE_MAX_SIZE) {
+ error_report("virtio: map attempt out of bounds: %d > %d",
+ num_sg, VIRTQUEUE_MAX_SIZE);
+ exit(1);
+ }
+
for (i = 0; i < num_sg; i++) {
len = sg[i].iov_len;
sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping Michael S. Tsirkin
@ 2014-04-01 9:10 ` Amit Shah
0 siblings, 0 replies; 81+ messages in thread
From: Amit Shah @ 2014-04-01 9:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, Anthony Liguori, qemu-devel, dgilbert, qemu-stable
On (Mon) 31 Mar 2014 [17:17:05], Michael S. Tsirkin wrote:
> CVE-2013-4535
> CVE-2013-4536
>
> Both virtio-block and virtio-serial read,
> VirtQueueElements are read in as buffers, and passed to
> virtqueue_map_sg(), where num_sg is taken from the wire and can force
> writes to indicies beyond VIRTQUEUE_MAX_SIZE.
>
> To fix, validate num_sg.
>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/virtio/virtio.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index bcbfbb2..003b6ad 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -430,6 +430,12 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> unsigned int i;
> hwaddr len;
>
> + if (num_sg >= VIRTQUEUE_MAX_SIZE) {
> + error_report("virtio: map attempt out of bounds: %d > %d",
> + num_sg, VIRTQUEUE_MAX_SIZE);
> + exit(1);
Doesn't compile; needs to be %zd because num_sg is size_t
Amit
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (17 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:29 ` Peter Maydell
2014-03-31 17:26 ` Don Koch
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load Michael S. Tsirkin
` (10 subsequent siblings)
29 siblings, 2 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-stable, dgilbert, Don Koch, mdroth
CVE-2013-4533
s->rx_level is read from the wire and used to determine how many bytes
to subsequently read into s->rx_fifo[]. If s->rx_level exceeds the
length of s->rx_fifo[] the buffer can be overrun with arbitrary data
from the wire.
Fix this by validating rx_level against the size of s->rx_fifo.
Cc: Don Koch <dkoch@verizon.com>
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/arm/pxa2xx.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 0429148..e0cd847 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -732,7 +732,7 @@ static void pxa2xx_ssp_save(QEMUFile *f, void *opaque)
static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
{
PXA2xxSSPState *s = (PXA2xxSSPState *) opaque;
- int i;
+ int i, v;
s->enable = qemu_get_be32(f);
@@ -746,7 +746,11 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id)
qemu_get_8s(f, &s->ssrsa);
qemu_get_8s(f, &s->ssacd);
- s->rx_level = qemu_get_byte(f);
+ v = qemu_get_byte(f);
+ if (v < 0 || v > ARRAY_SIZE(s->rx_fifo)) {
+ return -EINVAL;
+ }
+ s->rx_level = v;
s->rx_start = 0;
for (i = 0; i < s->rx_level; i ++)
s->rx_fifo[i] = qemu_get_byte(f);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 15:29 ` Peter Maydell
2014-03-31 17:26 ` Don Koch
1 sibling, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, qemu-stable, QEMU Developers, Don Koch, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4533
>
> s->rx_level is read from the wire and used to determine how many bytes
> to subsequently read into s->rx_fifo[]. If s->rx_level exceeds the
> length of s->rx_fifo[] the buffer can be overrun with arbitrary data
> from the wire.
>
> Fix this by validating rx_level against the size of s->rx_fifo.
>
> Cc: Don Koch <dkoch@verizon.com>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration Michael S. Tsirkin
2014-03-31 15:29 ` Peter Maydell
@ 2014-03-31 17:26 ` Don Koch
1 sibling, 0 replies; 81+ messages in thread
From: Don Koch @ 2014-03-31 17:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, mdroth, Don Koch, qemu-stable, dgilbert
On Mon, 31 Mar 2014 17:17:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> CVE-2013-4533
>
> s->rx_level is read from the wire and used to determine how many bytes
> to subsequently read into s->rx_fifo[]. If s->rx_level exceeds the
> length of s->rx_fifo[] the buffer can be overrun with arbitrary data
> from the wire.
>
> Fix this by validating rx_level against the size of s->rx_fifo.
>
> Cc: Don Koch <dkoch@verizon.com>
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Don Koch <dkoch@verizon.com>
-d
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (18 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:44 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun " Michael S. Tsirkin
` (9 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Crosthwaite, mdroth, qemu-stable, dgilbert,
=?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4537
s->arglen is taken from wire and used as idx
in ssi_sd_transfer().
Validate it before access.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/sd/ssi-sd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 3273c8a..d9c4aee 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -230,6 +230,9 @@ static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id)
for (i = 0; i < 5; i++)
s->response[i] = qemu_get_be32(f);
s->arglen = qemu_get_be32(f);
+ if (s->mode == SSI_SD_CMDARG && s->arglen > ARRAY_SIZE(s->cmdarg)) {
+ return -EINVAL;
+ }
s->response_pos = qemu_get_be32(f);
s->stopping = qemu_get_be32(f);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load Michael S. Tsirkin
@ 2014-03-31 15:44 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Peter Crosthwaite, Michael Roth, qemu-stable,
QEMU Developers, Andreas Färber, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4537
>
> s->arglen is taken from wire and used as idx
> in ssi_sd_transfer().
>
> Validate it before access.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/sd/ssi-sd.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 3273c8a..d9c4aee 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -230,6 +230,9 @@ static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id)
> for (i = 0; i < 5; i++)
> s->response[i] = qemu_get_be32(f);
> s->arglen = qemu_get_be32(f);
> + if (s->mode == SSI_SD_CMDARG && s->arglen > ARRAY_SIZE(s->cmdarg)) {
> + return -EINVAL;
> + }
> s->response_pos = qemu_get_be32(f);
> s->stopping = qemu_get_be32(f);
This is insufficient. You don't sanity check response_pos, which
means it can be used to overrun a buffer in the SSI_SD_RESPONSE
case if it's set to a negative value.
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (19 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:35 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun " Michael S. Tsirkin
` (8 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Crosthwaite, mdroth, qemu-stable, dgilbert, Gerd Hoffmann,
Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4538
s->cmd_len used as index in ssd0323_transfer() to store 32-bit field.
Possible this field might then be supplied by guest to overwrite a
return addr somewhere. Same for row/col fields, which are indicies into
framebuffer array.
To fix validate after load.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/display/ssd0323.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 971152e..b520c69 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -312,13 +312,22 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
return -EINVAL;
s->cmd_len = qemu_get_be32(f);
+ if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) {
+ return -EINVAL;
+ }
s->cmd = qemu_get_be32(f);
for (i = 0; i < 8; i++)
s->cmd_data[i] = qemu_get_be32(f);
s->row = qemu_get_be32(f);
+ if (s->row < 0 || s->row >= 80 ) {
+ return -EINVAL;
+ }
s->row_start = qemu_get_be32(f);
s->row_end = qemu_get_be32(f);
s->col = qemu_get_be32(f);
+ if (s->col < 0 || s->col >= 64 ) {
+ return -EINVAL;
+ }
s->col_start = qemu_get_be32(f);
s->col_end = qemu_get_be32(f);
s->redraw = qemu_get_be32(f);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun on invalid state load
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun " Michael S. Tsirkin
@ 2014-03-31 15:35 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Crosthwaite, Michael Roth, qemu-stable, QEMU Developers,
Gerd Hoffmann, Paolo Bonzini, Andreas Färber, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4538
>
> s->cmd_len used as index in ssd0323_transfer() to store 32-bit field.
> Possible this field might then be supplied by guest to overwrite a
> return addr somewhere. Same for row/col fields, which are indicies into
> framebuffer array.
>
> To fix validate after load.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/display/ssd0323.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index 971152e..b520c69 100644
> --- a/hw/display/ssd0323.c
> +++ b/hw/display/ssd0323.c
> @@ -312,13 +312,22 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
> return -EINVAL;
>
> s->cmd_len = qemu_get_be32(f);
> + if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) {
> + return -EINVAL;
> + }
> s->cmd = qemu_get_be32(f);
> for (i = 0; i < 8; i++)
> s->cmd_data[i] = qemu_get_be32(f);
> s->row = qemu_get_be32(f);
> + if (s->row < 0 || s->row >= 80 ) {
> + return -EINVAL;
> + }
> s->row_start = qemu_get_be32(f);
> s->row_end = qemu_get_be32(f);
> s->col = qemu_get_be32(f);
> + if (s->col < 0 || s->col >= 64 ) {
> + return -EINVAL;
> + }
> s->col_start = qemu_get_be32(f);
> s->col_end = qemu_get_be32(f);
> s->redraw = qemu_get_be32(f);
This isn't sufficient. You also need to validate that
the row/col_start/end are within bounds; otherwise the
guest can provoke an overrun by either setting the
_end field so large that the row++ increments just walk
off the end of the array, or by setting the _start
value to something bogus and then letting the "we hit
end of row" logic reset row to row_start.
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (20 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun " Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:39 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 23/30] zaurus: " Michael S. Tsirkin
` (7 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Stefan Hajnoczi, mdroth, qemu-stable, dgilbert,
Alex Bligh, Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4539
s->precision, nextprecision, function and nextfunction
come from wire and are used
as idx into resolution[] in TSC_CUT_RESOLUTION.
Validate after load to avoid buffer overrun.
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/input/tsc210x.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index 485c9e5..65a0d08 100644
--- a/hw/input/tsc210x.c
+++ b/hw/input/tsc210x.c
@@ -1070,9 +1070,21 @@ static int tsc210x_load(QEMUFile *f, void *opaque, int version_id)
s->enabled = qemu_get_byte(f);
s->host_mode = qemu_get_byte(f);
s->function = qemu_get_byte(f);
+ if (s->function > ARRAY_SIZE(mode_regs)) {
+ return -EINVAL;
+ }
s->nextfunction = qemu_get_byte(f);
+ if (s->nextfunction < 0 || s->nextfunction >= ARRAY_SIZE(mode_regs)) {
+ return -EINVAL;
+ }
s->precision = qemu_get_byte(f);
+ if (s->precision < 0 || s->precision >= ARRAY_SIZE(resolution)) {
+ return -EINVAL;
+ }
s->nextprecision = qemu_get_byte(f);
+ if (s->nextprecision < 0 || s->nextprecision >= ARRAY_SIZE(resolution)) {
+ return -EINVAL;
+ }
s->filter = qemu_get_byte(f);
s->pin_func = qemu_get_byte(f);
s->ref = qemu_get_byte(f);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun on invalid state load
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun " Michael S. Tsirkin
@ 2014-03-31 15:39 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, QEMU Developers, Michael Roth, qemu-stable,
Alex Bligh, Paolo Bonzini, Andreas Färber, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4539
>
> s->precision, nextprecision, function and nextfunction
> come from wire and are used
> as idx into resolution[] in TSC_CUT_RESOLUTION.
>
> Validate after load to avoid buffer overrun.
>
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/input/tsc210x.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
> index 485c9e5..65a0d08 100644
> --- a/hw/input/tsc210x.c
> +++ b/hw/input/tsc210x.c
> @@ -1070,9 +1070,21 @@ static int tsc210x_load(QEMUFile *f, void *opaque, int version_id)
> s->enabled = qemu_get_byte(f);
> s->host_mode = qemu_get_byte(f);
> s->function = qemu_get_byte(f);
> + if (s->function > ARRAY_SIZE(mode_regs)) {
> + return -EINVAL;
> + }
Why no check for negative values? Also, shouldn't
this be >=, like the checks below?
> s->nextfunction = qemu_get_byte(f);
> + if (s->nextfunction < 0 || s->nextfunction >= ARRAY_SIZE(mode_regs)) {
> + return -EINVAL;
> + }
> s->precision = qemu_get_byte(f);
> + if (s->precision < 0 || s->precision >= ARRAY_SIZE(resolution)) {
> + return -EINVAL;
> + }
> s->nextprecision = qemu_get_byte(f);
> + if (s->nextprecision < 0 || s->nextprecision >= ARRAY_SIZE(resolution)) {
> + return -EINVAL;
> + }
> s->filter = qemu_get_byte(f);
> s->pin_func = qemu_get_byte(f);
> s->ref = qemu_get_byte(f);
> --
> MST
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 23/30] zaurus: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (21 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun " Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-04-01 11:18 ` Dr. David Alan Gilbert
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load Michael S. Tsirkin
` (6 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Stefan Weil, qemu-stable, dgilbert, Stefan Hajnoczi,
Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4540
Within scoop_gpio_handler_update, if prev_level has a high bit set, then
we get bit > 16 and that causes a buffer overrun.
Since prev_level comes from wire indirectly, this can
happen on invalid state load.
Similarly for gpio_level and gpio_dir.
To fix, limit to 16 bit.
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/gpio/zaurus.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
index dc79a8b..8e2ce04 100644
--- a/hw/gpio/zaurus.c
+++ b/hw/gpio/zaurus.c
@@ -203,6 +203,15 @@ static bool is_version_0 (void *opaque, int version_id)
return version_id == 0;
}
+static bool vmstate_scoop_validate(void *opaque, int version_id)
+{
+ ScoopInfo *s = opaque;
+
+ return !(s->prev_level & 0xffff0000) &&
+ !(s->gpio_level & 0xffff0000) &&
+ !(s->gpio_dir & 0xffff0000);
+}
+
static const VMStateDescription vmstate_scoop_regs = {
.name = "scoop",
.version_id = 1,
@@ -215,6 +224,7 @@ static const VMStateDescription vmstate_scoop_regs = {
VMSTATE_UINT32(gpio_level, ScoopInfo),
VMSTATE_UINT32(gpio_dir, ScoopInfo),
VMSTATE_UINT32(prev_level, ScoopInfo),
+ VMSTATE_VALIDATE("irq levels are 16 bit", vmstate_scoop_validate),
VMSTATE_UINT16(mcr, ScoopInfo),
VMSTATE_UINT16(cdr, ScoopInfo),
VMSTATE_UINT16(ccr, ScoopInfo),
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 23/30] zaurus: fix buffer overrun on invalid state load
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 23/30] zaurus: " Michael S. Tsirkin
@ 2014-04-01 11:18 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 11:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, Stefan Weil, qemu-stable, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, =?UTF-8?q?Andreas=20F=C3=A4rber?=,
dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4540
>
> Within scoop_gpio_handler_update, if prev_level has a high bit set, then
> we get bit > 16 and that causes a buffer overrun.
>
> Since prev_level comes from wire indirectly, this can
> happen on invalid state load.
>
> Similarly for gpio_level and gpio_dir.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
(Without much understanding of the actual, apparently undocumented hardware)
> To fix, limit to 16 bit.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/gpio/zaurus.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
> index dc79a8b..8e2ce04 100644
> --- a/hw/gpio/zaurus.c
> +++ b/hw/gpio/zaurus.c
> @@ -203,6 +203,15 @@ static bool is_version_0 (void *opaque, int version_id)
> return version_id == 0;
> }
>
> +static bool vmstate_scoop_validate(void *opaque, int version_id)
> +{
> + ScoopInfo *s = opaque;
> +
> + return !(s->prev_level & 0xffff0000) &&
> + !(s->gpio_level & 0xffff0000) &&
> + !(s->gpio_dir & 0xffff0000);
> +}
> +
> static const VMStateDescription vmstate_scoop_regs = {
> .name = "scoop",
> .version_id = 1,
> @@ -215,6 +224,7 @@ static const VMStateDescription vmstate_scoop_regs = {
> VMSTATE_UINT32(gpio_level, ScoopInfo),
> VMSTATE_UINT32(gpio_dir, ScoopInfo),
> VMSTATE_UINT32(prev_level, ScoopInfo),
> + VMSTATE_VALIDATE("irq levels are 16 bit", vmstate_scoop_validate),
> VMSTATE_UINT16(mcr, ScoopInfo),
> VMSTATE_UINT16(cdr, ScoopInfo),
> VMSTATE_UINT16(ccr, ScoopInfo),
> --
> MST
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (22 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 23/30] zaurus: " Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:48 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 25/30] virtio-scsi: fix buffer overrun on invalid state load Michael S. Tsirkin
` (5 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, Gerd Hoffmann, dgilbert, mdroth
From: Gerd Hoffmann <kraxel@redhat.com>
CVE-2013-4541
s->setup_len and s->setup_index are fed into usb_packet_copy as
size/offset into s->data_buf, it's possible for invalid state to exploit
this to load arbitrary data.
setup_len and setup_index should be checked against data_buf size.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/usb/bus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index fe70429..8052bf1 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -53,6 +53,10 @@ static int usb_device_post_load(void *opaque, int version_id)
dev->setup_len >= sizeof(dev->data_buf)) {
return -EINVAL;
}
+ if (dev->setup_index >= sizeof(dev->data_buf) ||
+ dev->setup_len >= sizeof(dev->data_buf)) {
+ return -EINVAL;
+ }
return 0;
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load Michael S. Tsirkin
@ 2014-03-31 15:48 ` Peter Maydell
2014-04-01 6:23 ` Gerd Hoffmann
0 siblings, 1 reply; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 15:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, Gerd Hoffmann, QEMU Developers, Dave Gilbert, qemu-stable
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
>
> CVE-2013-4541
>
> s->setup_len and s->setup_index are fed into usb_packet_copy as
> size/offset into s->data_buf, it's possible for invalid state to exploit
> this to load arbitrary data.
>
> setup_len and setup_index should be checked against data_buf size.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/usb/bus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index fe70429..8052bf1 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -53,6 +53,10 @@ static int usb_device_post_load(void *opaque, int version_id)
> dev->setup_len >= sizeof(dev->data_buf)) {
> return -EINVAL;
> }
> + if (dev->setup_index >= sizeof(dev->data_buf) ||
> + dev->setup_len >= sizeof(dev->data_buf)) {
> + return -EINVAL;
> + }
> return 0;
> }
(1) This patch has already been applied; looks like a rebase
merge error meant you failed to drop it.
(2) Shouldn't we be checking for setup_index and setup_len
being negative as well?
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load
2014-03-31 15:48 ` Peter Maydell
@ 2014-04-01 6:23 ` Gerd Hoffmann
0 siblings, 0 replies; 81+ messages in thread
From: Gerd Hoffmann @ 2014-04-01 6:23 UTC (permalink / raw)
To: Peter Maydell
Cc: Michael Roth, qemu-stable, QEMU Developers, Dave Gilbert,
Michael S. Tsirkin
> > + if (dev->setup_index >= sizeof(dev->data_buf) ||
> > + dev->setup_len >= sizeof(dev->data_buf)) {
> > + return -EINVAL;
> > + }
> > return 0;
> > }
>
> (2) Shouldn't we be checking for setup_index and setup_len
> being negative as well?
Oops, they are signed, so yes, I guess we should. Or we can just make
them unsigned, they should never ever be negative. But I'm not fully
sure we can do that without breaking migration ...
cheers,
Gerd
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 25/30] virtio-scsi: fix buffer overrun on invalid state load
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (23 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 24/30] usb: sanity check setup_index+setup_len in post_load Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 26/30] savevm: fix potential segfault on invalid state Michael S. Tsirkin
` (4 subsequent siblings)
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Anthony Liguori, Paolo Bonzini,
=?UTF-8?q?Andreas=20F=C3=A4rber?=
CVE-2013-4542
hw/scsi/scsi-bus.c invokes load_request.
virtio_scsi_load_request does:
qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
this probably can make elem invalid, for example,
make in_num or out_num huge, then:
virtio_scsi_parse_req(s, vs->cmd_vqs[n], req);
will do:
if (req->elem.out_num > 1) {
qemu_sgl_init_external(req, &req->elem.out_sg[1],
&req->elem.out_addr[1],
req->elem.out_num - 1);
} else {
qemu_sgl_init_external(req, &req->elem.in_sg[1],
&req->elem.in_addr[1],
req->elem.in_num - 1);
}
and this will access out of array bounds.
Note: this adds security checks within assert calls since
SCSIBusInfo's load_request cannot fail.
For now simply disable builds with NDEBUG - there seems
to be little value in supporting these.
Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/scsi/virtio-scsi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0d7517..1752193 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -147,6 +147,15 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
qemu_get_be32s(f, &n);
assert(n < vs->conf.num_queues);
qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
+ /* TODO: add a way for SCSIBusInfo's load_request to fail,
+ * and fail migration instead of asserting here.
+ * When we do, we might be able to re-enable NDEBUG below.
+ */
+#ifdef NDEBUG
+#error building with NDEBUG is not supported
+#endif
+ assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
+ assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
virtio_scsi_parse_req(s, vs->cmd_vqs[n], req);
scsi_req_ref(sreq);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH v4 26/30] savevm: fix potential segfault on invalid state
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (24 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 25/30] virtio-scsi: fix buffer overrun on invalid state load Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 16:04 ` Peter Maydell
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest Michael S. Tsirkin
` (3 subsequent siblings)
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
dgilbert, =?UTF-8?q?Andreas=20F=C3=A4rber?=
savevm will segfault if version_id < vmsd->minimum_version_id &&
version_id >= vmsd->minimum_version_id_old
This calls through a NULL pointer. This is a bug (should
exit not crash).
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
vmstate.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/vmstate.c b/vmstate.c
index e1e9cae..5451fd2 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -67,6 +67,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return -EINVAL;
}
if (version_id < vmsd->minimum_version_id) {
+ if (!vmsd->load_state_old) {
+ return -EINVAL;
+ }
return vmsd->load_state_old(f, opaque, version_id);
}
if (vmsd->pre_load) {
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 26/30] savevm: fix potential segfault on invalid state
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 26/30] savevm: fix potential segfault on invalid state Michael S. Tsirkin
@ 2014-03-31 16:04 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2014-03-31 16:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Roth, Juan Quintela, Alexey Kardashevskiy, qemu-stable,
QEMU Developers, Andreas Färber, Dave Gilbert
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> savevm will segfault if version_id < vmsd->minimum_version_id &&
> version_id >= vmsd->minimum_version_id_old
...and the vmstate has no load_state_old handler.
> This calls through a NULL pointer. This is a bug (should
> exit not crash).
I'd previously assumed that this was a vmstate description
bug if it happened (ie that a vmstate with
minimum_version_id_old < minimum_version_id but no
load_state_old wasn't allowed).
Rather than failing migration here, wouldn't it be better
to either:
(a) diagnose the bug, by asserting at the earliest
opportunity
(b) define that the value of minimum_version_id_old is not
relevant unless load_state_old is set
I would strongly prefer (b) -- this would allow us to
remove the now-pointless setting of minimum_version_id_old
in huge numbers of vmstate structures. (Only five devices
make use of load_state_old: acpi, apic, i440fx, pit and
the ppc cpu).
thanks
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (25 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 26/30] savevm: fix potential segfault on invalid state Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:45 ` Dr. David Alan Gilbert
2014-04-01 11:33 ` Dr. David Alan Gilbert
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration Michael S. Tsirkin
` (2 subsequent siblings)
29 siblings, 2 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Stefan Hajnoczi, Dmitry Fleytman,
Paolo Bonzini, Anthony Liguori
From: Dmitry Fleytman <dmitry@daynix.com>
CVE-2013-4544
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5be807c..a4b5c11 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -52,6 +52,9 @@
#define VMXNET3_DEVICE_VERSION 0x1
#define VMXNET3_DEVICE_REVISION 0x1
+/* Number of interrupt vectors for INTx/MSI */
+#define VMXNET3_MAX_NMSIX_INTRS (1)
+
/* Macros for rings descriptors access */
#define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
(vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
@@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
(pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
}
+static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
+{
+ int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
+ if (idx >= max_ints) {
+ hw_error("Bad interrupt index: %d\n", idx);
+ }
+}
+
+static void vmxnet3_validate_interrupts(VMXNET3State *s)
+{
+ int i;
+
+ VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
+ vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
+
+ for (i = 0; i < s->txq_num; i++) {
+ int idx = s->txq_descr[i].intr_idx;
+ VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
+ vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+ }
+
+ for (i = 0; i < s->rxq_num; i++) {
+ int idx = s->rxq_descr[i].intr_idx;
+ VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
+ vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+ }
+}
+
static void vmxnet3_activate_device(VMXNET3State *s)
{
int i;
@@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
sizeof(s->rxq_descr[i].rxq_stats));
}
+ vmxnet3_validate_interrupts(s);
+
/* Make sure everything is in place before device activation */
smp_wmb();
@@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
}
}
-#define VMXNET3_MSI_NUM_VECTORS (1)
#define VMXNET3_MSI_OFFSET (0x50)
#define VMXNET3_USE_64BIT (true)
#define VMXNET3_PER_VECTOR_MASK (false)
@@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
PCIDevice *d = PCI_DEVICE(s);
int res;
- res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
+ res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
if (0 > res) {
VMW_WRPRN("Failed to initialize MSI, error %d", res);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest Michael S. Tsirkin
@ 2014-03-31 15:45 ` Dr. David Alan Gilbert
2014-04-01 9:54 ` Dmitry Fleytman
2014-04-01 11:33 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-03-31 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mdroth, qemu-stable, qemu-devel, Stefan Hajnoczi,
Dmitry Fleytman, Paolo Bonzini, Anthony Liguori
* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
>
> CVE-2013-4544
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5be807c..a4b5c11 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -52,6 +52,9 @@
> #define VMXNET3_DEVICE_VERSION 0x1
> #define VMXNET3_DEVICE_REVISION 0x1
>
> +/* Number of interrupt vectors for INTx/MSI */
> +#define VMXNET3_MAX_NMSIX_INTRS (1)
> +
> /* Macros for rings descriptors access */
> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> }
>
> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> +{
> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
Is that ?: the right way around?
> + if (idx >= max_ints) {
> + hw_error("Bad interrupt index: %d\n", idx);
> + }
> +}
> +
> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> +{
> + int i;
> +
> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +
> + for (i = 0; i < s->txq_num; i++) {
> + int idx = s->txq_descr[i].intr_idx;
> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> + }
> +
> + for (i = 0; i < s->rxq_num; i++) {
> + int idx = s->rxq_descr[i].intr_idx;
> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> + }
> +}
> +
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
> int i;
> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> sizeof(s->rxq_descr[i].rxq_stats));
> }
>
> + vmxnet3_validate_interrupts(s);
> +
> /* Make sure everything is in place before device activation */
> smp_wmb();
>
> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> }
> }
>
> -#define VMXNET3_MSI_NUM_VECTORS (1)
> #define VMXNET3_MSI_OFFSET (0x50)
> #define VMXNET3_USE_64BIT (true)
> #define VMXNET3_PER_VECTOR_MASK (false)
> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> PCIDevice *d = PCI_DEVICE(s);
> int res;
>
> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> if (0 > res) {
> VMW_WRPRN("Failed to initialize MSI, error %d", res);
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-03-31 15:45 ` Dr. David Alan Gilbert
@ 2014-04-01 9:54 ` Dmitry Fleytman
2014-04-01 10:03 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-01 9:54 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel, mdroth,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]
On Mar 31, 2014, at 18:45 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>>
>> CVE-2013-4544
>>
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5be807c..a4b5c11 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -52,6 +52,9 @@
>> #define VMXNET3_DEVICE_VERSION 0x1
>> #define VMXNET3_DEVICE_REVISION 0x1
>>
>> +/* Number of interrupt vectors for INTx/MSI */
>> +#define VMXNET3_MAX_NMSIX_INTRS (1)
>> +
>> /* Macros for rings descriptors access */
>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>> }
>>
>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>> +{
>> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>
> Is that ?: the right way around?
Yes, this is the right way around.
VMXNET3_MAX_NMSIX_INTRS - NMSIX is for Non-MSIx
>
>> + if (idx >= max_ints) {
>> + hw_error("Bad interrupt index: %d\n", idx);
>> + }
>> +}
>> +
>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> +{
>> + int i;
>> +
>> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>> +
>> + for (i = 0; i < s->txq_num; i++) {
>> + int idx = s->txq_descr[i].intr_idx;
>> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> + }
>> +
>> + for (i = 0; i < s->rxq_num; i++) {
>> + int idx = s->rxq_descr[i].intr_idx;
>> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> + }
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>> int i;
>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> sizeof(s->rxq_descr[i].rxq_stats));
>> }
>>
>> + vmxnet3_validate_interrupts(s);
>> +
>> /* Make sure everything is in place before device activation */
>> smp_wmb();
>>
>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>> }
>> }
>>
>> -#define VMXNET3_MSI_NUM_VECTORS (1)
>> #define VMXNET3_MSI_OFFSET (0x50)
>> #define VMXNET3_USE_64BIT (true)
>> #define VMXNET3_PER_VECTOR_MASK (false)
>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>> PCIDevice *d = PCI_DEVICE(s);
>> int res;
>>
>> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>> if (0 > res) {
>> VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> --
>> MST
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[-- Attachment #2: Type: text/html, Size: 5978 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-04-01 9:54 ` Dmitry Fleytman
@ 2014-04-01 10:03 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 10:03 UTC (permalink / raw)
To: Dmitry Fleytman
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel, mdroth,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
* Dmitry Fleytman (dmitry@daynix.com) wrote:
>
> On Mar 31, 2014, at 18:45 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> From: Dmitry Fleytman <dmitry@daynix.com>
> >>
> >> CVE-2013-4544
> >>
> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 5be807c..a4b5c11 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -52,6 +52,9 @@
> >> #define VMXNET3_DEVICE_VERSION 0x1
> >> #define VMXNET3_DEVICE_REVISION 0x1
> >>
> >> +/* Number of interrupt vectors for INTx/MSI */
> >> +#define VMXNET3_MAX_NMSIX_INTRS (1)
> >> +
> >> /* Macros for rings descriptors access */
> >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> >> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> >> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> >> }
> >>
> >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> >> +{
> >> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> >
> > Is that ?: the right way around?
>
> Yes, this is the right way around.
> VMXNET3_MAX_NMSIX_INTRS - NMSIX is for Non-MSIx
Ah, thank you - makes more sense now :-)
Dave
>
> >
> >> + if (idx >= max_ints) {
> >> + hw_error("Bad interrupt index: %d\n", idx);
> >> + }
> >> +}
> >> +
> >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> >> +{
> >> + int i;
> >> +
> >> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> >> +
> >> + for (i = 0; i < s->txq_num; i++) {
> >> + int idx = s->txq_descr[i].intr_idx;
> >> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> + }
> >> +
> >> + for (i = 0; i < s->rxq_num; i++) {
> >> + int idx = s->rxq_descr[i].intr_idx;
> >> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> + }
> >> +}
> >> +
> >> static void vmxnet3_activate_device(VMXNET3State *s)
> >> {
> >> int i;
> >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> >> sizeof(s->rxq_descr[i].rxq_stats));
> >> }
> >>
> >> + vmxnet3_validate_interrupts(s);
> >> +
> >> /* Make sure everything is in place before device activation */
> >> smp_wmb();
> >>
> >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> >> }
> >> }
> >>
> >> -#define VMXNET3_MSI_NUM_VECTORS (1)
> >> #define VMXNET3_MSI_OFFSET (0x50)
> >> #define VMXNET3_USE_64BIT (true)
> >> #define VMXNET3_PER_VECTOR_MASK (false)
> >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> >> PCIDevice *d = PCI_DEVICE(s);
> >> int res;
> >>
> >> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> >> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> >> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> >> if (0 > res) {
> >> VMW_WRPRN("Failed to initialize MSI, error %d", res);
> >> --
> >> MST
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest Michael S. Tsirkin
2014-03-31 15:45 ` Dr. David Alan Gilbert
@ 2014-04-01 11:33 ` Dr. David Alan Gilbert
2014-04-01 13:04 ` Dmitry Fleytman
1 sibling, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 11:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Dmitry Fleytman, Paolo Bonzini, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
>
> CVE-2013-4544
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5be807c..a4b5c11 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -52,6 +52,9 @@
> #define VMXNET3_DEVICE_VERSION 0x1
> #define VMXNET3_DEVICE_REVISION 0x1
>
> +/* Number of interrupt vectors for INTx/MSI */
> +#define VMXNET3_MAX_NMSIX_INTRS (1)
As per Dmitry's reply this is apparently the number of non-MSIX
interrupts; can we change the comment to make this clear.
> +
> /* Macros for rings descriptors access */
> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> }
>
> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> +{
> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> + if (idx >= max_ints) {
> + hw_error("Bad interrupt index: %d\n", idx);
> + }
Can we avoid hw_error here, we're using in this in state load, so I'm
OK at the thought of error_report, but then we should make
this return a bool to say it's failed and bubble this up so
that it just fails the post_load test like any other bad state load.
Dave
> +}
> +
> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> +{
> + int i;
> +
> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +
> + for (i = 0; i < s->txq_num; i++) {
> + int idx = s->txq_descr[i].intr_idx;
> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> + }
> +
> + for (i = 0; i < s->rxq_num; i++) {
> + int idx = s->rxq_descr[i].intr_idx;
> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> + }
> +}
> +
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
> int i;
> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> sizeof(s->rxq_descr[i].rxq_stats));
> }
>
> + vmxnet3_validate_interrupts(s);
> +
> /* Make sure everything is in place before device activation */
> smp_wmb();
>
> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> }
> }
>
> -#define VMXNET3_MSI_NUM_VECTORS (1)
> #define VMXNET3_MSI_OFFSET (0x50)
> #define VMXNET3_USE_64BIT (true)
> #define VMXNET3_PER_VECTOR_MASK (false)
> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> PCIDevice *d = PCI_DEVICE(s);
> int res;
>
> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> if (0 > res) {
> VMW_WRPRN("Failed to initialize MSI, error %d", res);
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-04-01 11:33 ` Dr. David Alan Gilbert
@ 2014-04-01 13:04 ` Dmitry Fleytman
2014-04-01 13:07 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-01 13:04 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel, mdroth,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 4126 bytes --]
On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>>
>> CVE-2013-4544
>>
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5be807c..a4b5c11 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -52,6 +52,9 @@
>> #define VMXNET3_DEVICE_VERSION 0x1
>> #define VMXNET3_DEVICE_REVISION 0x1
>>
>> +/* Number of interrupt vectors for INTx/MSI */
>> +#define VMXNET3_MAX_NMSIX_INTRS (1)
>
> As per Dmitry's reply this is apparently the number of non-MSIX
> interrupts; can we change the comment to make this clear.
Not sure how to change it.
There are three modes of operation:
1. INTx - 1 interrupt is used
2. MSI - 1 interrupt is used
3. MSIx - up to 25 interrupts are used.
This define covers 2 first modes of operation.
Would something like
/* Number of interrupt vectors for non-MSIx modes */
be better?
>
>> +
>> /* Macros for rings descriptors access */
>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>> }
>>
>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>> +{
>> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>> + if (idx >= max_ints) {
>> + hw_error("Bad interrupt index: %d\n", idx);
>> + }
>
> Can we avoid hw_error here, we're using in this in state load, so I'm
> OK at the thought of error_report, but then we should make
> this return a bool to say it's failed and bubble this up so
> that it just fails the post_load test like any other bad state load.
>
> Dave
>
>> +}
>> +
>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> +{
>> + int i;
>> +
>> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>> +
>> + for (i = 0; i < s->txq_num; i++) {
>> + int idx = s->txq_descr[i].intr_idx;
>> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> + }
>> +
>> + for (i = 0; i < s->rxq_num; i++) {
>> + int idx = s->rxq_descr[i].intr_idx;
>> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> + }
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>> int i;
>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> sizeof(s->rxq_descr[i].rxq_stats));
>> }
>>
>> + vmxnet3_validate_interrupts(s);
>> +
>> /* Make sure everything is in place before device activation */
>> smp_wmb();
>>
>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>> }
>> }
>>
>> -#define VMXNET3_MSI_NUM_VECTORS (1)
>> #define VMXNET3_MSI_OFFSET (0x50)
>> #define VMXNET3_USE_64BIT (true)
>> #define VMXNET3_PER_VECTOR_MASK (false)
>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>> PCIDevice *d = PCI_DEVICE(s);
>> int res;
>>
>> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>> if (0 > res) {
>> VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> --
>> MST
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[-- Attachment #2: Type: text/html, Size: 6755 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-04-01 13:04 ` Dmitry Fleytman
@ 2014-04-01 13:07 ` Dr. David Alan Gilbert
2014-04-03 16:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-04-01 13:07 UTC (permalink / raw)
To: Dmitry Fleytman
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel, mdroth,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
* Dmitry Fleytman (dmitry@daynix.com) wrote:
>
> On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> From: Dmitry Fleytman <dmitry@daynix.com>
> >>
> >> CVE-2013-4544
> >>
> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 5be807c..a4b5c11 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -52,6 +52,9 @@
> >> #define VMXNET3_DEVICE_VERSION 0x1
> >> #define VMXNET3_DEVICE_REVISION 0x1
> >>
> >> +/* Number of interrupt vectors for INTx/MSI */
> >> +#define VMXNET3_MAX_NMSIX_INTRS (1)
> >
> > As per Dmitry's reply this is apparently the number of non-MSIX
> > interrupts; can we change the comment to make this clear.
>
> Not sure how to change it.
> There are three modes of operation:
> 1. INTx - 1 interrupt is used
> 2. MSI - 1 interrupt is used
> 3. MSIx - up to 25 interrupts are used.
>
> This define covers 2 first modes of operation.
> Would something like
>
> /* Number of interrupt vectors for non-MSIx modes */
>
> be better?
Yes - that's fine.
Dave
>
>
> >
> >> +
> >> /* Macros for rings descriptors access */
> >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> >> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> >> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> >> }
> >>
> >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> >> +{
> >> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> >> + if (idx >= max_ints) {
> >> + hw_error("Bad interrupt index: %d\n", idx);
> >> + }
> >
> > Can we avoid hw_error here, we're using in this in state load, so I'm
> > OK at the thought of error_report, but then we should make
> > this return a bool to say it's failed and bubble this up so
> > that it just fails the post_load test like any other bad state load.
> >
> > Dave
> >
> >> +}
> >> +
> >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> >> +{
> >> + int i;
> >> +
> >> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> >> +
> >> + for (i = 0; i < s->txq_num; i++) {
> >> + int idx = s->txq_descr[i].intr_idx;
> >> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> + }
> >> +
> >> + for (i = 0; i < s->rxq_num; i++) {
> >> + int idx = s->rxq_descr[i].intr_idx;
> >> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> + }
> >> +}
> >> +
> >> static void vmxnet3_activate_device(VMXNET3State *s)
> >> {
> >> int i;
> >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> >> sizeof(s->rxq_descr[i].rxq_stats));
> >> }
> >>
> >> + vmxnet3_validate_interrupts(s);
> >> +
> >> /* Make sure everything is in place before device activation */
> >> smp_wmb();
> >>
> >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> >> }
> >> }
> >>
> >> -#define VMXNET3_MSI_NUM_VECTORS (1)
> >> #define VMXNET3_MSI_OFFSET (0x50)
> >> #define VMXNET3_USE_64BIT (true)
> >> #define VMXNET3_PER_VECTOR_MASK (false)
> >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> >> PCIDevice *d = PCI_DEVICE(s);
> >> int res;
> >>
> >> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> >> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> >> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> >> if (0 > res) {
> >> VMW_WRPRN("Failed to initialize MSI, error %d", res);
> >> --
> >> MST
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-04-01 13:07 ` Dr. David Alan Gilbert
@ 2014-04-03 16:07 ` Michael S. Tsirkin
2014-04-04 9:47 ` Dmitry Fleytman
0 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-04-03 16:07 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Anthony Liguori, qemu-stable, qemu-devel, Stefan Hajnoczi,
Dmitry Fleytman, Paolo Bonzini, mdroth
On Tue, Apr 01, 2014 at 02:07:52PM +0100, Dr. David Alan Gilbert wrote:
> * Dmitry Fleytman (dmitry@daynix.com) wrote:
> >
> > On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> >
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > >> From: Dmitry Fleytman <dmitry@daynix.com>
> > >>
> > >> CVE-2013-4544
> > >>
> > >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> > >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >> ---
> > >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> > >> 1 file changed, 34 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > >> index 5be807c..a4b5c11 100644
> > >> --- a/hw/net/vmxnet3.c
> > >> +++ b/hw/net/vmxnet3.c
> > >> @@ -52,6 +52,9 @@
> > >> #define VMXNET3_DEVICE_VERSION 0x1
> > >> #define VMXNET3_DEVICE_REVISION 0x1
> > >>
> > >> +/* Number of interrupt vectors for INTx/MSI */
> > >> +#define VMXNET3_MAX_NMSIX_INTRS (1)
> > >
> > > As per Dmitry's reply this is apparently the number of non-MSIX
> > > interrupts; can we change the comment to make this clear.
> >
> > Not sure how to change it.
> > There are three modes of operation:
> > 1. INTx - 1 interrupt is used
> > 2. MSI - 1 interrupt is used
> > 3. MSIx - up to 25 interrupts are used.
> >
> > This define covers 2 first modes of operation.
> > Would something like
> >
> > /* Number of interrupt vectors for non-MSIx modes */
> >
> > be better?
>
> Yes - that's fine.
>
> Dave
OK Dmitry, can you please resend patches with suggested comment changes?
> >
> >
> > >
> > >> +
> > >> /* Macros for rings descriptors access */
> > >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> > >> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> > >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> > >> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> > >> }
> > >>
> > >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> > >> +{
> > >> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> > >> + if (idx >= max_ints) {
> > >> + hw_error("Bad interrupt index: %d\n", idx);
> > >> + }
> > >
> > > Can we avoid hw_error here, we're using in this in state load, so I'm
> > > OK at the thought of error_report, but then we should make
> > > this return a bool to say it's failed and bubble this up so
> > > that it just fails the post_load test like any other bad state load.
> > >
> > > Dave
> > >
> > >> +}
> > >> +
> > >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> > >> +{
> > >> + int i;
> > >> +
> > >> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> > >> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> > >> +
> > >> + for (i = 0; i < s->txq_num; i++) {
> > >> + int idx = s->txq_descr[i].intr_idx;
> > >> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> > >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> > >> + }
> > >> +
> > >> + for (i = 0; i < s->rxq_num; i++) {
> > >> + int idx = s->rxq_descr[i].intr_idx;
> > >> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> > >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> > >> + }
> > >> +}
> > >> +
> > >> static void vmxnet3_activate_device(VMXNET3State *s)
> > >> {
> > >> int i;
> > >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> > >> sizeof(s->rxq_descr[i].rxq_stats));
> > >> }
> > >>
> > >> + vmxnet3_validate_interrupts(s);
> > >> +
> > >> /* Make sure everything is in place before device activation */
> > >> smp_wmb();
> > >>
> > >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> > >> }
> > >> }
> > >>
> > >> -#define VMXNET3_MSI_NUM_VECTORS (1)
> > >> #define VMXNET3_MSI_OFFSET (0x50)
> > >> #define VMXNET3_USE_64BIT (true)
> > >> #define VMXNET3_PER_VECTOR_MASK (false)
> > >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> > >> PCIDevice *d = PCI_DEVICE(s);
> > >> int res;
> > >>
> > >> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> > >> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> > >> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> > >> if (0 > res) {
> > >> VMW_WRPRN("Failed to initialize MSI, error %d", res);
> > >> --
> > >> MST
> > >>
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest
2014-04-03 16:07 ` Michael S. Tsirkin
@ 2014-04-04 9:47 ` Dmitry Fleytman
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-04 9:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Paolo Bonzini, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 4936 bytes --]
On Apr 3, 2014, at 19:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 01, 2014 at 02:07:52PM +0100, Dr. David Alan Gilbert wrote:
>> * Dmitry Fleytman (dmitry@daynix.com) wrote:
>>>
>>> On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>>
>>>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>>>>> From: Dmitry Fleytman <dmitry@daynix.com>
>>>>>
>>>>> CVE-2013-4544
>>>>>
>>>>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>>>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 34 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>>> index 5be807c..a4b5c11 100644
>>>>> --- a/hw/net/vmxnet3.c
>>>>> +++ b/hw/net/vmxnet3.c
>>>>> @@ -52,6 +52,9 @@
>>>>> #define VMXNET3_DEVICE_VERSION 0x1
>>>>> #define VMXNET3_DEVICE_REVISION 0x1
>>>>>
>>>>> +/* Number of interrupt vectors for INTx/MSI */
>>>>> +#define VMXNET3_MAX_NMSIX_INTRS (1)
>>>>
>>>> As per Dmitry's reply this is apparently the number of non-MSIX
>>>> interrupts; can we change the comment to make this clear.
>>>
>>> Not sure how to change it.
>>> There are three modes of operation:
>>> 1. INTx - 1 interrupt is used
>>> 2. MSI - 1 interrupt is used
>>> 3. MSIx - up to 25 interrupts are used.
>>>
>>> This define covers 2 first modes of operation.
>>> Would something like
>>>
>>> /* Number of interrupt vectors for non-MSIx modes */
>>>
>>> be better?
>>
>> Yes - that's fine.
>>
>> Dave
>
>
> OK Dmitry, can you please resend patches with suggested comment changes?
Done. Sent to the list.
>
>>>
>>>
>>>>
>>>>> +
>>>>> /* Macros for rings descriptors access */
>>>>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>>>>> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>>>>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>>>>> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>>>>> }
>>>>>
>>>>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>>>>> +{
>>>>> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>>>>> + if (idx >= max_ints) {
>>>>> + hw_error("Bad interrupt index: %d\n", idx);
>>>>> + }
>>>>
>>>> Can we avoid hw_error here, we're using in this in state load, so I'm
>>>> OK at the thought of error_report, but then we should make
>>>> this return a bool to say it's failed and bubble this up so
>>>> that it just fails the post_load test like any other bad state load.
>>>>
>>>> Dave
>>>>
>>>>> +}
>>>>> +
>>>>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>>>>> +{
>>>>> + int i;
>>>>> +
>>>>> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>>>>> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>>>>> +
>>>>> + for (i = 0; i < s->txq_num; i++) {
>>>>> + int idx = s->txq_descr[i].intr_idx;
>>>>> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>>>>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>>>>> + }
>>>>> +
>>>>> + for (i = 0; i < s->rxq_num; i++) {
>>>>> + int idx = s->rxq_descr[i].intr_idx;
>>>>> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>>>>> + vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static void vmxnet3_activate_device(VMXNET3State *s)
>>>>> {
>>>>> int i;
>>>>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>>>> sizeof(s->rxq_descr[i].rxq_stats));
>>>>> }
>>>>>
>>>>> + vmxnet3_validate_interrupts(s);
>>>>> +
>>>>> /* Make sure everything is in place before device activation */
>>>>> smp_wmb();
>>>>>
>>>>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>>>>> }
>>>>> }
>>>>>
>>>>> -#define VMXNET3_MSI_NUM_VECTORS (1)
>>>>> #define VMXNET3_MSI_OFFSET (0x50)
>>>>> #define VMXNET3_USE_64BIT (true)
>>>>> #define VMXNET3_PER_VECTOR_MASK (false)
>>>>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>>>>> PCIDevice *d = PCI_DEVICE(s);
>>>>> int res;
>>>>>
>>>>> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>>>>> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>>>> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>>>> if (0 > res) {
>>>>> VMW_WRPRN("Failed to initialize MSI, error %d", res);
>>>>> --
>>>>> MST
>>>>>
>>>> --
>>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[-- Attachment #2: Type: text/html, Size: 7267 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (26 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 16:33 ` Dr. David Alan Gilbert
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest Michael S. Tsirkin
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 30/30] vmxnet3: validate queues configuration read on migration Michael S. Tsirkin
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Stefan Hajnoczi, Dmitry Fleytman,
Paolo Bonzini, Anthony Liguori
From: Dmitry Fleytman <dmitry@daynix.com>
CVE-2013-4544
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/vmxnet3.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index a4b5c11..8c6df05 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
}
}
+ vmxnet3_validate_interrupts(s);
+
return 0;
}
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration Michael S. Tsirkin
@ 2014-03-31 16:33 ` Dr. David Alan Gilbert
2014-03-31 19:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-03-31 16:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Dmitry Fleytman, Paolo Bonzini, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
>
> CVE-2013-4544
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/vmxnet3.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index a4b5c11..8c6df05 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
> }
> }
>
> + vmxnet3_validate_interrupts(s);
> +
When you repost to fix the other stuff, why not merge this with what
is currently #30.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration
2014-03-31 16:33 ` Dr. David Alan Gilbert
@ 2014-03-31 19:38 ` Michael S. Tsirkin
2014-04-01 10:15 ` Dmitry Fleytman
0 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 19:38 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Anthony Liguori, qemu-stable, qemu-devel, Stefan Hajnoczi,
Dmitry Fleytman, Paolo Bonzini, mdroth
On Mon, Mar 31, 2014 at 05:33:44PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > From: Dmitry Fleytman <dmitry@daynix.com>
> >
> > CVE-2013-4544
> >
> > Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> > Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/net/vmxnet3.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > index a4b5c11..8c6df05 100644
> > --- a/hw/net/vmxnet3.c
> > +++ b/hw/net/vmxnet3.c
> > @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
> > }
> > }
> >
> > + vmxnet3_validate_interrupts(s);
> > +
>
> When you repost to fix the other stuff, why not merge this with what
> is currently #30.
>
> Dave
I generally try to avoid making random changes to other's patches,
unless absolutely necessary.
Dmitry?
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration
2014-03-31 19:38 ` Michael S. Tsirkin
@ 2014-04-01 10:15 ` Dmitry Fleytman
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-01 10:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Paolo Bonzini, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]
On Mar 31, 2014, at 22:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 31, 2014 at 05:33:44PM +0100, Dr. David Alan Gilbert wrote:
>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>>> From: Dmitry Fleytman <dmitry@daynix.com>
>>>
>>> CVE-2013-4544
>>>
>>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>> hw/net/vmxnet3.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index a4b5c11..8c6df05 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
>>> }
>>> }
>>>
>>> + vmxnet3_validate_interrupts(s);
>>> +
>>
>> When you repost to fix the other stuff, why not merge this with what
>> is currently #30.
>>
>> Dave
>
> I generally try to avoid making random changes to other's patches,
> unless absolutely necessary.
> Dmitry?
Of course I see no problem for these patches to be merged.
My personal preference however is to keep those separated for cuter history.
>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[-- Attachment #2: Type: text/html, Size: 2802 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (27 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
2014-03-31 15:48 ` Dr. David Alan Gilbert
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 30/30] vmxnet3: validate queues configuration read on migration Michael S. Tsirkin
29 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Stefan Hajnoczi, Dmitry Fleytman,
Paolo Bonzini, Anthony Liguori
From: Dmitry Fleytman <dmitry@daynix.com>
CVE-2013-4544
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/vmxnet3.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 8c6df05..0fa54e7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
}
}
+static void vmxnet3_validate_queues(VMXNET3State *s)
+{
+ if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
+ hw_error("Bad TX queues number: %d\n", s->txq_num);
+ }
+
+ if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
+ hw_error("Bad RX queues number: %d\n", s->rxq_num);
+ }
+}
+
static void vmxnet3_activate_device(VMXNET3State *s)
{
int i;
@@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
- assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
+ vmxnet3_validate_queues(s);
qdescr_table_pa =
VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest Michael S. Tsirkin
@ 2014-03-31 15:48 ` Dr. David Alan Gilbert
2014-04-01 10:04 ` Dmitry Fleytman
0 siblings, 1 reply; 81+ messages in thread
From: Dr. David Alan Gilbert @ 2014-03-31 15:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Dmitry Fleytman, Paolo Bonzini, dgilbert
* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
>
> CVE-2013-4544
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/vmxnet3.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8c6df05..0fa54e7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
> }
> }
>
> +static void vmxnet3_validate_queues(VMXNET3State *s)
> +{
> + if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
> + hw_error("Bad TX queues number: %d\n", s->txq_num);
> + }
> +
> + if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
> + hw_error("Bad RX queues number: %d\n", s->rxq_num);
> + }
Why isn't that >= ?
(I agree it matches the original assert).
Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
static void vmxnet3_fill_stats(VMXNET3State *s)
{
int i;
for (i = 0; i < s->txq_num; i++) {
cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
&s->txq_descr[i].txq_stats,
sizeof(s->txq_descr[i].txq_stats));
}
so that looks like it's 0 indexed.
Dave
> +}
> +
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
> int i;
> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>
> VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
> - assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
> + vmxnet3_validate_queues(s);
>
> qdescr_table_pa =
> VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
> --
> MST
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest
2014-03-31 15:48 ` Dr. David Alan Gilbert
@ 2014-04-01 10:04 ` Dmitry Fleytman
2014-04-01 14:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-01 10:04 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Anthony Liguori, Michael S. Tsirkin, qemu-devel, mdroth,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]
On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>>
>> CVE-2013-4544
>>
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8c6df05..0fa54e7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> }
>> }
>>
>> +static void vmxnet3_validate_queues(VMXNET3State *s)
>> +{
>> + if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>> + hw_error("Bad TX queues number: %d\n", s->txq_num);
>> + }
>> +
>> + if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>> + hw_error("Bad RX queues number: %d\n", s->rxq_num);
>> + }
>
> Why isn't that >= ?
> (I agree it matches the original assert).
>
> Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
> Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>
> static void vmxnet3_fill_stats(VMXNET3State *s)
> {
> int i;
> for (i = 0; i < s->txq_num; i++) {
> cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
> &s->txq_descr[i].txq_stats,
> sizeof(s->txq_descr[i].txq_stats));
> }
>
> so that looks like it's 0 indexed.
>
> Dave
HI Dave, thanks for the review.
The verification is ok because s->txq_num and s->rxq_num are total number of queues, not a queue index.
Dmitry.
>
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>> int i;
>> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>>
>> VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>> - assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>> + vmxnet3_validate_queues(s);
>>
>> qdescr_table_pa =
>> VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
>> --
>> MST
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[-- Attachment #2: Type: text/html, Size: 4798 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest
2014-04-01 10:04 ` Dmitry Fleytman
@ 2014-04-01 14:52 ` Michael S. Tsirkin
2014-04-01 18:40 ` Dmitry Fleytman
0 siblings, 1 reply; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-04-01 14:52 UTC (permalink / raw)
To: Dmitry Fleytman
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Paolo Bonzini, Dr. David Alan Gilbert
On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
>
> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
>
>
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>
> From: Dmitry Fleytman <dmitry@daynix.com>
>
> CVE-2013-4544
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/vmxnet3.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8c6df05..0fa54e7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
> (VMXNET3State *s)
> }
> }
>
> +static void vmxnet3_validate_queues(VMXNET3State *s)
> +{
> + if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
> + hw_error("Bad TX queues number: %d\n", s->txq_num);
> + }
> +
> + if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
> + hw_error("Bad RX queues number: %d\n", s->rxq_num);
> + }
>
>
> Why isn't that >= ?
> (I agree it matches the original assert).
>
> Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
> Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>
> static void vmxnet3_fill_stats(VMXNET3State *s)
> {
> int i;
> for (i = 0; i < s->txq_num; i++) {
> cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
> &s->txq_descr[i].txq_stats,
> sizeof(s->txq_descr[i].txq_stats));
> }
>
> so that looks like it's 0 indexed.
>
> Dave
>
>
> HI Dave, thanks for the review.
>
> The verification is ok because s->txq_num and s->rxq_num are total number of
> queues, not a queue index.
>
> Dmitry.
A code comment here might be helpful.
>
>
>
>
> +}
> +
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
> int i;
> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
> *s)
> VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
> devRead.misc.numRxQueues);
>
> VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
> - assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
> + vmxnet3_validate_queues(s);
>
> qdescr_table_pa =
> VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
> devRead.misc.queueDescPA);
> --
> MST
>
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest
2014-04-01 14:52 ` Michael S. Tsirkin
@ 2014-04-01 18:40 ` Dmitry Fleytman
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Fleytman @ 2014-04-01 18:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, mdroth, qemu-stable,
Stefan Hajnoczi, Paolo Bonzini, Dr. David Alan Gilbert
> On Apr 1, 2014, at 5:52 PM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
>>
>> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
>> wrote:
>>
>>
>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>>
>> From: Dmitry Fleytman <dmitry@daynix.com>
>>
>> CVE-2013-4544
>>
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8c6df05..0fa54e7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
>> (VMXNET3State *s)
>> }
>> }
>>
>> +static void vmxnet3_validate_queues(VMXNET3State *s)
>> +{
>> + if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>> + hw_error("Bad TX queues number: %d\n", s->txq_num);
>> + }
>> +
>> + if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>> + hw_error("Bad RX queues number: %d\n", s->rxq_num);
>> + }
>>
>>
>> Why isn't that >= ?
>> (I agree it matches the original assert).
>>
>> Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>> Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>>
>> static void vmxnet3_fill_stats(VMXNET3State *s)
>> {
>> int i;
>> for (i = 0; i < s->txq_num; i++) {
>> cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>> &s->txq_descr[i].txq_stats,
>> sizeof(s->txq_descr[i].txq_stats));
>> }
>>
>> so that looks like it's 0 indexed.
>>
>> Dave
>>
>>
>> HI Dave, thanks for the review.
>>
>> The verification is ok because s->txq_num and s->rxq_num are total number of
>> queues, not a queue index.
>>
>> Dmitry.
>
> A code comment here might be helpful.
No problem. I'll resend these two patches with fixed comments.
>
>>
>>
>>
>>
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>> int i;
>> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
>> *s)
>> VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
>> devRead.misc.numRxQueues);
>>
>> VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>> - assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>> + vmxnet3_validate_queues(s);
>>
>> qdescr_table_pa =
>> VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
>> devRead.misc.queueDescPA);
>> --
>> MST
>>
>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH v4 30/30] vmxnet3: validate queues configuration read on migration
2014-03-31 14:15 [Qemu-devel] [PATCH v4 00/30] qemu state loading issues Michael S. Tsirkin
` (28 preceding siblings ...)
2014-03-31 14:17 ` [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest Michael S. Tsirkin
@ 2014-03-31 14:17 ` Michael S. Tsirkin
29 siblings, 0 replies; 81+ messages in thread
From: Michael S. Tsirkin @ 2014-03-31 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, qemu-stable, dgilbert, Stefan Hajnoczi, Dmitry Fleytman,
Paolo Bonzini, Anthony Liguori
From: Dmitry Fleytman <dmitry@daynix.com>
CVE-2013-4544
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/vmxnet3.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 0fa54e7..d062049 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2385,6 +2385,7 @@ static int vmxnet3_post_load(void *opaque, int version_id)
}
}
+ vmxnet3_validate_queues(s);
vmxnet3_validate_interrupts(s);
return 0;
--
MST
^ permalink raw reply related [flat|nested] 81+ messages in thread