From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzOLc-0002UU-S0 for qemu-devel@nongnu.org; Wed, 26 Oct 2016 09:34:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzOLY-0000xL-3i for qemu-devel@nongnu.org; Wed, 26 Oct 2016 09:34:28 -0400 Received: from mail-ua0-x22e.google.com ([2607:f8b0:400c:c08::22e]:40153) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1bzOLX-0000xE-TB for qemu-devel@nongnu.org; Wed, 26 Oct 2016 09:34:23 -0400 Received: by mail-ua0-x22e.google.com with SMTP id u14so4626666uau.7 for ; Wed, 26 Oct 2016 06:34:23 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1477398120-9000-1-git-send-email-ppandit@redhat.com> From: Peter Maydell Date: Wed, 26 Oct 2016 14:34:02 +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 On 26 October 2016 at 13:33, P J P wrote: > Hello, > > +-- On Tue, 25 Oct 2016, Peter Maydell wrote --+ > | > case 2: /* Packet Number Register */ > | > - s->packet_num = value; > | > + s->packet_num = value & (NUM_PACKETS - 1); > | > | 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. > > NUM_PACKETS macro is used at most places to limit access into 'data' array. > > | Q: what about attempts to use packet numbers that have not > | been allocated by the 91c111's MMU ? > > Added 'n & s->allocated' in patch v2. > > | 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. > > Done in patch v2. > > > | > + return 0x80; > | > | Why 0x80 ? > > Changed it to 0. > > | There's also an access to s->data[] in smc91c111_do_tx() which needs > | some kind of guard. > > Rotuine 'smc91c111_queue_tx' which calls 'smc91c111_do_tx' has a guard in > place before calling _do_tx, The queue_tx function checks s->tx_fifo_len (because it's about to put something into s->tx_fifo[]), but it does not check anything about the values it puts into tx_fifo[]. The do_tx function then does packetnum = s->tx_fifo[i]; p = &s->data[packetnum][0]; where packetnum could be out of bounds. > static void smc91c111_queue_tx(smc91c111_state *s, int packet) > { > if (s->tx_fifo_len == NUM_PACKETS) > return; > s->tx_fifo[s->tx_fifo_len++] = packet; > smc91c111_do_tx(s); > } > > | smc91c111_release_packet() also assumes the packet number it > | is passed is sane. > > It seems to have check in place for s->allocated which > checks if given > packet number is allocated. If the passed 'packet' value is greater than 31 or negative then the function will invoke undefined behaviour. If it's less than 32 but bigger than the max number of packets then it will set allocated to a value it ought not to be able to hold, which makes the rest of the code harder to reason about. > | 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. > > IMO second is better. > > | Do we need to sanitize s->allocated in incoming vmstate data? > > Not sure. It does not seem to be set by user. It can be set by malicious incoming vmstate data (and by arranging for release_packet() to be called with various out-of-bounds values, as described above). I think a bogus allocated bitmap is not exploitable, but it does make the code a bit harder to reason about. thanks -- PMM