diff mbox

[v2] net: mipsnet: check transmit buffer size before sending

Message ID 1465382224-6791-1-git-send-email-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit June 8, 2016, 10:37 a.m. UTC 
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 <qiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/mipsnet.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Update as per:
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02089.html

Comments

Jason Wang June 13, 2016, 3:01 a.m. UTC | #1 
On 2016年06月08日 18:37, P J P 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 <qiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/net/mipsnet.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> Update as per:
>    -> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02089.html
>
> diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
> index 740cd98..450e42d 100644
> --- a/hw/net/mipsnet.c
> +++ b/hw/net/mipsnet.c
> @@ -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;

I believe we may still have a buffer overflow here, no?

> -        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;
Prasad Pandit June 13, 2016, 7:17 a.m. UTC | #2 
Hello Jason,

+-- On Mon, 13 Jun 2016, Jason Wang wrote --+
| >       case MIPSNET_TX_DATA_BUFFER:
| >           s->tx_buffer[s->tx_written++] = val;
| 
| I believe we may still have a buffer overflow here, no?

  No, this is the overflow that the patch is meant to fix.
 
| > -        if (s->tx_written == s->tx_count) {
| > +        if ((s->tx_written >= MAX_ETH_FRAME_SIZE)
| > +            || (s->tx_written == s->tx_count)) {
| >               /* Send buffer. */

  Earlier, send buffer would occur when if 'tx_written' reached 'tx_count'. 
With this patch, it'll also occur when 'tx_written' reaches maximum frame 
size.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Jason Wang June 15, 2016, 8:48 a.m. UTC | #3 
On 2016年06月13日 15:17, P J P wrote:
>    Hello Jason,
>
> +-- On Mon, 13 Jun 2016, Jason Wang wrote --+
> | >       case MIPSNET_TX_DATA_BUFFER:
> | >           s->tx_buffer[s->tx_written++] = val;
> |
> | I believe we may still have a buffer overflow here, no?
>
>    No, this is the overflow that the patch is meant to fix.
>   
> | > -        if (s->tx_written == s->tx_count) {
> | > +        if ((s->tx_written >= MAX_ETH_FRAME_SIZE)
> | > +            || (s->tx_written == s->tx_count)) {
> | >               /* Send buffer. */
>
>    Earlier, send buffer would occur when if 'tx_written' reached 'tx_count'.
> With this patch, it'll also occur when 'tx_written' reaches maximum frame
> size.
>
> Thank you.

Ok, applied.

I tend to remove mipsnet in the future (maybe 2.8).

Thanks

> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>
Peter Maydell June 15, 2016, 5:06 p.m. UTC | #4 
On 15 June 2016 at 09:48, Jason Wang <jasowang@redhat.com> wrote:
> I tend to remove mipsnet in the future (maybe 2.8).

If we're going to do this then I think:
 (1) we should remove the whole mipssim machine model,
 not just its ethernet device
 (2) we should announce in the 2.7 release notes that
 we plan to remove it in 2.8

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
index 740cd98..450e42d 100644
--- a/hw/net/mipsnet.c
+++ b/hw/net/mipsnet.c
@@ -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;