From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUvEU-0007LC-Dt for qemu-devel@nongnu.org; Tue, 01 Apr 2014 05:43:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUvEP-0008Ma-Id for qemu-devel@nongnu.org; Tue, 01 Apr 2014 05:43:50 -0400 Date: Tue, 1 Apr 2014 10:43:32 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140401094332.GC2411@work-vm> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-15-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1396275242-10810-15-git-send-email-mst@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: mdroth@linux.vnet.ibm.com, Peter Maydell , qemu-stable@nongnu.org, qemu-devel@nongnu.org, dgilbert@redhat.com * Michael S. Tsirkin (mst@redhat.com) wrote: > 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 its execution. > > Fix this by failing migration if the value from the wire would make > guest access the array out of bounds. > > Reported-by: Michael Roth > Reported-by: Peter Maydell > Signed-off-by: Michael S. Tsirkin > --- > hw/net/stellaris_enet.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index 182fd3e..aed00fd 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque) > static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) > { > stellaris_enet_state *s = (stellaris_enet_state *)opaque; > - int i, v; > + int i, v, sz; > > if (version_id != 1) > return -EINVAL; > @@ -374,7 +374,13 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) > 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); > + v = qemu_get_be32(f); > + /* How many bytes does data use in tx fifo. */ > + sz = s->tx_frame_len == -1 ? 2 : 4; > + if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) { > + return -EINVAL; > + } > + s->tx_fifo_len = v; > qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo)); I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' write case is pretty hairy. Dave > for (i = 0; i < 31; i++) { > s->rx[i].len = qemu_get_be32(f); > -- > MST > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK