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

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

[P J P]

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

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 <yanghongke@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 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) {
+                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;
             }
             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;
-- 
2.5.0

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 <pjp@fedoraproject.org>
>
> 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 <yanghongke@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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;

[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:jasowang@redhat.com] 
发送时间: 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 <pjp@fedoraproject.org>
>
> 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 <yanghongke@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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;

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

[P J P]

+-- On Fri, 5 Feb 2016, yanghongke wrote --+
| 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)

  That'd break the loop, instead of resetting index to s->start, when index 
reaches s->stop.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[P J P]

  Hello Jason,

+-- On Fri, 5 Feb 2016, Jason Wang wrote --+
| I suspect this could even work. Consider after realizing, s->stop is
| zero, any attempt to set STARTPG will fail?

 Ie after 'pci_ne2000_realize'? It does not seem to set or reset s->stop 
register.
 
| This may not be sufficient, consider:
| 
| set start to 1
| set stop to 100
| set boundary to 50
| then set stop to 10

  I think any attempts to define the ring buffer limits should reset 
'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a driver 
should be allowed to fiddle with the ring buffers location inside contorller's 
memory. It does not seem right.
 
| I'm thinking maybe we need check during receiving like what we did in
| dd793a74882477ca38d49e191110c17dfee51dcc?

  Check if (s->start == s->stop) at each receive call?

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[P J P]

+-- On Tue, 9 Feb 2016, P J P wrote --+
| +-- On Fri, 5 Feb 2016, Jason Wang wrote --+
| | I suspect this could even work. Consider after realizing, s->stop is
| | zero, any attempt to set STARTPG will fail?
| 
|  Ie after 'pci_ne2000_realize'? It does not seem to set or reset s->stop 
| register.
|  
| | This may not be sufficient, consider:
| | 
| | set start to 1
| | set stop to 100
| | set boundary to 50
| | then set stop to 10
| 
|   I think any attempts to define the ring buffer limits should reset 
| 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a driver 
| should be allowed to fiddle with the ring buffers location inside contorller's 
| memory. It does not seem right.
|  
| | I'm thinking maybe we need check during receiving like what we did in
| | dd793a74882477ca38d49e191110c17dfee51dcc?
| 
|   Check if (s->start == s->stop) at each receive call?


Ping...Jason?
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Jason Wang]

On 02/09/2016 02:47 PM, P J P wrote:
>   Hello Jason,
>
> +-- On Fri, 5 Feb 2016, Jason Wang wrote --+
> | I suspect this could even work. Consider after realizing, s->stop is
> | zero, any attempt to set STARTPG will fail?
>
>  Ie after 'pci_ne2000_realize'? It does not seem to set or reset s->stop 
> register.

I mean with your patch, driver will only be allowed to set EN0_STOPPG
before EN0_STARTPG. So if a driver want to set STARTPG first, the check

+            if (v < NE2000_PMEM_END && v < s->stop) {

will prevent the driver from working correctly since s->stop is zero here.

>  
> | This may not be sufficient, consider:
> | 
> | set start to 1
> | set stop to 100
> | set boundary to 50
> | then set stop to 10
>
>   I think any attempts to define the ring buffer limits should reset 
> 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a driver 
> should be allowed to fiddle with the ring buffers location inside contorller's 
> memory. It does not seem right.

Well, I think we could not assume the behavior of a driver, especially
consider it may be malicious.

>  
> | I'm thinking maybe we need check during receiving like what we did in
> | dd793a74882477ca38d49e191110c17dfee51dcc?
>
>   Check if (s->start == s->stop) at each receive call?

Or in ne2000_buffer_full()?

>
> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

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

[P J P]

   Hello Jason,

+-- On Tue, 23 Feb 2016, Jason Wang wrote --+
| I mean with your patch, driver will only be allowed to set EN0_STOPPG
| before EN0_STARTPG. So if a driver want to set STARTPG first, the check
| 
| +            if (v < NE2000_PMEM_END && v < s->stop) {
| 
| will prevent the driver from working correctly since s->stop is zero here.

  Before drivers could start using NIC, it'll be initialised from its ROM, 
right? Which would set the PSTART & PSTOP registers to the default values. 
With '-net nic,model=ne2k_pci,vlan=0' I see,

    s->start = 19456, s->stop = 32768

| >   I think any attempts to define the ring buffer limits should reset 
| > 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a 
| > driver should be allowed to fiddle with the ring buffers location inside 
| > contorller's memory. It does not seem right.
| 
| Well, I think we could not assume the behavior of a driver, especially
| consider it may be malicious.

  Yes; That's why it'll help to keep drivers from fiddling with the ring 
buffer dimensions. IIUC, there is an upper limit to where PSTOP could 
point[1],

  "In 8 bit mode the PSTOP register should not exceed to 0x60,
    in 16 bit mode the PSTOP register should not exceed to 0x80"

[1] http://www.ethernut.de/pdf/8019asds.pdf

Kernel drivers too seem to have it fixed
  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne.c#n398
  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne2k-pci.c#n342

| >   Check if (s->start == s->stop) at each receive call?
| Or in ne2000_buffer_full()?

  ne2000_buffer_full() too assumes that 's->stop > s->start'

    ...
    avail = (s->stop - s->start) - (index - boundary);

Is there a case wherein drivers need to adjust ring buffer pointers? If not, I 
think it's better to convert EN0_STARTPG:, EN0_STOPPG:, EN0_BOUNDARY: and 
EN1_CURPAG: cases into no-ops.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Jason Wang]

On 02/23/2016 04:28 PM, P J P wrote:
>    Hello Jason,
>
> +-- On Tue, 23 Feb 2016, Jason Wang wrote --+
> | I mean with your patch, driver will only be allowed to set EN0_STOPPG
> | before EN0_STARTPG. So if a driver want to set STARTPG first, the check
> | 
> | +            if (v < NE2000_PMEM_END && v < s->stop) {
> | 
> | will prevent the driver from working correctly since s->stop is zero here.
>
>   Before drivers could start using NIC, it'll be initialised from its ROM, 
> right? Which would set the PSTART & PSTOP registers to the default values. 
> With '-net nic,model=ne2k_pci,vlan=0' I see,
>
>     s->start = 19456, s->stop = 32768

So in this case, if a driver want to do the following things:

1) set s->stop to 16384
2) set s->start to 8192

Then it won't work.

>
> | >   I think any attempts to define the ring buffer limits should reset 
> | > 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a 
> | > driver should be allowed to fiddle with the ring buffers location inside 
> | > contorller's memory. It does not seem right.
> | 
> | Well, I think we could not assume the behavior of a driver, especially
> | consider it may be malicious.
>
>   Yes; That's why it'll help to keep drivers from fiddling with the ring 
> buffer dimensions. 

Right, but since setting STARTPG,STOPPG,BOUNDARY and CURPAG is not
atomic. Try to limit it during value setting is hard to be correct.

> IIUC, there is an upper limit to where PSTOP could 
> point[1],
>
>   "In 8 bit mode the PSTOP register should not exceed to 0x60,
>     in 16 bit mode the PSTOP register should not exceed to 0x80"
>
> [1] http://www.ethernut.de/pdf/8019asds.pdf
>
> Kernel drivers too seem to have it fixed
>   -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne.c#n398
>   -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne2k-pci.c#n342
>
> | >   Check if (s->start == s->stop) at each receive call?
> | Or in ne2000_buffer_full()?
>
>   ne2000_buffer_full() too assumes that 's->stop > s->start'
>
>     ...
>     avail = (s->stop - s->start) - (index - boundary);

Then let's return true when s->stop <= s->start?

> Is there a case wherein drivers need to adjust ring buffer pointers? If not, I 
> think it's better to convert EN0_STARTPG:, EN0_STOPPG:, EN0_BOUNDARY: and 
> EN1_CURPAG: cases into no-ops.

It's really hard to say there's no such driver. Which means if there's
such a driver and it works on real hardware, we need make it work for qemu.

>
> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

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

[P J P]

+-- On Wed, 24 Feb 2016, Jason Wang wrote --+
| Right, but since setting STARTPG,STOPPG,BOUNDARY and CURPAG is not
| atomic. Try to limit it during value setting is hard to be correct.

  I see.
 
| Then let's return true when s->stop <= s->start?

  Okay. Though I'm not sure if it's the right place for it, because more than 
buffer is full, it says buffer does not exist.
 
| It's really hard to say there's no such driver. Which means if there's
| such a driver and it works on real hardware, we need make it work for qemu.

  I guess better would be to see if the hardware standard specifies such an 
interface for the driver. I did not find it mentioned anywhere; Even kernel 
drivers have it fixed.

I'll send a revised patch for ne2000_buffer_full().
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F