From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUvMW-0005Wl-RB for qemu-devel@nongnu.org; Tue, 01 Apr 2014 05:52:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUvMP-0003hg-Kf for qemu-devel@nongnu.org; Tue, 01 Apr 2014 05:52:08 -0400 Date: Tue, 1 Apr 2014 10:51:54 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140401095153.GD2411@work-vm> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-16-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-16-git-send-email-mst@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3) 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_frame_len is read from the wire and can later used as an index > into s->tx_fifo[] for memset() when a DATA command is issued by the guest. > > In this case s->tx_frame_len is checked to avoid an overrun, but if the > value is negative a subsequently executed guest can underrun the buffer > with zeros via the memset() call. > > Additionally, tx_frame_len is used to validate that tx_fifo_len > doesn't exceed the fifo bounds - the assumption being that data model > never makes it exceed 2032. > > Fix this by failing migration if the incoming value of s->tx_frame_len > is less than -1 (the emulation code allows from -1 as a special case) > or if it exceeds 2032. > > Reported-by: Michael Roth > Reported-by: Peter Maydell > Signed-off-by: Michael S. Tsirkin > --- > hw/net/stellaris_enet.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index aed00fd..90ff950 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -373,7 +373,11 @@ 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); > + v = qemu_get_be32(f); > + if (v < -1 || s->tx_frame_len > 2032) { > + return -EINVAL; > + } > + s->tx_frame_len = v; I think that's wrong; 'stellaris_enet_write' does the following: 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; 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; So lets say that tx_frame_len is initially 2032 when written; 14 is added to it at this point, and if the CRC flag is set then another 4. Thus it seems a user can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying given the buffer is only 2048 bytes. Dave > v = qemu_get_be32(f); > /* How many bytes does data use in tx fifo. */ > sz = s->tx_frame_len == -1 ? 2 : 4; > -- > MST > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK