Re: [Qemu-devel] [PATCH v5 18/24] ssd0323: fix buffer overun on invalid state load

[Peter Maydell]

On 3 April 2014 17:52, Michael S. Tsirkin <address@hidden> 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.
>
> Additionally, 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.
>
> For completeness, validate mode as well.
>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/display/ssd0323.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index 971152e..d4b9ea3 100644
> --- a/hw/display/ssd0323.c
> +++ b/hw/display/ssd0323.c
> @@ -312,18 +312,42 @@ 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 ) {

Spurious space before ')' (here and below).

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

though
#define SSD0323_ROWS 128
#define SSD0323_ROWBYTES (SSD0323_ROWS / 2)
#define SSD0323_COLS 64

and using the symbolic constants here and elsewhere
in the device wouldn't be a bad idea at some point.

thanks
-- PMM