From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRcKH-0003hp-R0 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 04:05:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRcK9-0002w5-34 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 04:05:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRcK8-0002tH-U5 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 04:05:05 -0500 References: <1454423392-7732-1-git-send-email-ppandit@redhat.com> From: Jason Wang Message-ID: <56B465B7.8000201@redhat.com> Date: Fri, 5 Feb 2016 17:04:55 +0800 MIME-Version: 1.0 In-Reply-To: <1454423392-7732-1-git-send-email-ppandit@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: ne2000: check ring buffer control registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P , QEMU Developers Cc: Yang Hongke , Prasad J Pandit On 02/02/2016 10:29 PM, P J P wrote: > From: Prasad J Pandit > > Ne2000 NIC uses ring buffer of NE2000_MEM_SIZE(49152) > bytes to process network packets. Four registers PSTART, > PSTOP, CURPAGE and BOUNDARY are used to control ring buffer > access. Setting these registers to invalid values could > lead to infinite loop or OOB r/w access issues. Add checks > to avoid it. > > Reported-by: Yang Hongke > Signed-off-by: Prasad J Pandit > --- > hw/net/ne2000.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c > index 9dd0c67..b032212 100644 > --- a/hw/net/ne2000.c > +++ b/hw/net/ne2000.c > @@ -269,6 +269,7 @@ ssize_t ne2000_receive(NetClientState *nc, const uint8_t *buf, size_t size_) > > static void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val) > { > + uint32_t v; > NE2000State *s = opaque; > int offset, page, index; > > @@ -309,17 +310,20 @@ static void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val) > offset = addr | (page << 4); > switch(offset) { > case EN0_STARTPG: > - if (val << 8 <= NE2000_PMEM_END) { > - s->start = val << 8; > + v = val << 8; > + if (v < NE2000_PMEM_END && v < s->stop) { I suspect this could even work. Consider after realizing, s->stop is zero, any attempt to set STARTPG will fail? > + s->start = v; > } > break; > case EN0_STOPPG: > - if (val << 8 <= NE2000_PMEM_END) { > - s->stop = val << 8; > + v = val << 8; > + if (v <= NE2000_PMEM_END && v > s->start) { > + s->stop = v; > } > break; > case EN0_BOUNDARY: > - if (val << 8 < NE2000_PMEM_END) { > + v = val << 8; > + if (v >= s->start && v <= s->stop) { > s->boundary = val; This may not be sufficient, consider: set start to 1 set stop to 100 set boundary to 50 then set stop to 10 I'm thinking maybe we need check during receiving like what we did in dd793a74882477ca38d49e191110c17dfee51dcc? > } > break; > @@ -362,7 +366,8 @@ static void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val) > s->phys[offset - EN1_PHYS] = val; > break; > case EN1_CURPAG: > - if (val << 8 < NE2000_PMEM_END) { > + v = val << 8; > + if (v >= s->start && v <= s->stop) { > s->curpag = val; > } > break;