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

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

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