diff mbox

[09/23] target-arm/machine.c: fix buffer overflow on invalid state load

Message ID 1386087086-3691-10-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Dec. 3, 2013, 4:28 p.m. UTC
CVE-2013-4531

cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
cpreg_vmstate_array_len will cause a buffer overflow.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 target-arm/machine.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Peter Maydell Dec. 3, 2013, 5:16 p.m. UTC | #1
On 3 December 2013 16:28, 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.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  target-arm/machine.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 74f010f..d46b7e8 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -178,6 +178,10 @@ static int cpu_post_load(void *opaque, int version_id)
>      ARMCPU *cpu = opaque;
>      int i, v;
>
> +    if (cpu->cpreg_vmstate_array_len < 0) {
> +        return -1;
> +    }
> +

I think this is the wrong way to fix this bug. The intent of the
code is that using VMSTATE_INT32_LE() for the array_len
makes the migration vmstate code do a check and reject
array lengths which overflow the array. We should fix this
check rather than attempting to catch the cases where it
didn't work in the post_load hook, not least because by the
time we've reached the post-load hook we'll have already
overrun the buffer...

Given the uses of VMSTATE_INT32_LE I suspect we
could safely redefine its semantics to mean "only OK
if less-than-or-equal to X and non-negative". Or we could
have a new VMSTATE_ macro with those semantics,
for array-length checks.

thanks
-- PMM
diff mbox

Patch

diff --git a/target-arm/machine.c b/target-arm/machine.c
index 74f010f..d46b7e8 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -178,6 +178,10 @@  static int cpu_post_load(void *opaque, int version_id)
     ARMCPU *cpu = opaque;
     int i, v;
 
+    if (cpu->cpreg_vmstate_array_len < 0) {
+        return -1;
+    }
+
     /* Update the values list from the incoming migration data.
      * Anything in the incoming data which we don't know about is
      * a migration failure; anything we know about but the incoming