From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WV0TK-0004WK-Pg for qemu-devel@nongnu.org; Tue, 01 Apr 2014 11:19:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WV0TF-0003VF-QI for qemu-devel@nongnu.org; Tue, 01 Apr 2014 11:19:30 -0400 Date: Tue, 1 Apr 2014 18:19:40 +0300 From: "Michael S. Tsirkin" Message-ID: <20140401151940.GB9260@redhat.com> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-14-git-send-email-mst@redhat.com> <20140331171122.GG11125@work-vm> <20140331204900.GC12403@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Michael Roth , "Dr. David Alan Gilbert" , qemu-stable On Mon, Mar 31, 2014 at 10:13:04PM +0100, Peter Maydell wrote: > On 31 March 2014 21:49, Michael S. Tsirkin wrote: > > On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote: > >> * Michael S. Tsirkin (mst@redhat.com) wrote: > >> > CVE-2013-4532 > >> > > >> > 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 failing migration if s->next_packet we read from > >> > the wire exceeds this. > >> > > >> > Similarly, validate rx_fifo against sizeof(s->rx[].data). > >> > > >> > Finally, constrain rx len to a sensibly small positive > >> > value, to avoid integer overruns when data model > >> > later uses this value. > >> > > >> > Reported-by: Michael Roth > >> > Reported-by: Peter Maydell > >> > Signed-off-by: Michael S. Tsirkin > >> > --- > >> > hw/net/stellaris_enet.c | 24 ++++++++++++++++++++---- > >> > 1 file changed, 20 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > >> > index d04e6a4..182fd3e 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; > >> > + int i, v; > >> > > >> > if (version_id != 1) > >> > return -EINVAL; > >> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) > >> > qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data)); > >> > > >> > } > >> > >> The loop that's just off the top here is: > >> for (i = 0; i < 31; i++) { > >> s->rx[i].len = qemu_get_be32(f); > >> qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data)); > >> > >> } > >> > >> Doesn't that 'len' need validating? I assume it's the size of the > >> packet in the fixed sized buffer? (??) > > > > Not that I see where it's used as such. > > In the DATA case of stellaris_enet_read() -- when the current > rx_fifo_len goes to zero we will uncritically set rx_fifo_len to > s->rx[s->next_packet].len. So we must validate that it's between > 0 and 2048 (the size of the rx[].data array), otherwise further > reads from DATA will be able to run off the end of the data array > for the following packet. > > >> > - s->next_packet = qemu_get_be32(f); > >> > - s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f); > >> > - s->rx_fifo_len = qemu_get_be32(f); > >> > + v = qemu_get_be32(f); > >> > + if (v < 0 || v >= ARRAY_SIZE(s->rx)) { > >> > + return -EINVAL; > >> > + } > >> > + s->next_packet = v; > >> > + v = qemu_get_be32(f); > >> > + if (v < 0 || v >= sizeof(s->rx[i].data)) { > >> > + return -EINVAL; > >> > + } > >> > + s->rx_fifo = s->rx[s->next_packet].data + v; > >> > + v = qemu_get_be32(f); > >> > + /* Set limit low enough to avoid integer overflow when > >> > + * we do math on len later, but high enough to avoid > >> > + * truncating any packets. > >> > + */ > >> > + if (v < 0 || v >= 0x100000) { > >> > + return -EINVAL; > >> > + } > >> > + s->rx_fifo_len = v; > >> > >> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within > >> the buffer (or I think it might be legal for the sum to point to the byte after the > >> buffer)? > >> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len > >> related to the current packet in-flight; although I've not quite convinced myself > >> about what is supposed to happen at the end of the packet (which is why > >> I say rx_fifo might point just at? the end. > > > Actually I forgot why I wrote this last check. > > Peter said we should and I thought I see the issue ... > > But I no longer see what kind of damage can rx_fifo_len cause > > unless validated. > > Again, look at the DATA read logic. Every time the guest does a > DATA read, we read from the four bytes at s->rx_fifo, increment > rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len > eventually goes to zero we will (on the subsequent read) reset > both rx_fifo and rx_fifo_len from the next packet in the rx queue. > So if the incoming data sets rx_fifo_len to (let us say) 0x10000, > then the guest can cause us to read well off the end of the rx data > array. This means your check isn't tight enough -- we need to > ensure that rx_fifo and rx_fifo_len between them define a window > into the rx data and nowhere else. As David says this means you > need: > > v1 = qemu_get_be32(f); > if (v1 < 0 || v1 > sizeof(s->rx[i].data)) { > return -EINVAL; > } > s->rx_fifo = s->rx[s->next_packet].data + v1; > v2 = qemu_get_be32(f); > if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) { > return -EINVAL; > } > s->rx_fifo_len = v2; > > The max check on v1 is actually only there to ensure that we > don't have to think about integer overflow when we do the > upper-bound check on v1 + v2. Note that v1 == sizeof(array) > is OK if (and only if) v2 == 0. > > An assert in stellaris_enet_receive() that the net code never > hands us a packet we can't fit in the array wouldn't go amiss > either, but that's a separate issue. > > thanks > -- PMM Got it now, thanks a lot for the explanation.