[Qemu-devel] [PATCH v4 0/7] stellaris_enet: overhaul tx/rx, convert to vmstate

[Qemu-devel] [PATCH v4 0/7] stellaris_enet: overhaul tx/rx, convert to vmstate

[Peter Maydell]

This patchset overhauls the stellaris_enet TX and RX handling code,
and converts it to use vmstate.

The principal motivation is to fix the buffer overrun noted
in the first patch, and to reimplement things using simpler
state fields which are easier to migrate and to validate in
post_load. I also fixed a couple of other bugs I noticed while
I was there.

This isn't actually sufficient to get my test image to work:
that needs proper implementation of the MII registers in the PHY.
I tested this with a minor hack to make all MII registers return
0x24, which happens to satisfy the test image's setup code.
However implementing the PHY registers is more work than I want
to do on this device right now...

Although patch 7 fixes a CVE (code is exploitable by malicious
incoming migration state), I haven't cc'd stable on it;
this is because:
 * migration doesn't work on the stellaris board anyway
   because not all the board's devices support it, so nobody
   will be using it
 * there are many lurking problems with most of our more-or-less
   unmaintained boards, so nobody IMHO should be treating
   guest-to-host as a security boundary for those boards

I think patch 7 is the only one still needing review.

thanks
-- PMM

Changes v3->v4:
 * fix +/-4 error in guard on rx_fifo_offset
 * drop now-unneeded minimum_version_id_old entries in vmstate

Changes v2->v3:
 * don't bother updating vmstate version in intermediate patches
 * use uint32_t rather than int32_t for rx_fifo_offset, next_packet,
   tx_fifo_len and rx[].len 
 * check tx_fifo_len range in post-load
 * tweak rx_fifo_offset post-load check to avoid possible overflow
   in addition
 * fixed ordering of next_packet/rx_fifo_offset in vmstate to
   match the stellaris_enet_state structure

Changes v1->v2:
 * only transmit when 1 is written to TR, not on any write
 * new patches to get rid of rx_fifo, rx_fifo_len
 * vmstate conversion (includes migration sanitizing code)


Peter Maydell (7):
  hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer
    overrun
  hw/net/stellaris_enet: Correct handling of packet padding
  hw/net/stellaris_enet: Rewrite tx fifo handling code
  hw/net/stellaris_enet: Correctly implement the TR and THR registers
  hw/net/stellaris_enet: Fix debug format strings
  hw/net/stellaris_enet: Get rid of rx_fifo pointer
  hw/net/stellaris_enet: Convert to vmstate

 hw/net/stellaris_enet.c | 312 +++++++++++++++++++++++++++++-------------------
 1 file changed, 186 insertions(+), 126 deletions(-)

-- 
1.9.2

[Qemu-devel] [PATCH v4 1/7] hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer overrun

[Peter Maydell]

The current tx_fifo code has a corner case where the guest can overrun
the fifo buffer: if automatic CRCs are disabled we allow the guest to write
the CRC word even if there isn't actually space for it in the FIFO.
The datasheet is unclear about exactly how the hardware deals with this
situation; the most plausible answer seems to be that the CRC word is
just lost.

Implement this fix by separating the "can we stuff another word in the
FIFO" logic from the "should we transmit the packet now" check. This
also moves us closer to the real hardware, which has a number of ways
it can be configured to trigger sending the packet, some of which we
don't implement.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/net/stellaris_enet.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d04e6a4..bd844cd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -253,10 +253,12 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                 s->tx_fifo[s->tx_fifo_len++] = value >> 24;
             }
         } else {
-            s->tx_fifo[s->tx_fifo_len++] = value;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+                s->tx_fifo[s->tx_fifo_len++] = value;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            }
             if (s->tx_fifo_len >= s->tx_frame_len) {
                 /* We don't implement explicit CRC, so just chop it off.  */
                 if ((s->tctl & SE_TCTL_CRC) == 0)
-- 
1.9.2

[Qemu-devel] [PATCH v4 2/7] hw/net/stellaris_enet: Correct handling of packet padding

[Peter Maydell]

The PADEN bit in the transmit control register enables padding of short
data packets out to the required minimum length. However a typo here
meant we were adjusting tx_fifo_len rather than tx_frame_len, so the
padding didn't actually happen. Fix this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/net/stellaris_enet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index bd844cd..d0da819 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -265,7 +265,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                     s->tx_frame_len -= 4;
                 if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
                     memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_fifo_len = 60;
+                    s->tx_frame_len = 60;
                 }
                 qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
                                  s->tx_frame_len);
-- 
1.9.2

[Qemu-devel] [PATCH v4 3/7] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Peter Maydell]

The datasheet is clear that the frame length written to the DATA
register is actually stored in the TX FIFO; this means we don't
need to keep both tx_frame_len and tx_fifo_len state separately.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 42 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d0da819..b99f93e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -59,7 +59,6 @@ typedef struct {
     uint32_t mtxd;
     uint32_t mrxd;
     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.
@@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
     qemu_set_irq(s->irq, (s->ris & s->im) != 0);
 }
 
+/* Return the data length of the packet currently being assembled
+ * in the TX fifo.
+ */
+static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
+{
+    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
+}
+
+/* Return true if the packet currently in the TX FIFO is complete,
+* ie the FIFO holds enough bytes for the data length, ethernet header,
+* payload and optionally CRC.
+*/
+static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+    framelen += 16;
+    if (!(s->tctl & SE_TCTL_CRC)) {
+        framelen += 4;
+    }
+    /* Cover the corner case of a 2032 byte payload with auto-CRC disabled:
+     * this requires more bytes than will fit in the FIFO. It's not totally
+     * clear how the h/w handles this, but if using threshold-based TX
+     * it will definitely try to transmit something.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo));
+    return s->tx_fifo_len >= framelen;
+}
+
+/* Send the packet currently in the TX FIFO */
+static void stellaris_enet_send(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+
+    /* Ethernet header is in the FIFO but not in the datacount.
+     * We don't implement explicit CRC, so just ignore any
+     * CRC value in the FIFO.
+     */
+    framelen += 14;
+    if ((s->tctl & SE_TCTL_PADEN) && framelen < 60) {
+        memset(&s->tx_fifo[framelen + 2], 0, 60 - framelen);
+        framelen = 60;
+    }
+    /* This MIN will have no effect unless the FIFO data is corrupt
+     * (eg bad data from an incoming migration); otherwise the check
+     * on the datalen at the start of writing the data into the FIFO
+     * will have caught this. Silently write a corrupt half-packet,
+     * which is what the hardware does in FIFO underrun situations.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo) - 2);
+    qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo + 2, framelen);
+    s->tx_fifo_len = 0;
+    s->ris |= SE_INT_TXEMP;
+    stellaris_enet_update(s);
+    DPRINTF("Done TX\n");
+}
+
 /* TODO: Implement MAC address filtering.  */
 static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
@@ -215,8 +270,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
-        if (value & SE_INT_TXER)
-            s->tx_frame_len = -1;
+        if (value & SE_INT_TXER) {
+            s->tx_fifo_len = 0;
+        }
         break;
     case 0x04: /* IM */
         DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
@@ -235,46 +291,27 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         s->tctl = value;
         break;
     case 0x10: /* DATA */
-        if (s->tx_frame_len == -1) {
-            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;
+        if (s->tx_fifo_len == 0) {
+            /* The first word is special, it contains the data length */
+            int framelen = value & 0xffff;
+            if (framelen > 2032) {
+                DPRINTF("TX frame too long (%d)\n", framelen);
                 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;
-                s->tx_fifo_len = 0;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-        } else {
-            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
-                s->tx_fifo[s->tx_fifo_len++] = value;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-            if (s->tx_fifo_len >= s->tx_frame_len) {
-                /* We don't implement explicit CRC, so just chop it off.  */
-                if ((s->tctl & SE_TCTL_CRC) == 0)
-                    s->tx_frame_len -= 4;
-                if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
-                    memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_frame_len = 60;
-                }
-                qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
-                                 s->tx_frame_len);
-                s->tx_frame_len = -1;
-                s->ris |= SE_INT_TXEMP;
-                stellaris_enet_update(s);
-                DPRINTF("Done TX\n");
+                break;
             }
         }
+
+        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+            s->tx_fifo[s->tx_fifo_len++] = value;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+        }
+
+        if (stellaris_txpacket_complete(s)) {
+            stellaris_enet_send(s);
+        }
         break;
     case 0x14: /* IA0 */
         s->conf.macaddr.a[0] = value;
@@ -326,7 +363,7 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
     s->im = SE_INT_PHY | SE_INT_MD | SE_INT_RXER | SE_INT_FOV | SE_INT_TXEMP
             | SE_INT_TXER | SE_INT_RX;
     s->thr = 0x3f;
-    s->tx_frame_len = -1;
+    s->tx_fifo_len = 0;
 }
 
 static void stellaris_enet_save(QEMUFile *f, void *opaque)
@@ -344,7 +381,6 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->mtxd);
     qemu_put_be32(f, s->mrxd);
     qemu_put_be32(f, s->np);
-    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++) {
@@ -375,7 +411,6 @@ 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);
     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++) {
-- 
1.9.2

[Qemu-devel] [PATCH v4 4/7] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Peter Maydell]

Packet transmission for the stellaris ethernet controller can be triggered
in one of two ways:
 * by setting a threshold value in the THR register; when the FIFO
   fill level reaches the threshold, the h/w starts transmitting.
   Software has to finish filling the FIFO before the transmit
   process completes to avoid a (silent) underrun
 * by software writing to the TR register to explicitly trigger
   transmission

Since QEMU transmits packets instantaneously (from the guest's
point of view), implement "transmit based on threshold" with
our existing mechanism of "transmit as soon as we have the whole
packet", with the additional wrinkle that we don't transmit if
the packet size is below the specified threshold, and implement
"transmit by specific request" properly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index b99f93e..2079fc6 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -109,6 +109,15 @@ static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
     return s->tx_fifo_len >= framelen;
 }
 
+/* Return true if the TX FIFO threshold is enabled and the FIFO
+ * has filled enough to reach it.
+ */
+static inline bool stellaris_tx_thr_reached(stellaris_enet_state *s)
+{
+    return (s->thr < 0x3f &&
+            (s->tx_fifo_len >= 4 * (s->thr * 8 + 1)));
+}
+
 /* Send the packet currently in the TX FIFO */
 static void stellaris_enet_send(stellaris_enet_state *s)
 {
@@ -309,7 +318,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
             s->tx_fifo[s->tx_fifo_len++] = value >> 24;
         }
 
-        if (stellaris_txpacket_complete(s)) {
+        if (stellaris_tx_thr_reached(s) && stellaris_txpacket_complete(s)) {
             stellaris_enet_send(s);
         }
         break;
@@ -338,9 +347,13 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     case 0x2c: /* MTXD */
         s->mtxd = value & 0xff;
         break;
+    case 0x38: /* TR */
+        if (value & 1) {
+            stellaris_enet_send(s);
+        }
+        break;
     case 0x30: /* MRXD */
     case 0x34: /* NP */
-    case 0x38: /* TR */
         /* Ignored.  */
     case 0x3c: /* Undocuented: Timestamp? */
         /* Ignored.  */
-- 
1.9.2

[Qemu-devel] [PATCH v4 5/7] hw/net/stellaris_enet: Fix debug format strings

[Peter Maydell]

Fix various debug format strings which were incorrect for the
data type, so that building with debug enabled is possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 2079fc6..1cb95e0 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -161,7 +161,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
         return -1;
     }
 
-    DPRINTF("Received packet len=%d\n", size);
+    DPRINTF("Received packet len=%zu\n", size);
     n = s->next_packet + s->np;
     if (n >= 31)
         n -= 31;
@@ -276,7 +276,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     switch (offset) {
     case 0x00: /* IACK */
         s->ris &= ~value;
-        DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ ack %02" PRIx64 "/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
         if (value & SE_INT_TXER) {
@@ -284,7 +284,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         }
         break;
     case 0x04: /* IM */
-        DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ mask %02" PRIx64 "/%02x\n", value, s->ris);
         s->im = value;
         stellaris_enet_update(s);
         break;
-- 
1.9.2

[Qemu-devel] [PATCH v4 6/7] hw/net/stellaris_enet: Get rid of rx_fifo pointer

[Peter Maydell]

The rx_fifo pointer is awkward to migrate, and is actually
redundant since it is always possible to determine it from
the current rx[].len/.data and rx_fifo_len. Remove both
rx_fifo and rx_fifo_len from the state, replacing them with
a simple rx_fifo_offset which points at the current location
in the RX fifo.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 1cb95e0..9e8f143 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -67,8 +67,7 @@ typedef struct {
         uint8_t data[2048];
         int len;
     } rx[31];
-    uint8_t *rx_fifo;
-    int rx_fifo_len;
+    int rx_fifo_offset;
     int next_packet;
     NICState *nic;
     NICConf conf;
@@ -216,21 +215,21 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
     case 0x0c: /* TCTL */
         return s->tctl;
     case 0x10: /* DATA */
-        if (s->rx_fifo_len == 0) {
-            if (s->np == 0) {
-                BADF("RX underflow\n");
-                return 0;
-            }
-            s->rx_fifo_len = s->rx[s->next_packet].len;
-            s->rx_fifo = s->rx[s->next_packet].data;
-            DPRINTF("RX FIFO start packet len=%d\n", s->rx_fifo_len);
+    {
+        uint8_t *rx_fifo;
+
+        if (s->np == 0) {
+            BADF("RX underflow\n");
+            return 0;
         }
-        val = s->rx_fifo[0] | (s->rx_fifo[1] << 8) | (s->rx_fifo[2] << 16)
-              | (s->rx_fifo[3] << 24);
-        s->rx_fifo += 4;
-        s->rx_fifo_len -= 4;
-        if (s->rx_fifo_len <= 0) {
-            s->rx_fifo_len = 0;
+
+        rx_fifo = s->rx[s->next_packet].data + s->rx_fifo_offset;
+
+        val = rx_fifo[0] | (rx_fifo[1] << 8) | (rx_fifo[2] << 16)
+              | (rx_fifo[3] << 24);
+        s->rx_fifo_offset += 4;
+        if (s->rx_fifo_offset >= s->rx[s->next_packet].len) {
+            s->rx_fifo_offset = 0;
             s->next_packet++;
             if (s->next_packet >= 31)
                 s->next_packet = 0;
@@ -238,6 +237,7 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
             DPRINTF("RX done np=%d\n", s->np);
         }
         return val;
+    }
     case 0x14: /* IA0 */
         return s->conf.macaddr.a[0] | (s->conf.macaddr.a[1] << 8)
             | (s->conf.macaddr.a[2] << 16)
@@ -291,8 +291,8 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     case 0x08: /* RCTL */
         s->rctl = value;
         if (value & SE_RCTL_RSTFIFO) {
-            s->rx_fifo_len = 0;
             s->np = 0;
+            s->rx_fifo_offset = 0;
             stellaris_enet_update(s);
         }
         break;
@@ -402,8 +402,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
 
     }
     qemu_put_be32(f, s->next_packet);
-    qemu_put_be32(f, s->rx_fifo - s->rx[s->next_packet].data);
-    qemu_put_be32(f, s->rx_fifo_len);
+    qemu_put_be32(f, s->rx_fifo_offset);
 }
 
 static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
@@ -432,8 +431,7 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
 
     }
     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);
+    s->rx_fifo_offset = qemu_get_be32(f);
 
     return 0;
 }
-- 
1.9.2

[Qemu-devel] [PATCH v4 7/7] hw/net/stellaris_enet: Convert to vmstate

[Peter Maydell]

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.

The sanitizing fixes CVE-2013-4532 (though nobody should be
relying on the security properties of most of the unmaintained
ARM board models anyway, and migration doesn't actually
work on this board due to issues in other device models).

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

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 9e8f143..c9ee5d3 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];
+    uint32_t len;
+} StellarisEnetRxFrame;
+
+typedef struct {
     SysBusDevice parent_obj;
 
     uint32_t ris;
@@ -59,22 +64,89 @@ typedef struct {
     uint32_t mtxd;
     uint32_t mrxd;
     uint32_t np;
-    int tx_fifo_len;
+    uint32_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];
+    uint32_t rx_fifo_offset;
+    uint32_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,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
+        VMSTATE_UINT32(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 >= 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 > ARRAY_SIZE(s->rx[i].data)) {
+            return -1;
+        }
+    }
+
+    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) - 4) {
+        return -1;
+    }
+
+    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
+        return -1;
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_stellaris_enet = {
+    .name = "stellaris_enet",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .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_UINT32(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_UINT32(rx_fifo_offset, stellaris_enet_state),
+        VMSTATE_UINT32(next_packet, 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 +451,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 +482,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, 1,
-                    stellaris_enet_save, stellaris_enet_load, s);
     return 0;
 }
 
@@ -476,8 +489,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 +505,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.2

Re: [Qemu-devel] [PATCH v4 7/7] hw/net/stellaris_enet: Convert to vmstate

[Dr. David Alan Gilbert]

* 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.
> 
> The sanitizing fixes CVE-2013-4532 (though nobody should be
> relying on the security properties of most of the unmaintained
> ARM board models anyway, and migration doesn't actually
> work on this board due to issues in other device models).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/net/stellaris_enet.c | 148 ++++++++++++++++++++++++++----------------------
>  1 file changed, 80 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 9e8f143..c9ee5d3 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];
> +    uint32_t len;
> +} StellarisEnetRxFrame;
> +
> +typedef struct {
>      SysBusDevice parent_obj;
>  
>      uint32_t ris;
> @@ -59,22 +64,89 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_fifo_len;
> +    uint32_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];
> +    uint32_t rx_fifo_offset;
> +    uint32_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,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
> +        VMSTATE_UINT32(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 >= 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 > ARRAY_SIZE(s->rx[i].data)) {
> +            return -1;
> +        }
> +    }
> +
> +    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) - 4) {
> +        return -1;
> +    }
> +
> +    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stellaris_enet = {
> +    .name = "stellaris_enet",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .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_UINT32(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_UINT32(rx_fifo_offset, stellaris_enet_state),
> +        VMSTATE_UINT32(next_packet, 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 +451,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 +482,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, 1,
> -                    stellaris_enet_save, stellaris_enet_load, s);
>      return 0;
>  }
>  
> @@ -476,8 +489,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 +505,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.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 7/7] hw/net/stellaris_enet: Convert to vmstate

[Michael S. Tsirkin]

On Thu, May 08, 2014 at 12:53:07PM +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.
> 
> The sanitizing fixes CVE-2013-4532 (though nobody should be
> relying on the security properties of most of the unmaintained
> ARM board models anyway, and migration doesn't actually
> work on this board due to issues in other device models).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/net/stellaris_enet.c | 148 ++++++++++++++++++++++++++----------------------
>  1 file changed, 80 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 9e8f143..c9ee5d3 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];
> +    uint32_t len;
> +} StellarisEnetRxFrame;
> +
> +typedef struct {
>      SysBusDevice parent_obj;
>  
>      uint32_t ris;
> @@ -59,22 +64,89 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_fifo_len;
> +    uint32_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];
> +    uint32_t rx_fifo_offset;
> +    uint32_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,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
> +        VMSTATE_UINT32(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 >= 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 > ARRAY_SIZE(s->rx[i].data)) {
> +            return -1;
> +        }
> +    }
> +
> +    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) - 4) {
> +        return -1;
> +    }
> +
> +    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stellaris_enet = {
> +    .name = "stellaris_enet",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .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_UINT32(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_UINT32(rx_fifo_offset, stellaris_enet_state),
> +        VMSTATE_UINT32(next_packet, 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 +451,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 +482,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, 1,
> -                    stellaris_enet_save, stellaris_enet_load, s);
>      return 0;
>  }
>  
> @@ -476,8 +489,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 +505,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.2