Re: [Qemu-devel] [Qemu-arm] [PATCH] net: smc91c111: check packet number and data register index

[P J P <ppandit@redhat.com>]

  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,

    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.

| 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.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F