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

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  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