Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (address@hidden) wrote:
> CVE-2013-4532
> 
> s->tx_frame_len is read from the wire and can later used as an index
> into s->tx_fifo[] for memset() when a DATA command is issued by the guest.
> 
> In this case s->tx_frame_len is checked to avoid an overrun, but if the
> value is negative a subsequently executed guest can underrun the buffer
> with zeros via the memset() call.
> 
> Additionally, tx_frame_len is used to validate that tx_fifo_len
> doesn't exceed the fifo bounds - the assumption being that data model
> never makes it exceed 2032.
> 
> Fix this by failing migration if the incoming value of s->tx_frame_len
> is less than -1 (the emulation code allows from -1 as a special case)
> or if it exceeds 2032.
> 
> Reported-by: Michael Roth <address@hidden>
> Reported-by: Peter Maydell <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/net/stellaris_enet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index aed00fd..90ff950 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void 
> *opaque, int version_id)
>      s->mtxd = qemu_get_be32(f);
>      s->mrxd = qemu_get_be32(f);
>      s->np = qemu_get_be32(f);
> -    s->tx_frame_len = qemu_get_be32(f);
> +    v = qemu_get_be32(f);
> +    if (v < -1 || s->tx_frame_len > 2032) {
> +        return -EINVAL;
> +    }
> +    s->tx_frame_len = v;

I think that's wrong; 'stellaris_enet_write' does the following:

            s->tx_frame_len = value & 0xffff;
            if (s->tx_frame_len > 2032) {
                DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
                s->tx_frame_len = 0;
                s->ris |= SE_INT_TXER;
                stellaris_enet_update(s);
            } else {
                DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
                /* The value written does not include the ethernet header.  */
                s->tx_frame_len += 14;
                if ((s->tctl & SE_TCTL_CRC) == 0)
                    s->tx_frame_len += 4;

So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
at this point, and if the CRC flag is set then another 4.   Thus it seems a user
can set the value in tx_frame_len to 2032+14+4=2050  - which is a bit worrying
given the buffer is only 2048 bytes.

Dave

>      v = qemu_get_be32(f);
>      /* How many bytes does data use in tx fifo. */
>      sz = s->tx_frame_len == -1 ? 2 : 4;
> -- 
> MST
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK