Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

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

* Michael S. Tsirkin (mst@redhat.com) wrote:
> CVE-2013-4532
> 
> s->next_packet is read from wire as an index into s->rx[]. If
> s->next_packet exceeds the length of s->rx[], the buffer can be
> subsequently overrun with arbitrary data from the wire.
> 
> Fix this by failing migration if s->next_packet we read from
> the wire exceeds this.
> 
> Similarly, validate rx_fifo against sizeof(s->rx[].data).
> 
> Finally, constrain rx len to a sensibly small positive
> value, to avoid integer overruns when data model
> later uses this value.
> 
> 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 | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d04e6a4..182fd3e 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;
> +    int i, v;
>  
>      if (version_id != 1)
>          return -EINVAL;
> @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>  
>      }

The loop that's just off the top here is:
    for (i = 0; i < 31; i++) {
        s->rx[i].len = qemu_get_be32(f);
        qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));

    }

Doesn't that 'len' need validating? I assume it's the size of the
packet in the fixed sized buffer? (??)

> -    s->next_packet = qemu_get_be32(f);
> -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> -    s->rx_fifo_len = qemu_get_be32(f);
> +    v = qemu_get_be32(f);
> +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> +        return -EINVAL;
> +    }
> +    s->next_packet = v;
> +    v = qemu_get_be32(f);
> +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
> +        return -EINVAL;
> +    }
> +    s->rx_fifo = s->rx[s->next_packet].data + v;
> +    v = qemu_get_be32(f);
> +    /* Set limit low enough to avoid integer overflow when
> +     * we do math on len later, but high enough to avoid
> +     * truncating any packets.
> +     */
> +    if (v < 0 || v >= 0x100000) {
> +        return -EINVAL;
> +    }
> +    s->rx_fifo_len = v;

I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
the buffer (or I think it might be legal for the sum to point to the byte after the
buffer)?
My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
related to the current packet in-flight; although I've not quite convinced myself
about what is supposed to happen at the end of the packet (which is why
I say rx_fifo might point just at? the end.

Dave

>  
>      return 0;
>  }
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK