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

[Peter Maydell <peter.maydell@linaro.org>]

On 26 October 2016 at 21:02, 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 set incorrectly, they could lead to an
> 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 | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> Update per: add check to _release_packet and for 's->allocated'
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06530.html
>
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 3b16dcf..e9b37c2 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -203,6 +203,9 @@ static void smc91c111_pop_tx_fifo_done(smc91c111_state *s)
>  /* Release the memory allocated to a packet.  */
>  static void smc91c111_release_packet(smc91c111_state *s, int packet)
>  {
> +    if (packet >= NUM_PACKETS

Negative numbers ?

> || !(packet & s->allocated)) {

Is this supposed to be checking "is the bit number corresponding
to 'packet' in s->allocated set" ? It doesn't do that.

This change seems likely to break the normal operation of
the device -- how are you testing this patch?

> +        return;
> +    }
>      s->allocated &= ~(1 << packet);
>      if (s->tx_alloc == 0x80)
>          smc91c111_tx_alloc(s);
> @@ -224,6 +227,9 @@ static void smc91c111_do_tx(smc91c111_state *s)
>          return;
>      for (i = 0; i < s->tx_fifo_len; i++) {
>          packetnum = s->tx_fifo[i];
> +        if (packetnum >= NUM_PACKETS || !(packetnum & s->allocated)) {
> +            return;
> +        }

Ditto.

>          p = &s->data[packetnum][0];
>          /* Set status word.  */
>          *(p++) = 0x01;
> @@ -418,7 +424,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
>              /* Ignore.  */
>              return;
>          case 2: /* Packet Number Register */
> -            s->packet_num = value;
> +            s->packet_num = value & 0x03F;
>              return;
>          case 3: case 4: case 5:
>              /* Should be readonly, but linux writes to them anyway. Ignore.  */
> @@ -438,13 +444,17 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
>                      n = s->rx_fifo[0];
>                  else
>                      n = s->packet_num;
> -                p = s->ptr & 0x07ff;
> +                p = s->ptr;
>                  if (s->ptr & 0x4000) {
>                      s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff);
>                  } else {
>                      p += (offset & 3);
>                  }
> -                s->data[n][p] = value;
> +                p &= 0x07ff;
> +                if (s->allocated < NUM_PACKETS
> +                    && n < NUM_PACKETS && n & s->allocated) {

This condition makes no sense. s->allocated is a bitmap, so
checking whether it is < NUM_PACKETS doesn't do anything
useful.

> +                    s->data[n][p] = value;
> +                }
>              }
>              return;
>          case 12: /* Interrupt ACK.  */
> @@ -517,7 +527,8 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
>                  int i;
>                  int n;
>                  n = 0;
> -                for (i = 0; i < NUM_PACKETS; i++) {
> +                for (i = 0;
> +                     s->allocated < NUM_PACKETS && i < NUM_PACKETS; i++) {

This isn't doing anything sensible either.

>                      if (s->allocated & (1 << i))
>                          n++;
>                  }
> @@ -558,9 +569,9 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
>          case 0: case 1: /* MMUCR Busy bit.  */
>              return 0;
>          case 2: /* Packet Number.  */
> -            return s->packet_num;
> +            return s->packet_num & 0x3F;

No point masking on register-reads.

>          case 3: /* Allocation Result.  */
> -            return s->tx_alloc;
> +            return s->tx_alloc < NUM_PACKETS ? s->tx_alloc : 0;
>          case 4: /* TX FIFO */
>              if (s->tx_fifo_done_len == 0)
>                  return 0x80;
> @@ -584,13 +595,18 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
>                      n = s->rx_fifo[0];
>                  else
>                      n = s->packet_num;
> -                p = s->ptr & 0x07ff;
> +                p = s->ptr;
>                  if (s->ptr & 0x4000) {
>                      s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x07ff);
>                  } else {
>                      p += (offset & 3);
>                  }
> -                return s->data[n][p];
> +                p &= 0x07ff;
> +                if (s->allocated < NUM_PACKETS
> +                    && n < NUM_PACKETS && n & s->allocated) {
> +                    return s->data[n][p];
> +                }
> +                return 0;
>              }
>          case 12: /* Interrupt status.  */
>              return s->int_level;
> --
> 2.7.4

thanks
-- PMM