* Peter Maydell (peter.mayd...@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.mayd...@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 / dgilb...@redhat.com / Manchester, UK