Message ID | 1454423392-7732-1-git-send-email-ppandit@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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;
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;
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
+-- 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
+-- 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
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 >
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
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 >
+-- 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
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;