Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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