From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUvZb-0007Un-6A for qemu-devel@nongnu.org; Tue, 01 Apr 2014 06:05:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUvZT-0000OD-Uu for qemu-devel@nongnu.org; Tue, 01 Apr 2014 06:05:39 -0400 Received: from mail-la0-f45.google.com ([209.85.215.45]:61468) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUvZT-0000Nf-Os for qemu-devel@nongnu.org; Tue, 01 Apr 2014 06:05:31 -0400 Received: by mail-la0-f45.google.com with SMTP id hr17so6710154lab.4 for ; Tue, 01 Apr 2014 03:05:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20140401094332.GC2411@work-vm> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-15-git-send-email-mst@redhat.com> <20140401094332.GC2411@work-vm> From: Peter Maydell Date: Tue, 1 Apr 2014 11:05:09 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 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: "Dr. David Alan Gilbert" Cc: Michael Roth , qemu-stable , QEMU Developers , "Michael S. Tsirkin" On 1 April 2014 10:43, Dr. David Alan Gilbert wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: >> CVE-2013-4532 >> @@ -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. It's not that complicated. The first word of DATA has the frame length in the bottom 16 bits, and the first 16 bits of actual data. All the subsequent words written to DATA are part of the actual data. However, I think this checking code is wrong. We need to check tx_frame_len as well -- otherwise the incoming data can simply set a hugely oversize frame length and then have the guest stuff data into DATA until we overrun the tx_fifo. As with the rx fifo, I think the right approach here is to actually check that the incoming migration data satisfies the invariants: tx_frame_len == -1 OR tx_frame_len >= 0 && tx_frame_len <= ARRAY_SIZE(tx_fifo) && tx_fifo_len >= 0 && tx_fifo_len <= tx_frame_len - 4 (We don't have to have funny ?2:4 logic because the case where we only write 2 bytes is when tx_frame_len is -1, which will also reinitialize tx_fifo_len and write strictly to the start of the fifo.) But note that there seems to be a bug or two in the DATA read logic: our cutoff for tx frame too long is tx_frame_len > 2032, but for the limit case of 2032, if we add 14 for the ethernet header and 4 for explicit CRC then we get 2050, which is slightly larger than the tx_fifo buffer... I think we either need to wind that 2032 value down or enlarge the tx_fifo buffer, or possibly just tweak the logic not to try to actually write the CRC bytes into the FIFO since we promptly ignore them -- need to check the h/w datasheet to find out which. Also a bug I think is that the PADEN handling code is setting tx_fifo_len to 60 for short packets, which will have no effect; I'm pretty sure it should be setting tx_frame_len instead. That's just wrong behaviour rather than an overrun issue though. thanks -- PMM