| Message ID | 1386087086-3691-12-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 > > s->tx_fifo_len is read from the wire and later used as an index into > s->tx_fifo[] when a DATA command is issued by the guest. If > s->tx_fifo_len is greater than the length of s->tx_fifo[], or less > than 0, the buffer can be overrun/underrun by arbitrary data written out > by the guest upon resuming it's execution. "its". thanks -- PMM
On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > CVE-2013-4532 > > s->tx_fifo_len is read from the wire and later used as an index into > s->tx_fifo[] when a DATA command is issued by the guest. If > s->tx_fifo_len is greater than the length of s->tx_fifo[], or less > than 0, the buffer can be overrun/underrun by arbitrary data written out > by the guest upon resuming it's execution. > > Fix this by introducing a constant that defines the length of > s->tx_fifo[] and failing migration if the value from the wire exceeds > this, or is less than 0. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/net/stellaris_enet.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index db12a99..65f0ba8 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -63,10 +63,11 @@ typedef struct { > int tx_fifo_len; > /* 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_FIFO_LEN 2048 > + uint8_t tx_fifo[SE_FIFO_LEN]; > #define SE_RX_BUF_LEN 31 > struct { > - uint8_t data[2048]; > + uint8_t data[SE_FIFO_LEN]; > int len; > } rx[SE_RX_BUF_LEN]; > uint8_t *rx_fifo; > @@ -375,6 +376,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) > s->np = qemu_get_be32(f); > s->tx_frame_len = qemu_get_be32(f); > s->tx_fifo_len = qemu_get_be32(f); > + if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) { > + return -EINVAL; > + } Actually this isn't quite right, is it? tx_fifo_len should only be allowed to be SE_FIFO_LEN-3 .. SE_FIFO_LEN if tx_frame_len is -1. Otherwise the guest can write up to 4 bytes off the end of the array. (Admittedly that's pretty harmless as it's just into the rx fifo at the moment, but still.) thanks -- PMM
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c index db12a99..65f0ba8 100644 --- a/hw/net/stellaris_enet.c +++ b/hw/net/stellaris_enet.c @@ -63,10 +63,11 @@ typedef struct { int tx_fifo_len; /* 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_FIFO_LEN 2048 + uint8_t tx_fifo[SE_FIFO_LEN]; #define SE_RX_BUF_LEN 31 struct { - uint8_t data[2048]; + uint8_t data[SE_FIFO_LEN]; int len; } rx[SE_RX_BUF_LEN]; uint8_t *rx_fifo; @@ -375,6 +376,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) s->np = qemu_get_be32(f); s->tx_frame_len = qemu_get_be32(f); s->tx_fifo_len = qemu_get_be32(f); + if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) { + return -EINVAL; + } qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo)); for (i = 0; i < SE_RX_BUF_LEN; i++) { s->rx[i].len = qemu_get_be32(f);