Re: [Qemu-devel] [PATCH] net: ne2000: check ring buffer control registers

[Jason Wang]

On 02/02/2016 10:29 PM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
>
> Ne2000 NIC uses ring buffer of NE2000_MEM_SIZE(49152)
> bytes to process network packets. Four registers PSTART,
> PSTOP, CURPAGE and BOUNDARY are used to control ring buffer
> access. Setting these registers to invalid values could
> lead to infinite loop or OOB r/w access issues. Add checks
> to avoid it.
>
> Reported-by: Yang Hongke <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  hw/net/ne2000.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
> index 9dd0c67..b032212 100644
> --- a/hw/net/ne2000.c
> +++ b/hw/net/ne2000.c
> @@ -269,6 +269,7 @@ ssize_t ne2000_receive(NetClientState *nc, const uint8_t 
> *buf, size_t size_)
>  
>  static void ne2000_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>  {
> +    uint32_t v;
>      NE2000State *s = opaque;
>      int offset, page, index;
>  
> @@ -309,17 +310,20 @@ static void ne2000_ioport_write(void *opaque, uint32_t 
> addr, uint32_t val)
>          offset = addr | (page << 4);
>          switch(offset) {
>          case EN0_STARTPG:
> -            if (val << 8 <= NE2000_PMEM_END) {
> -                s->start = val << 8;
> +            v = val << 8;
> +            if (v < NE2000_PMEM_END && v < s->stop) {

I suspect this could even work. Consider after realizing, s->stop is
zero, any attempt to set STARTPG will fail?

> +                s->start = v;
>              }
>              break;
>          case EN0_STOPPG:
> -            if (val << 8 <= NE2000_PMEM_END) {
> -                s->stop = val << 8;
> +            v = val << 8;
> +            if (v <= NE2000_PMEM_END && v > s->start) {
> +                s->stop = v;
>              }
>              break;
>          case EN0_BOUNDARY:
> -            if (val << 8 < NE2000_PMEM_END) {
> +            v = val << 8;
> +            if (v >= s->start && v <= s->stop) {
>                  s->boundary = val;

This may not be sufficient, consider:

set start to 1
set stop to 100
set boundary to 50
then set stop to 10

I'm thinking maybe we need check during receiving like what we did in
dd793a74882477ca38d49e191110c17dfee51dcc?

>              }
>              break;
> @@ -362,7 +366,8 @@ static void ne2000_ioport_write(void *opaque, uint32_t 
> addr, uint32_t val)
>              s->phys[offset - EN1_PHYS] = val;
>              break;
>          case EN1_CURPAG:
> -            if (val << 8 < NE2000_PMEM_END) {
> +            v = val << 8;
> +            if (v >= s->start && v <= s->stop) {
>                  s->curpag = val;
>              }
>              break;