[v2,7/7] hw/net/stellaris_enet: Convert to vmstate

Convert this device to use vmstate for its save/load, including
providing a post_load function that sanitizes inbound data to
avoid possible buffer overflows if it is malicious.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 147 ++++++++++++++++++++++++++----------------------
 1 file changed, 79 insertions(+), 68 deletions(-)

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Convert this device to use vmstate for its save/load, including
> providing a post_load function that sanitizes inbound data to
> avoid possible buffer overflows if it is malicious.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/stellaris_enet.c | 147 ++++++++++++++++++++++++++----------------------
>  1 file changed, 79 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index af1c3ef..b8cbf82 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -47,6 +47,11 @@ do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0)
>      OBJECT_CHECK(stellaris_enet_state, (obj), TYPE_STELLARIS_ENET)
>  
>  typedef struct {
> +    uint8_t data[2048];
> +    int32_t len;
> +} StellarisEnetRxFrame;
> +
> +typedef struct {
>      SysBusDevice parent_obj;
>  
>      uint32_t ris;
> @@ -59,22 +64,88 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_fifo_len;
> +    int32_t 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.  */
> -    struct {
> -        uint8_t data[2048];
> -        int len;
> -    } rx[31];
> -    int rx_fifo_offset;
> -    int next_packet;
> +    StellarisEnetRxFrame rx[31];
> +    int32_t rx_fifo_offset;
> +    int32_t next_packet;

I think if I were changing these I'd make them uint's for safety,
IMHO it's better than having to put explicit < 0 checks in the load code.
(assuming there is nowhere that they're designed to go -ve)

>      NICState *nic;
>      NICConf conf;
>      qemu_irq irq;
>      MemoryRegion mmio;
>  } stellaris_enet_state;
>  
> +static const VMStateDescription vmstate_rx_frame = {
> +    .name = "stellaris_enet/rx_frame",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
> +        VMSTATE_INT32(len, StellarisEnetRxFrame),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int stellaris_enet_post_load(void *opaque, int version_id)
> +{
> +    stellaris_enet_state *s = opaque;
> +    int i;
> +
> +    /* Sanitize inbound state. Note that next_packet is an index but
> +     * np is a size; hence their valid upper bounds differ.
> +     */
> +    if (s->next_packet < 0 || s->next_packet >= ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    if (s->np > ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
> +        if (s->rx[i].len < 0 || s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
> +            return -1;
> +        }
> +    }
> +
> +    if (s->rx_fifo_offset < 0 ||
> +        s->rx_fifo_offset + 4 > ARRAY_SIZE(s->rx[0].data)) {
> +        return -1;
> +    }

You're not checking the tx loaded values, I think most of the code
is safe, but, in your patch 3 you have:

> +        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {

which I think could be made to overflow; so should probably
either check tx_fifo_len here, or fix that, or both.


> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stellaris_enet = {
> +    .name = "stellaris_enet",
> +    .version_id = 4,
> +    .minimum_version_id = 4,
> +    .minimum_version_id_old = 4,
> +    .post_load = stellaris_enet_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(ris, stellaris_enet_state),
> +        VMSTATE_UINT32(im, stellaris_enet_state),
> +        VMSTATE_UINT32(rctl, stellaris_enet_state),
> +        VMSTATE_UINT32(tctl, stellaris_enet_state),
> +        VMSTATE_UINT32(thr, stellaris_enet_state),
> +        VMSTATE_UINT32(mctl, stellaris_enet_state),
> +        VMSTATE_UINT32(mdv, stellaris_enet_state),
> +        VMSTATE_UINT32(mtxd, stellaris_enet_state),
> +        VMSTATE_UINT32(mrxd, stellaris_enet_state),
> +        VMSTATE_UINT32(np, stellaris_enet_state),
> +        VMSTATE_INT32(tx_fifo_len, stellaris_enet_state),
> +        VMSTATE_UINT8_ARRAY(tx_fifo, stellaris_enet_state, 2048),
> +        VMSTATE_STRUCT_ARRAY(rx, stellaris_enet_state, 31, 1,
> +                             vmstate_rx_frame, StellarisEnetRxFrame),
> +        VMSTATE_INT32(next_packet, stellaris_enet_state),
> +        VMSTATE_INT32(rx_fifo_offset, stellaris_enet_state),

The next_packet/rx_fifo_offset are in the opposite order from your structure
after the previous patches.

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void stellaris_enet_update(stellaris_enet_state *s)
>  {
>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> @@ -379,63 +450,6 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
>      s->tx_fifo_len = 0;
>  }
>  
> -static void stellaris_enet_save(QEMUFile *f, void *opaque)
> -{

Good :-)


Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Tue, Apr 01, 2014 at 11:14:55PM +0100, Peter Maydell wrote:
> Convert this device to use vmstate for its save/load, including
> providing a post_load function that sanitizes inbound data to
> avoid possible buffer overflows if it is malicious.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Might be a good idea to mention that this fixes CVE-2013-4532 in the commit log.


> ---
>  hw/net/stellaris_enet.c | 147 ++++++++++++++++++++++++++----------------------
>  1 file changed, 79 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index af1c3ef..b8cbf82 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -47,6 +47,11 @@ do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0)
>      OBJECT_CHECK(stellaris_enet_state, (obj), TYPE_STELLARIS_ENET)
>  
>  typedef struct {
> +    uint8_t data[2048];
> +    int32_t len;
> +} StellarisEnetRxFrame;
> +
> +typedef struct {
>      SysBusDevice parent_obj;
>  
>      uint32_t ris;
> @@ -59,22 +64,88 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_fifo_len;
> +    int32_t 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.  */
> -    struct {
> -        uint8_t data[2048];
> -        int len;
> -    } rx[31];
> -    int rx_fifo_offset;
> -    int next_packet;
> +    StellarisEnetRxFrame rx[31];
> +    int32_t rx_fifo_offset;
> +    int32_t next_packet;
>      NICState *nic;
>      NICConf conf;
>      qemu_irq irq;
>      MemoryRegion mmio;
>  } stellaris_enet_state;
>  
> +static const VMStateDescription vmstate_rx_frame = {
> +    .name = "stellaris_enet/rx_frame",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
> +        VMSTATE_INT32(len, StellarisEnetRxFrame),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int stellaris_enet_post_load(void *opaque, int version_id)
> +{
> +    stellaris_enet_state *s = opaque;
> +    int i;
> +
> +    /* Sanitize inbound state. Note that next_packet is an index but
> +     * np is a size; hence their valid upper bounds differ.
> +     */
> +    if (s->next_packet < 0 || s->next_packet >= ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    if (s->np > ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
> +        if (s->rx[i].len < 0 || s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
> +            return -1;
> +        }
> +    }
> +
> +    if (s->rx_fifo_offset < 0 ||
> +        s->rx_fifo_offset + 4 > ARRAY_SIZE(s->rx[0].data)) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stellaris_enet = {
> +    .name = "stellaris_enet",
> +    .version_id = 4,
> +    .minimum_version_id = 4,
> +    .minimum_version_id_old = 4,
> +    .post_load = stellaris_enet_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(ris, stellaris_enet_state),
> +        VMSTATE_UINT32(im, stellaris_enet_state),
> +        VMSTATE_UINT32(rctl, stellaris_enet_state),
> +        VMSTATE_UINT32(tctl, stellaris_enet_state),
> +        VMSTATE_UINT32(thr, stellaris_enet_state),
> +        VMSTATE_UINT32(mctl, stellaris_enet_state),
> +        VMSTATE_UINT32(mdv, stellaris_enet_state),
> +        VMSTATE_UINT32(mtxd, stellaris_enet_state),
> +        VMSTATE_UINT32(mrxd, stellaris_enet_state),
> +        VMSTATE_UINT32(np, stellaris_enet_state),
> +        VMSTATE_INT32(tx_fifo_len, stellaris_enet_state),
> +        VMSTATE_UINT8_ARRAY(tx_fifo, stellaris_enet_state, 2048),
> +        VMSTATE_STRUCT_ARRAY(rx, stellaris_enet_state, 31, 1,
> +                             vmstate_rx_frame, StellarisEnetRxFrame),
> +        VMSTATE_INT32(next_packet, stellaris_enet_state),
> +        VMSTATE_INT32(rx_fifo_offset, stellaris_enet_state),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void stellaris_enet_update(stellaris_enet_state *s)
>  {
>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> @@ -379,63 +450,6 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
>      s->tx_fifo_len = 0;
>  }
>  
> -static void stellaris_enet_save(QEMUFile *f, void *opaque)
> -{
> -    stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> -    int i;
> -
> -    qemu_put_be32(f, s->ris);
> -    qemu_put_be32(f, s->im);
> -    qemu_put_be32(f, s->rctl);
> -    qemu_put_be32(f, s->tctl);
> -    qemu_put_be32(f, s->thr);
> -    qemu_put_be32(f, s->mctl);
> -    qemu_put_be32(f, s->mdv);
> -    qemu_put_be32(f, s->mtxd);
> -    qemu_put_be32(f, s->mrxd);
> -    qemu_put_be32(f, s->np);
> -    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++) {
> -        qemu_put_be32(f, s->rx[i].len);
> -        qemu_put_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> -
> -    }
> -    qemu_put_be32(f, s->next_packet);
> -    qemu_put_be32(f, s->rx_fifo_offset);
> -}
> -
> -static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> -    int i;
> -
> -    if (version_id != 1)
> -        return -EINVAL;
> -
> -    s->ris = qemu_get_be32(f);
> -    s->im = qemu_get_be32(f);
> -    s->rctl = qemu_get_be32(f);
> -    s->tctl = qemu_get_be32(f);
> -    s->thr = qemu_get_be32(f);
> -    s->mctl = qemu_get_be32(f);
> -    s->mdv = qemu_get_be32(f);
> -    s->mtxd = qemu_get_be32(f);
> -    s->mrxd = qemu_get_be32(f);
> -    s->np = 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++) {
> -        s->rx[i].len = qemu_get_be32(f);
> -        qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> -
> -    }
> -    s->next_packet = qemu_get_be32(f);
> -    s->rx_fifo_offset = qemu_get_be32(f);
> -
> -    return 0;
> -}
> -
>  static void stellaris_enet_cleanup(NetClientState *nc)
>  {
>      stellaris_enet_state *s = qemu_get_nic_opaque(nc);
> @@ -467,8 +481,6 @@ static int stellaris_enet_init(SysBusDevice *sbd)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>  
>      stellaris_enet_reset(s);
> -    register_savevm(dev, "stellaris_enet", -1, 3,
> -                    stellaris_enet_save, stellaris_enet_load, s);
>      return 0;
>  }
>  
> @@ -476,8 +488,6 @@ static void stellaris_enet_unrealize(DeviceState *dev, Error **errp)
>  {
>      stellaris_enet_state *s = STELLARIS_ENET(dev);
>  
> -    unregister_savevm(DEVICE(s), "stellaris_enet", s);
> -
>      memory_region_destroy(&s->mmio);
>  }
>  
> @@ -494,6 +504,7 @@ static void stellaris_enet_class_init(ObjectClass *klass, void *data)
>      k->init = stellaris_enet_init;
>      dc->unrealize = stellaris_enet_unrealize;
>      dc->props = stellaris_enet_properties;
> +    dc->vmsd = &vmstate_stellaris_enet;
>  }
>  
>  static const TypeInfo stellaris_enet_info = {
> -- 
> 1.9.0
>