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

[yanghongke]

I think the important issue is that we should modify ne2000_receive

> @@ -247,6 +247,7ssize_t ne2000_receive(NetClientState *nc, const uint8_t 
> *buf, size_t size_)
> -     if (index <= s->stop)
> +     if (index < s->stop)


-----邮件原件-----
发件人: Jason Wang [mailto:address@hidden 
发送时间: 2016年2月5日 17:05
收件人: P J P; QEMU Developers
抄送: yanghongke; Prasad J Pandit
主题: Re: [PATCH] net: ne2000: check ring buffer control registers



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;