net: smc91c111: check packet number and data register index

From: Prasad J Pandit <pjp@fedoraproject.org>

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 <azureyang@tencent.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/smc91c111.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

On 25 October 2016 at 13:22, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <azureyang@tencent.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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

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

On 26 October 2016 at 13:33, P J P <ppandit@redhat.com> 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

+-- On Wed, 26 Oct 2016, Peter Maydell wrote --+
| 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.

   Oh, sorry. Fixed in patch v3.
 
| 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.
| 
| 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).

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