[Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[P J P]

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

When processing MIPSnet I/O port write operation, it uses a
transmit buffer tx_buffer[MAX_ETH_FRAME_SIZE=1514]. Two indices
's->tx_written' and 's->tx_count' are used to control data written
to this buffer. If the two were to be equal before writing, it'd
lead to an OOB write access beyond tx_buffer. Add check to avoid it.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/mipsnet.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
index 740cd98..8d5e5bf 100644
--- a/hw/net/mipsnet.c
+++ b/hw/net/mipsnet.c
@@ -158,7 +158,7 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
     trace_mipsnet_write(addr, val);
     switch (addr) {
     case MIPSNET_TX_DATA_COUNT:
-	s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
+        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
         s->tx_written = 0;
         break;
     case MIPSNET_INT_CTL:
@@ -180,10 +180,12 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
         break;
     case MIPSNET_TX_DATA_BUFFER:
         s->tx_buffer[s->tx_written++] = val;
-        if (s->tx_written == s->tx_count) {
+        if ((s->tx_written >= MAX_ETH_FRAME_SIZE)
+            || (s->tx_written == s->tx_count)) {
             /* Send buffer. */
-            trace_mipsnet_send(s->tx_count);
-            qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, s->tx_count);
+            trace_mipsnet_send(s->tx_written);
+            qemu_send_packet(qemu_get_queue(s->nic),
+                                s->tx_buffer, s->tx_written);
             s->tx_count = s->tx_written = 0;
             s->intctl |= MIPSNET_INTCTL_TXDONE;
             s->busy = 1;
-- 
2.5.5

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[Peter Maydell]

On 2 June 2016 at 07:44, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When processing MIPSnet I/O port write operation, it uses a
> transmit buffer tx_buffer[MAX_ETH_FRAME_SIZE=1514]. Two indices
> 's->tx_written' and 's->tx_count' are used to control data written
> to this buffer. If the two were to be equal before writing, it'd
> lead to an OOB write access beyond tx_buffer. Add check to avoid it.
>
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/mipsnet.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
> index 740cd98..8d5e5bf 100644
> --- a/hw/net/mipsnet.c
> +++ b/hw/net/mipsnet.c
> @@ -158,7 +158,7 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
>      trace_mipsnet_write(addr, val);
>      switch (addr) {
>      case MIPSNET_TX_DATA_COUNT:
> -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
> +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
>          s->tx_written = 0;

This is a behaviour change -- the register will now read
back as MAX_ETH_FRAME_SIZE rather than 0 if written with
an overlarge value.

Do we have any documentation on how this (simulated)
device is supposed to behave in this case?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[P J P]

+-- On Thu, 2 Jun 2016, Peter Maydell wrote --+
| >      case MIPSNET_TX_DATA_COUNT:
| > -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
| > +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
| >          s->tx_written = 0;
| 
| This is a behaviour change -- the register will now read
| back as MAX_ETH_FRAME_SIZE rather than 0 if written with
| an overlarge value.

  IIUC, 's->tx_count' indicates expected packet data length to be processed. 
Maybe if this value was zero, packet was not to be sent; not sure.

| Do we have any documentation on how this (simulated)
| device is supposed to behave in this case?

  I tried to find a specification, but didn't come across any.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[P J P]

+-- On Fri, 3 Jun 2016, P J P wrote --+
| +-- On Thu, 2 Jun 2016, Peter Maydell wrote --+
| | >      case MIPSNET_TX_DATA_COUNT:
| | > -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
| | > +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
| | >          s->tx_written = 0;
| | 
| | This is a behaviour change -- the register will now read
| | back as MAX_ETH_FRAME_SIZE rather than 0 if written with
| | an overlarge value.
| 
|   IIUC, 's->tx_count' indicates expected packet data length to be processed. 
| Maybe if this value was zero, packet was not to be sent; not sure.
| 
| | Do we have any documentation on how this (simulated)
| | device is supposed to behave in this case?

@Jason: @Leon: ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[Jason Wang]

On 2016年06月07日 13:02, P J P wrote:
> +-- On Fri, 3 Jun 2016, P J P wrote --+
> | +-- On Thu, 2 Jun 2016, Peter Maydell wrote --+
> | | >      case MIPSNET_TX_DATA_COUNT:
> | | > -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
> | | > +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
> | | >          s->tx_written = 0;
> | |
> | | This is a behaviour change -- the register will now read
> | | back as MAX_ETH_FRAME_SIZE rather than 0 if written with
> | | an overlarge value.
> |
> |   IIUC, 's->tx_count' indicates expected packet data length to be processed.
> | Maybe if this value was zero, packet was not to be sent; not sure.
> |
> | | Do we have any documentation on how this (simulated)
> | | device is supposed to behave in this case?
>
> @Jason: @Leon: ping...!
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

I could not find its docs, and the driver were even removed from linux 
kernel since it was no longer supported.

We need to fix this issue, but instead of changing the behavior, is it 
better the add a check in MIPSNET_TX_DATA_BUFFER?

Thanks

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[P J P]

  Hello Jason,

+-- On Wed, 8 Jun 2016, Jason Wang wrote --+
| We need to fix this issue, but instead of changing the behavior, is it 
| better the add a check in MIPSNET_TX_DATA_BUFFER?

  Yes, the patch has that too.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[Aurelien Jarno]

On 2016-06-02 10:28, Peter Maydell wrote:
> On 2 June 2016 at 07:44, P J P <ppandit@redhat.com> wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > When processing MIPSnet I/O port write operation, it uses a
> > transmit buffer tx_buffer[MAX_ETH_FRAME_SIZE=1514]. Two indices
> > 's->tx_written' and 's->tx_count' are used to control data written
> > to this buffer. If the two were to be equal before writing, it'd
> > lead to an OOB write access beyond tx_buffer. Add check to avoid it.
> >
> > Reported-by: Li Qiang <liqiang6-s@360.cn>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/net/mipsnet.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
> > index 740cd98..8d5e5bf 100644
> > --- a/hw/net/mipsnet.c
> > +++ b/hw/net/mipsnet.c
> > @@ -158,7 +158,7 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
> >      trace_mipsnet_write(addr, val);
> >      switch (addr) {
> >      case MIPSNET_TX_DATA_COUNT:
> > -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
> > +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
> >          s->tx_written = 0;
> 
> This is a behaviour change -- the register will now read
> back as MAX_ETH_FRAME_SIZE rather than 0 if written with
> an overlarge value.
> 
> Do we have any documentation on how this (simulated)
> device is supposed to behave in this case?

This device is not supported by the linux kernel for more than 2.5 years
(since v3.7). Do we want to keep this device in QEMU? 

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] net: mipsnet: check transmit buffer size before sending

[Jason Wang]

On 2016年06月13日 16:35, Aurelien Jarno wrote:
> On 2016-06-02 10:28, Peter Maydell wrote:
>> On 2 June 2016 at 07:44, P J P <ppandit@redhat.com> wrote:
>>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>>
>>> When processing MIPSnet I/O port write operation, it uses a
>>> transmit buffer tx_buffer[MAX_ETH_FRAME_SIZE=1514]. Two indices
>>> 's->tx_written' and 's->tx_count' are used to control data written
>>> to this buffer. If the two were to be equal before writing, it'd
>>> lead to an OOB write access beyond tx_buffer. Add check to avoid it.
>>>
>>> Reported-by: Li Qiang <liqiang6-s@360.cn>
>>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>>> ---
>>>   hw/net/mipsnet.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
>>> index 740cd98..8d5e5bf 100644
>>> --- a/hw/net/mipsnet.c
>>> +++ b/hw/net/mipsnet.c
>>> @@ -158,7 +158,7 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
>>>       trace_mipsnet_write(addr, val);
>>>       switch (addr) {
>>>       case MIPSNET_TX_DATA_COUNT:
>>> -       s->tx_count = (val <= MAX_ETH_FRAME_SIZE) ? val : 0;
>>> +        s->tx_count = (val < MAX_ETH_FRAME_SIZE) ? val : MAX_ETH_FRAME_SIZE;
>>>           s->tx_written = 0;
>> This is a behaviour change -- the register will now read
>> back as MAX_ETH_FRAME_SIZE rather than 0 if written with
>> an overlarge value.
>>
>> Do we have any documentation on how this (simulated)
>> device is supposed to behave in this case?
> This device is not supported by the linux kernel for more than 2.5 years
> (since v3.7). Do we want to keep this device in QEMU?
>
> Aurelien
>

Right, so I suggest to remove this from qemu.