Message ID | 1386087086-3691-11-git-send-email-mst@redhat.com |
---|---|
State | New |
Headers | show |
On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > 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 <mdroth@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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
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]; +#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; 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)); } 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); s->rx_fifo_len = qemu_get_be32(f);