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

[Peter Maydell]

On 26 October 2016 at 13:33, P J P <address@hidden> 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