Re: [Qemu-devel] [PATCH v6 1/5] ssi-sd: fix buffer overrun on invalid state load

[Peter Crosthwaite]

On Mon, Apr 28, 2014 at 11:08 PM, Michael S. Tsirkin <address@hidden> wrote:
> CVE-2013-4537
>
> s->arglen is taken from wire and used as idx
> in ssi_sd_transfer().
>
> Validate it before access.
>

So I'm wondering what the policy here is on validation. Do you only
need to catch the cases that can cause fatal bugs post migration, or
should you try and catch all forms of bad VMSD whether they they are
recoverable or not? E.G. by changing a few conditionals here you can
catch a few broken-on-save VMSDs that are recoverable but at the same
time could in no way have been generated by a valid save routine ...

> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/sd/ssi-sd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 3273c8a..b012e57 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -230,8 +230,17 @@ 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 < 0 || s->arglen >= ARRAY_SIZE(s->cmdarg))) {
> +        return -EINVAL;
> +    }
>      s->response_pos = qemu_get_be32(f);
>      s->stopping = qemu_get_be32(f);
> +    if (s->mode == SSI_SD_RESPONSE &&
> +        (s->response_pos < 0 || s->response_pos >= ARRAY_SIZE(s->response) ||
> +        (!s->stopping && s->arglen > ARRAY_SIZE(s->response)))) {

Having the stopping bit set doesn't excuse and OOB arglen really so
you could drop the !s->stopping.

You can also change the conditional to s->arglen > s->response_pos to
catch the less fatal but still erroneous case where arglen is <=5 but
still OOB of the requested response length.

Patch is good though,

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> +        return -EINVAL;
> +    }
>
>      ss->cs = qemu_get_be32(f);
>
> --
> MST
>
>