Re: [Qemu-devel] [PATCH 10/23] stellaris_enet: avoid buffer overrun on incoming migration

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <address@hidden> wrote:
> From: Michael Roth <address@hidden>
>
> CVE-2013-4532

(Is this device even used in any board which supports migration,
incidentally? Personally I would be surprised if migrating a stellaris
board worked since I don't expect anybody's ever tested it.)

> 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 introducing a constant that defines the length of
> s->rx[], and fail migration if the s->next_packet we read from
> the wire exceeds this.
>
> Signed-off-by: Michael Roth <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/net/stellaris_enet.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 9dd77f7..db12a99 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -61,13 +61,14 @@ typedef struct {
>      uint32_t np;
>      int tx_frame_len;
>      int tx_fifo_len;
> -    uint8_t tx_fifo[2048];
>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
>         We implement a full 31 packet fifo.  */
> +    uint8_t tx_fifo[2048];

...why have you moved this TX fifo buffer below the comment about
the RX fifo?

> +#define SE_RX_BUF_LEN 31
>      struct {
>          uint8_t data[2048];
>          int len;
> -    } rx[31];
> +    } rx[SE_RX_BUF_LEN];
>      uint8_t *rx_fifo;
>      int rx_fifo_len;
>      int next_packet;
> @@ -92,15 +93,15 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, 
> const uint8_t *buf, si
>
>      if ((s->rctl & SE_RCTL_RXEN) == 0)
>          return -1;
> -    if (s->np >= 31) {
> +    if (s->np >= SE_RX_BUF_LEN) {
>          DPRINTF("Packet dropped\n");
>          return -1;
>      }
>
>      DPRINTF("Received packet len=%d\n", size);
>      n = s->next_packet + s->np;
> -    if (n >= 31)
> -        n -= 31;
> +    if (n >= SE_RX_BUF_LEN)
> +        n -= SE_RX_BUF_LEN;

Fixing these hardcoded constants is nice, but coding style
demands braces if you're touching the code.

>      s->np++;
>
>      s->rx[n].len = size + 6;
> @@ -132,7 +133,7 @@ static int stellaris_enet_can_receive(NetClientState *nc)
>      if ((s->rctl & SE_RCTL_RXEN) == 0)
>          return 1;
>
> -    return (s->np < 31);
> +    return (s->np < SE_RX_BUF_LEN);
>  }
>
>  static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
> @@ -168,7 +169,7 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr 
> offset,
>          if (s->rx_fifo_len <= 0) {
>              s->rx_fifo_len = 0;
>              s->next_packet++;
> -            if (s->next_packet >= 31)
> +            if (s->next_packet >= SE_RX_BUF_LEN)
>                  s->next_packet = 0;
>              s->np--;
>              DPRINTF("RX done np=%d\n", s->np);
> @@ -344,7 +345,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, s->tx_frame_len);
>      qemu_put_be32(f, s->tx_fifo_len);
>      qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
> -    for (i = 0; i < 31; i++) {
> +    for (i = 0; i < SE_RX_BUF_LEN; i++) {
>          qemu_put_be32(f, s->rx[i].len);
>          qemu_put_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>
> @@ -375,12 +376,15 @@ static int stellaris_enet_load(QEMUFile *f, void 
> *opaque, int version_id)
>      s->tx_frame_len = qemu_get_be32(f);
>      s->tx_fifo_len = qemu_get_be32(f);

>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
> -    for (i = 0; i < 31; i++) {
> +    for (i = 0; i < SE_RX_BUF_LEN; i++) {
>          s->rx[i].len = qemu_get_be32(f);
>          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));

If you don't constrain the rx[i].len to be something sensible
the device model will subsequently happily read off the end
of the data buffer.

>
>      }
>      s->next_packet = qemu_get_be32(f);
> +    if (s->next_packet >= SE_RX_BUF_LEN) {
> +        return -EINVAL;
> +    }
>      s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);

It seems like a bad idea to let the incoming migration stream
completely determine the value of the rx_fifo pointer, which will
let the guest read arbitrary qemu process memory...

thanks
-- PMM