From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59805) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz1tE-00067E-Ez for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:35:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz1t9-0001ai-Lv for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:35:40 -0400 Received: from mail-ua0-x22a.google.com ([2607:f8b0:400c:c08::22a]:38873) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1bz1t9-0001aV-A8 for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:35:35 -0400 Received: by mail-ua0-x22a.google.com with SMTP id t11so13364493uaa.5 for ; Tue, 25 Oct 2016 06:35:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1477398120-9000-1-git-send-email-ppandit@redhat.com> References: <1477398120-9000-1-git-send-email-ppandit@redhat.com> From: Peter Maydell Date: Tue, 25 Oct 2016 14:35:13 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [Qemu-arm] [PATCH] net: smc91c111: check packet number and data register index List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qemu Developers , Azure Yang , Jason Wang , qemu-arm , Prasad J Pandit On 25 October 2016 at 13:22, P J P wrote: > From: Prasad J Pandit > > SMSC91C111 Ethernet interface emulator has registers to store > 'packet number' and a 'pointer' to Tx/Rx FIFO buffer area. > These two are used to derive an address to access into 'data' > registers. If they are not set correctly, they could lead to > OOB r/w access beyond packet 'data' area. Add check to avoid it. > > Reported-by: Azure Yang > Signed-off-by: Prasad J Pandit > --- > hw/net/smc91c111.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c > index 3b16dcf..2425da1 100644 > --- a/hw/net/smc91c111.c > +++ b/hw/net/smc91c111.c > @@ -418,7 +418,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset, > /* Ignore. */ > return; > case 2: /* Packet Number Register */ > - s->packet_num = value; > + s->packet_num = value & (NUM_PACKETS - 1); This is inconsistent with how we handle out of range packet numbers read from the rx_fifo[] in the read/write code below. Here we are effectively masking to squash the value into range, but below we will ignore a write or read as 0x80 if there is an out of value packet number. Unfortunately the data sheet doesn't say how bad packet numbers are handled, but we should probably try for consistency. The data sheet says that there are 6 valid bits in the packet number register, not 3, which suggests masking to NUM_PACKETS-1 here isn't the right thing. Q: what about attempts to use packet numbers that have not been allocated by the 91c111's MMU ? > return; > case 3: case 4: case 5: > /* Should be readonly, but linux writes to them anyway. Ignore. */ > @@ -444,7 +444,10 @@ static void smc91c111_writeb(void *opaque, hwaddr offset, > } else { > p += (offset & 3); > } > - s->data[n][p] = value; > + if (n < NUM_PACKETS > + && p < sizeof(s->data[n]) / sizeof(s->data[n][0])) { > + s->data[n][p] = value; > + } > } > return; > case 12: /* Interrupt ACK. */ > @@ -590,7 +593,12 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset) > } else { > p += (offset & 3); > } > - return s->data[n][p]; > + > + if (n < NUM_PACKETS > + && p < sizeof(s->data[n]) / sizeof(s->data[n][0])) { This looks like it's reinventing ARRAY_SIZE(s->data[n]). In any case, rather than doing this check on p I would suggest that we should do: p = s->ptr; if (s->ptr & 0x4000) { s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff); } else { p += (offset & 3); } p &= 0x7ff; (ie move the mask operation down a bit), which will ensure p is always within bounds. Ditto in read code. Perhaps it would be better to factor out the "find the address in the data buffer" code since it's fairly long and duplicated between read and write paths. > + return s->data[n][p]; > + } > + return 0x80; Why 0x80 ? > } > case 12: /* Interrupt status. */ > return s->int_level; There's also an access to s->data[] in smc91c111_do_tx() which needs some kind of guard. smc91c111_release_packet() also assumes the packet number it is passed is sane. Do we need to guard against bad packet numbers in incoming VMState data? The answer is no if we're using the "always check packet_num at point of use" approach, but yes if you're trying to ensure s->packet_num is always a valid value. Do we need to sanitize s->allocated in incoming vmstate data? thanks -- PMM