[Qemu-devel] net: vmxnet3: memory leakage issue

[Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

   Hello Dmitry, all

A memory leakage issue was reported by Mr Qinghao Tang, CC'd here.

In that, the Qemu VMXNET3 paravirtual device emulator does not check if the 
device is already active, before activating it. This leads to host memory 
leakage via calls to vmxnet_tx_pkt_init(), which calls g_malloc0().

===
static void vmxnet3_activate_device(VMXNET3State *s)
{
    ...
    /* Preallocate TX packet wrapper */
    VMW_CFPRN("Max TX fragments is %u", s->max_tx_frags);
    vmxnet_tx_pkt_init(&s->tx_pkt, s->max_tx_frags, s->peer_has_vhdr);
    ...
}
===

A malicious guest driver could use this flaw to leak excessive memory on the 
host, eventually killing the Qemu process.

Please see attached herein is a proposed (tested)patch which fixes this issue. 
Please let me know if it's okay or requires any changes.

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

[-- Attachment #2: Type: text/plain, Size: 1095 bytes --]

From 70f5e638d6f85a87b6bdeb90585f81b4616d31ef Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Wed, 2 Dec 2015 17:28:06 +0530
Subject: net: vmxnet3: avoid multiple activations of device

Vmxnet3 device emulator does not check if the device is active
before activating it, resulting in memory leakage on the host.
Added a check to verify device state and avoid memory leakage.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/vmxnet3.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 071feeb..7b727b3 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1425,6 +1425,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
         return;
     }
 
+    /* Verify if device is active */
+    if (s->device_active) {
+        VMW_CFPRN("Vmxnet3 device is active");
+        return;
+    }
+
     vmxnet3_adjust_by_guest_type(s);
     vmxnet3_update_features(s);
     vmxnet3_update_pm_state(s);
-- 
2.4.3

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

Hello Prasad,

The patch is good.
Jason, would you apply is from attachment or should it be resent by "git send-email”?

Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>

~Dmitry

> On 2 Dec 2015, at 14:17 PM, P J P <ppandit@redhat.com> wrote:
> 
>  Hello Dmitry, all
> 
> A memory leakage issue was reported by Mr Qinghao Tang, CC'd here.
> 
> In that, the Qemu VMXNET3 paravirtual device emulator does not check if the device is already active, before activating it. This leads to host memory leakage via calls to vmxnet_tx_pkt_init(), which calls g_malloc0().
> 
> ===
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
>   ...
>   /* Preallocate TX packet wrapper */
>   VMW_CFPRN("Max TX fragments is %u", s->max_tx_frags);
>   vmxnet_tx_pkt_init(&s->tx_pkt, s->max_tx_frags, s->peer_has_vhdr);
>   ...
> }
> ===
> 
> A malicious guest driver could use this flaw to leak excessive memory on the host, eventually killing the Qemu process.
> 
> Please see attached herein is a proposed (tested)patch which fixes this issue. Please let me know if it's okay or requires any changes.
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F<0001-net-vmxnet3-avoid-multiple-activations-of-device.patch>


[-- Attachment #2: Type: text/html, Size: 2345 bytes --]

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

  Hello Dmitry,

+-- On Thu, 3 Dec 2015, Dmitry Fleytman wrote --+
| The patch is good.
| Jason, would you apply is from attachment or should it be resent by "git send-email”?
| 
| Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>

Thank you. (/me makes a note to learn about git send-email.)
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Jason Wang]

On 12/03/2015 03:17 PM, Dmitry Fleytman wrote:
> Hello Prasad,
>
> The patch is good.
> Jason, would you apply is from attachment or should it be resent by
> "git send-email”?

Better with "git send-email". And I've a question for this patch which
needs to be answered before merging this.

What if guest deactivate the device before re-activate the device?

Thanks

>
> Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>
>
> ~Dmitry
>
>> On 2 Dec 2015, at 14:17 PM, P J P <ppandit@redhat.com
>> <mailto:ppandit@redhat.com>> wrote:
>>
>>  Hello Dmitry, all
>>
>> A memory leakage issue was reported by Mr Qinghao Tang, CC'd here.
>>
>> In that, the Qemu VMXNET3 paravirtual device emulator does not check
>> if the device is already active, before activating it. This leads to
>> host memory leakage via calls to vmxnet_tx_pkt_init(), which calls
>> g_malloc0().
>>
>> ===
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>>   ...
>>   /* Preallocate TX packet wrapper */
>>   VMW_CFPRN("Max TX fragments is %u", s->max_tx_frags);
>>   vmxnet_tx_pkt_init(&s->tx_pkt, s->max_tx_frags, s->peer_has_vhdr);
>>   ...
>> }
>> ===
>>
>> A malicious guest driver could use this flaw to leak excessive memory
>> on the host, eventually killing the Qemu process.
>>
>> Please see attached herein is a proposed (tested)patch which fixes
>> this issue. Please let me know if it's okay or requires any changes.
>>
>> Thank you.
>> --
>> Prasad J Pandit / Red Hat Product Security Team
>> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B
>> 041F<0001-net-vmxnet3-avoid-multiple-activations-of-device.patch>
>

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Jason,

+-- On Fri, 4 Dec 2015, Jason Wang wrote --+
| Better with "git send-email".

  Okay.

| What if guest deactivate the device before re-activate the device?
|Looks like it could be done through methods:
|
|1) VMXNET3_CMD_QUIESCE_DEV

  IIUC, it is used to pause the device when the receiver end is unable to 
keee-up with the incoming flow. After a brief period, the operation could be 
resumed again.

|2) VMXNET3_REG_DSAL

   Shared memory between a driver and the device appears to be set in two 
steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
guess 's->device_active' needs to be enabled again while setting the higher 
part of the address.

|So looks like need to free both tx_pkt and rx_pkt during deactivating?

  I think freeing 'tx_pkt' & 'rx_pkt' during pause wouldn't be good. It needs 
to call something like - 'vmxnet3_resume_device'.

Please see the patch below for the above two cases, does it look okay?

===
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..4b2305b 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1198,6 +1198,12 @@ static void vmxnet3_deactivate_device(VMXNET3State *s)
     s->device_active = false;
 }
 
+static void vmxnet3_resume_device(VMXNET3State *s)
+{
+    VMW_CBPRN("Resuming vmxnet3...");
+    s->device_active = true;
+}
+
 static void vmxnet3_reset(VMXNET3State *s)
 {
     VMW_CBPRN("Resetting vmxnet3...");
@@ -1627,8 +1633,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
         break;
 
     case VMXNET3_CMD_QUIESCE_DEV:
-        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
-        vmxnet3_deactivate_device(s);
+        if (s->device_active) {
+            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
+            vmxnet3_deactivate_device(s);
+        } else {
+            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
+            vmxnet3_resume_device(s);
+        }
         break;
 
     case VMXNET3_CMD_GET_CONF_INTR:
@@ -1756,6 +1767,9 @@ vmxnet3_io_bar1_write(void *opaque,
          * We already should have low address part.
          */
         s->drv_shmem = s->temp_shared_guest_driver_memory | (val << 32);
+        if (s->drv_shmem) {
+            s->device_active = true;
+        }
         break;
 
     /* Command */
===


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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

   Hello Jason, Dmitry,

+-- On Tue, 8 Dec 2015, P J P wrote --+
| |1) VMXNET3_CMD_QUIESCE_DEV
| 
|   IIUC, it is used to pause the device when the receiver end is unable to 
| keee-up with the incoming flow. After a brief period, the operation could be 
| resumed again.
| 
| |2) VMXNET3_REG_DSAL
| 
|    Shared memory between a driver and the device appears to be set in two 
| steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
| guess 's->device_active' needs to be enabled again while setting the higher 
| part of the address.

Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV 
case above. It's not clear if the device would be initialised and active while 
setting shared memory via VMXNET3_REG_DSAL/DSAH.

===
>From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Wed, 9 Dec 2015 20:45:25 +0530
Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device

Vmxnet3 device emulator does not check if the device is active
before activating it, resulting in memory leakage on the host.
This memory leakage could also occur, if the device was paused
during flow control and activated thereafter.

Introduced a 'Pause' state for the vmxnet3 device to differentiate
it from the earlier active and inactive states. One need not
'activate' the device to resume operation after pause.

This patch adds a check to verify these device states and avoid
memory leakage during activating the device.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++--------
 hw/net/vmxnet3.h |  6 ++++++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..be2c9e2 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
 static void vmxnet3_deactivate_device(VMXNET3State *s)
 {
     VMW_CBPRN("Deactivating vmxnet3...");
-    s->device_active = false;
+    s->device_active = VMXNET3_DEV_DEACTIVE;
+}
+
+static void vmxnet3_pause_device(VMXNET3State *s)
+{
+    VMW_CBPRN("Pausing vmxnet3...");
+    s->device_active = VMXNET3_DEV_PAUSE;
+}
+
+static void vmxnet3_resume_device(VMXNET3State *s)
+{
+    VMW_CBPRN("Resuming vmxnet3...");
+    s->device_active = VMXNET3_DEV_ACTIVE;
 }
 
 static void vmxnet3_reset(VMXNET3State *s)
@@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
         return;
     }
 
+    /* Verify if device is active */
+    if (s->device_active) {
+        VMW_CFPRN("Vmxnet3 device is active");
+        return;
+    }
+
     vmxnet3_adjust_by_guest_type(s);
     vmxnet3_update_features(s);
     vmxnet3_update_pm_state(s);
@@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
 
     vmxnet3_reset_mac(s);
 
-    s->device_active = true;
+    s->device_active = VMXNET3_DEV_ACTIVE;
 }
 
 static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
@@ -1627,8 +1645,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
         break;
 
     case VMXNET3_CMD_QUIESCE_DEV:
-        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
-        vmxnet3_deactivate_device(s);
+        if (s->device_active & VMXNET3_DEV_ACTIVE) {
+            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
+            vmxnet3_pause_device(s);
+        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
+            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
+            vmxnet3_resume_device(s);
+        }
         break;
 
     case VMXNET3_CMD_GET_CONF_INTR:
@@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
 
     switch (s->last_command) {
     case VMXNET3_CMD_ACTIVATE_DEV:
-        ret = (s->device_active) ? 0 : -1;
+        ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1;
         VMW_CFPRN("Device active: %" PRIx64, ret);
         break;
 
-    case VMXNET3_CMD_RESET_DEV:
     case VMXNET3_CMD_QUIESCE_DEV:
+        ret = (s->device_active & VMXNET3_DEV_PAUSE) ? 0 : -1;
+        VMW_CFPRN("Device pause: %" PRIx64, ret);
+        break;
+
+    case VMXNET3_CMD_RESET_DEV:
     case VMXNET3_CMD_GET_QUEUE_STATUS:
         ret = 0;
         break;
@@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque,
          * shared address only after we get the high part
          */
         if (val == 0) {
-            s->device_active = false;
+            s->device_active = VMXNET3_DEV_DEACTIVE;
         }
         s->temp_shared_guest_driver_memory = val;
         s->drv_shmem = 0;
@@ -1863,7 +1890,7 @@ static int
 vmxnet3_can_receive(NetClientState *nc)
 {
     VMXNET3State *s = qemu_get_nic_opaque(nc);
-    return s->device_active &&
+    return (s->device_active & VMXNET3_DEV_ACTIVE) &&
            VMXNET_FLAG_IS_SET(s->link_status_and_speed, VMXNET3_LINK_STATUS_UP);
 }
 
diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h
index f7006af..71a7861 100644
--- a/hw/net/vmxnet3.h
+++ b/hw/net/vmxnet3.h
@@ -202,6 +202,12 @@ enum {
     VMXNET3_CMD_GET_ADAPTIVE_RING_INFO                    /* 0xF00D0009 */
 };
 
+enum {
+    VMXNET3_DEV_DEACTIVE = 0x00,  /* device deactive */
+    VMXNET3_DEV_ACTIVE = 0x01,    /* device active  */
+    VMXNET3_DEV_PAUSE = 0x02      /* device pause */
+};
+
 /* Adaptive Ring Info Flags */
 #define VMXNET3_DISABLE_ADAPTIVE_RING 1
 
-- 
2.4.3
=== 



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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Jason Wang]

On 12/09/2015 11:28 PM, P J P wrote:
>    Hello Jason, Dmitry,
>
> +-- On Tue, 8 Dec 2015, P J P wrote --+
> | |1) VMXNET3_CMD_QUIESCE_DEV
> | 
> |   IIUC, it is used to pause the device when the receiver end is unable to 
> | keee-up with the incoming flow. After a brief period, the operation could be 
> | resumed again.
> | 
> | |2) VMXNET3_REG_DSAL
> | 
> |    Shared memory between a driver and the device appears to be set in two 
> | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
> | guess 's->device_active' needs to be enabled again while setting the higher 
> | part of the address.
>
> Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV 
> case above. It's not clear if the device would be initialised and active while 
> setting shared memory via VMXNET3_REG_DSAL/DSAH.

I think it's possible for attacker. Better wait for Dmitry's answer for
this.

>
> ===
> From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Wed, 9 Dec 2015 20:45:25 +0530
> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
>
> Vmxnet3 device emulator does not check if the device is active
> before activating it, resulting in memory leakage on the host.
> This memory leakage could also occur, if the device was paused
> during flow control and activated thereafter.
>
> Introduced a 'Pause' state for the vmxnet3 device to differentiate
> it from the earlier active and inactive states. One need not
> 'activate' the device to resume operation after pause.
>
> This patch adds a check to verify these device states and avoid
> memory leakage during activating the device.
>
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++--------
>  hw/net/vmxnet3.h |  6 ++++++
>  2 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..be2c9e2 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
>  static void vmxnet3_deactivate_device(VMXNET3State *s)
>  {
>      VMW_CBPRN("Deactivating vmxnet3...");
> -    s->device_active = false;
> +    s->device_active = VMXNET3_DEV_DEACTIVE;
> +}
> +
> +static void vmxnet3_pause_device(VMXNET3State *s)
> +{
> +    VMW_CBPRN("Pausing vmxnet3...");
> +    s->device_active = VMXNET3_DEV_PAUSE;
> +}
> +
> +static void vmxnet3_resume_device(VMXNET3State *s)
> +{
> +    VMW_CBPRN("Resuming vmxnet3...");
> +    s->device_active = VMXNET3_DEV_ACTIVE;
>  }
>  
>  static void vmxnet3_reset(VMXNET3State *s)
> @@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>          return;
>      }
>  
> +    /* Verify if device is active */
> +    if (s->device_active) {
> +        VMW_CFPRN("Vmxnet3 device is active");
> +        return;
> +    }

What if guest want to activate a paused device?

> +
>      vmxnet3_adjust_by_guest_type(s);
>      vmxnet3_update_features(s);
>      vmxnet3_update_pm_state(s);
> @@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>  
>      vmxnet3_reset_mac(s);
>  
> -    s->device_active = true;
> +    s->device_active = VMXNET3_DEV_ACTIVE;
>  }
>  
>  static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
> @@ -1627,8 +1645,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>          break;
>  
>      case VMXNET3_CMD_QUIESCE_DEV:
> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> -        vmxnet3_deactivate_device(s);
> +        if (s->device_active & VMXNET3_DEV_ACTIVE) {
> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> +            vmxnet3_pause_device(s);
> +        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
> +            vmxnet3_resume_device(s);
> +        }

Not sure this is the correct behavior. Is there a link to the spec?

>          break;
>  
>      case VMXNET3_CMD_GET_CONF_INTR:
> @@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
>  
>      switch (s->last_command) {
>      case VMXNET3_CMD_ACTIVATE_DEV:
> -        ret = (s->device_active) ? 0 : -1;
> +        ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1;
>          VMW_CFPRN("Device active: %" PRIx64, ret);
>          break;
>  
> -    case VMXNET3_CMD_RESET_DEV:
>      case VMXNET3_CMD_QUIESCE_DEV:
> +        ret = (s->device_active & VMXNET3_DEV_PAUSE) ? 0 : -1;
> +        VMW_CFPRN("Device pause: %" PRIx64, ret);
> +        break;
> +
> +    case VMXNET3_CMD_RESET_DEV:
>      case VMXNET3_CMD_GET_QUEUE_STATUS:
>          ret = 0;
>          break;
> @@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque,
>           * shared address only after we get the high part
>           */
>          if (val == 0) {
> -            s->device_active = false;
> +            s->device_active = VMXNET3_DEV_DEACTIVE;
>          }
>          s->temp_shared_guest_driver_memory = val;
>          s->drv_shmem = 0;
> @@ -1863,7 +1890,7 @@ static int
>  vmxnet3_can_receive(NetClientState *nc)
>  {
>      VMXNET3State *s = qemu_get_nic_opaque(nc);
> -    return s->device_active &&
> +    return (s->device_active & VMXNET3_DEV_ACTIVE) &&
>             VMXNET_FLAG_IS_SET(s->link_status_and_speed, VMXNET3_LINK_STATUS_UP);
>  }
>  
> diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h
> index f7006af..71a7861 100644
> --- a/hw/net/vmxnet3.h
> +++ b/hw/net/vmxnet3.h
> @@ -202,6 +202,12 @@ enum {
>      VMXNET3_CMD_GET_ADAPTIVE_RING_INFO                    /* 0xF00D0009 */
>  };
>  
> +enum {
> +    VMXNET3_DEV_DEACTIVE = 0x00,  /* device deactive */
> +    VMXNET3_DEV_ACTIVE = 0x01,    /* device active  */
> +    VMXNET3_DEV_PAUSE = 0x02      /* device pause */
> +};
> +
>  /* Adaptive Ring Info Flags */
>  #define VMXNET3_DISABLE_ADAPTIVE_RING 1
>  

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

Sent from my iPhone

> On 11 Dec 2015, at 11:10, Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> 
>> On 12/09/2015 11:28 PM, P J P wrote:
>>   Hello Jason, Dmitry,
>> 
>> +-- On Tue, 8 Dec 2015, P J P wrote --+
>> | |1) VMXNET3_CMD_QUIESCE_DEV
>> | 
>> |   IIUC, it is used to pause the device when the receiver end is unable to 
>> | keee-up with the incoming flow. After a brief period, the operation could be 
>> | resumed again.
>> | 
>> | |2) VMXNET3_REG_DSAL
>> | 
>> |    Shared memory between a driver and the device appears to be set in two 
>> | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
>> | guess 's->device_active' needs to be enabled again while setting the higher 
>> | part of the address.
>> 
>> Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV 
>> case above. It's not clear if the device would be initialised and active while 
>> setting shared memory via VMXNET3_REG_DSAL/DSAH.
> 
> I think it's possible for attacker. Better wait for Dmitry's answer for
> this.

Hi Guys,

I'd like to review this carefully. I'll post my thoughts tomorrow or on Sunday.

Thanks,
Dmitry


> 
>> 
>> ===
>> From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>> Date: Wed, 9 Dec 2015 20:45:25 +0530
>> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
>> 
>> Vmxnet3 device emulator does not check if the device is active
>> before activating it, resulting in memory leakage on the host.
>> This memory leakage could also occur, if the device was paused
>> during flow control and activated thereafter.
>> 
>> Introduced a 'Pause' state for the vmxnet3 device to differentiate
>> it from the earlier active and inactive states. One need not
>> 'activate' the device to resume operation after pause.
>> 
>> This patch adds a check to verify these device states and avoid
>> memory leakage during activating the device.
>> 
>> Reported-by: Qinghao Tang <luodalongde@gmail.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>> hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++--------
>> hw/net/vmxnet3.h |  6 ++++++
>> 2 files changed, 41 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 37373e5..be2c9e2 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
>> static void vmxnet3_deactivate_device(VMXNET3State *s)
>> {
>>     VMW_CBPRN("Deactivating vmxnet3...");
>> -    s->device_active = false;
>> +    s->device_active = VMXNET3_DEV_DEACTIVE;
>> +}
>> +
>> +static void vmxnet3_pause_device(VMXNET3State *s)
>> +{
>> +    VMW_CBPRN("Pausing vmxnet3...");
>> +    s->device_active = VMXNET3_DEV_PAUSE;
>> +}
>> +
>> +static void vmxnet3_resume_device(VMXNET3State *s)
>> +{
>> +    VMW_CBPRN("Resuming vmxnet3...");
>> +    s->device_active = VMXNET3_DEV_ACTIVE;
>> }
>> 
>> static void vmxnet3_reset(VMXNET3State *s)
>> @@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>         return;
>>     }
>> 
>> +    /* Verify if device is active */
>> +    if (s->device_active) {
>> +        VMW_CFPRN("Vmxnet3 device is active");
>> +        return;
>> +    }
> 
> What if guest want to activate a paused device?
> 
>> +
>>     vmxnet3_adjust_by_guest_type(s);
>>     vmxnet3_update_features(s);
>>     vmxnet3_update_pm_state(s);
>> @@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>> 
>>     vmxnet3_reset_mac(s);
>> 
>> -    s->device_active = true;
>> +    s->device_active = VMXNET3_DEV_ACTIVE;
>> }
>> 
>> static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>> @@ -1627,8 +1645,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>>         break;
>> 
>>     case VMXNET3_CMD_QUIESCE_DEV:
>> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
>> -        vmxnet3_deactivate_device(s);
>> +        if (s->device_active & VMXNET3_DEV_ACTIVE) {
>> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
>> +            vmxnet3_pause_device(s);
>> +        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
>> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
>> +            vmxnet3_resume_device(s);
>> +        }
> 
> Not sure this is the correct behavior. Is there a link to the spec?
> 
>>         break;
>> 
>>     case VMXNET3_CMD_GET_CONF_INTR:
>> @@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
>> 
>>     switch (s->last_command) {
>>     case VMXNET3_CMD_ACTIVATE_DEV:
>> -        ret = (s->device_active) ? 0 : -1;
>> +        ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1;
>>         VMW_CFPRN("Device active: %" PRIx64, ret);
>>         break;
>> 
>> -    case VMXNET3_CMD_RESET_DEV:
>>     case VMXNET3_CMD_QUIESCE_DEV:
>> +        ret = (s->device_active & VMXNET3_DEV_PAUSE) ? 0 : -1;
>> +        VMW_CFPRN("Device pause: %" PRIx64, ret);
>> +        break;
>> +
>> +    case VMXNET3_CMD_RESET_DEV:
>>     case VMXNET3_CMD_GET_QUEUE_STATUS:
>>         ret = 0;
>>         break;
>> @@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque,
>>          * shared address only after we get the high part
>>          */
>>         if (val == 0) {
>> -            s->device_active = false;
>> +            s->device_active = VMXNET3_DEV_DEACTIVE;
>>         }
>>         s->temp_shared_guest_driver_memory = val;
>>         s->drv_shmem = 0;
>> @@ -1863,7 +1890,7 @@ static int
>> vmxnet3_can_receive(NetClientState *nc)
>> {
>>     VMXNET3State *s = qemu_get_nic_opaque(nc);
>> -    return s->device_active &&
>> +    return (s->device_active & VMXNET3_DEV_ACTIVE) &&
>>            VMXNET_FLAG_IS_SET(s->link_status_and_speed, VMXNET3_LINK_STATUS_UP);
>> }
>> 
>> diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h
>> index f7006af..71a7861 100644
>> --- a/hw/net/vmxnet3.h
>> +++ b/hw/net/vmxnet3.h
>> @@ -202,6 +202,12 @@ enum {
>>     VMXNET3_CMD_GET_ADAPTIVE_RING_INFO                    /* 0xF00D0009 */
>> };
>> 
>> +enum {
>> +    VMXNET3_DEV_DEACTIVE = 0x00,  /* device deactive */
>> +    VMXNET3_DEV_ACTIVE = 0x01,    /* device active  */
>> +    VMXNET3_DEV_PAUSE = 0x02      /* device pause */
>> +};
>> +
>> /* Adaptive Ring Info Flags */
>> #define VMXNET3_DISABLE_ADAPTIVE_RING 1
> 

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Jason,

+-- On Fri, 11 Dec 2015, Jason Wang wrote --+
| I think it's possible for attacker. Better wait for Dmitry's answer for
| this.

  Okay.
 
| > +    /* Verify if device is active */
| > +    if (s->device_active) {
| > +        VMW_CFPRN("Vmxnet3 device is active");
| > +        return;
| > +    }
| 
| What if guest want to activate a paused device?

  There is a 'resume' operation defined below.
 
| >      case VMXNET3_CMD_QUIESCE_DEV:
| > -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
| > -        vmxnet3_deactivate_device(s);
| > +        if (s->device_active & VMXNET3_DEV_ACTIVE) {
| > +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
| > +            vmxnet3_pause_device(s);
| > +        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
| > +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
| > +            vmxnet3_resume_device(s);
| > +        }
| 
| Not sure this is the correct behavior. Is there a link to the spec?

  I couldn't find a spec for vmxnet3; I referred the vmxnet3 kernel driver, 
which seems to implement suspend & resume functions.

  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/vmxnet3/vmxnet3_drv.c

In general, Ethernet documents talk about 'pause' frame mechanism to stop NIC 
from buffering more data, till it has space available to process more, when it 
resumes its operation.

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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

> On 11 Dec 2015, at 12:04 PM, P J P <ppandit@redhat.com> wrote:
> 
>  Hello Jason,
> 
> +-- On Fri, 11 Dec 2015, Jason Wang wrote --+
> | I think it's possible for attacker. Better wait for Dmitry's answer for
> | this.
> 
>  Okay.
> 
> | > +    /* Verify if device is active */
> | > +    if (s->device_active) {
> | > +        VMW_CFPRN("Vmxnet3 device is active");
> | > +        return;
> | > +    }
> | 
> | What if guest want to activate a paused device?
> 
>  There is a 'resume' operation defined below.
> 
> | >      case VMXNET3_CMD_QUIESCE_DEV:
> | > -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> | > -        vmxnet3_deactivate_device(s);
> | > +        if (s->device_active & VMXNET3_DEV_ACTIVE) {
> | > +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> | > +            vmxnet3_pause_device(s);
> | > +        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
> | > +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
> | > +            vmxnet3_resume_device(s);
> | > +        }
> | 
> | Not sure this is the correct behavior. Is there a link to the spec?
> 
>  I couldn't find a spec for vmxnet3; I referred the vmxnet3 kernel driver, 
> which seems to implement suspend & resume functions.

Unfortunately the spec is not available.
The device was implemented using Linux/Windows drivers as references.

> 
>  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/vmxnet3/vmxnet3_drv.c
> 
> In general, Ethernet documents talk about 'pause' frame mechanism to stop NIC 
> from buffering more data, till it has space available to process more, when it 
> resumes its operation.
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

[-- Attachment #1: Type: text/plain, Size: 7206 bytes --]

Hello Prasad,

On 9 Dec 2015, at 17:28 PM, P J P <ppandit@redhat.com> wrote:
> 
>   Hello Jason, Dmitry,
> 
> +-- On Tue, 8 Dec 2015, P J P wrote --+
> | |1) VMXNET3_CMD_QUIESCE_DEV
> | 
> |   IIUC, it is used to pause the device when the receiver end is unable to 
> | keee-up with the incoming flow. After a brief period, the operation could be 
> | resumed again.
> | 
> | |2) VMXNET3_REG_DSAL
> | 
> |    Shared memory between a driver and the device appears to be set in two 
> | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I 
> | guess 's->device_active' needs to be enabled again while setting the higher 
> | part of the address.
> 
> Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV 
> case above. It's not clear if the device would be initialised and active while 
> setting shared memory via VMXNET3_REG_DSAL/DSAH.
> 
> ===
> From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Wed, 9 Dec 2015 20:45:25 +0530
> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
> 
> Vmxnet3 device emulator does not check if the device is active
> before activating it, resulting in memory leakage on the host.
> This memory leakage could also occur, if the device was paused
> during flow control and activated thereafter.
> 
> Introduced a 'Pause' state for the vmxnet3 device to differentiate
> it from the earlier active and inactive states. One need not
> 'activate' the device to resume operation after pause.
> 
> This patch adds a check to verify these device states and avoid
> memory leakage during activating the device.
> 
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++--------
> hw/net/vmxnet3.h |  6 ++++++
> 2 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..be2c9e2 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
> static void vmxnet3_deactivate_device(VMXNET3State *s)
> {
>     VMW_CBPRN("Deactivating vmxnet3...");
> -    s->device_active = false;
> +    s->device_active = VMXNET3_DEV_DEACTIVE;
> +}
> +
> +static void vmxnet3_pause_device(VMXNET3State *s)
> +{
> +    VMW_CBPRN("Pausing vmxnet3...");
> +    s->device_active = VMXNET3_DEV_PAUSE;
> +}
> +
> +static void vmxnet3_resume_device(VMXNET3State *s)
> +{
> +    VMW_CBPRN("Resuming vmxnet3...");
> +    s->device_active = VMXNET3_DEV_ACTIVE;
> }
> 
> static void vmxnet3_reset(VMXNET3State *s)
> @@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>         return;
>     }
> 
> +    /* Verify if device is active */
> +    if (s->device_active) {
> +        VMW_CFPRN("Vmxnet3 device is active");
> +        return;
> +    }
> +
>     vmxnet3_adjust_by_guest_type(s);
>     vmxnet3_update_features(s);
>     vmxnet3_update_pm_state(s);
> @@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> 
>     vmxnet3_reset_mac(s);
> 
> -    s->device_active = true;
> +    s->device_active = VMXNET3_DEV_ACTIVE;
> }
> 
> static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
> @@ -1627,8 +1645,13 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>         break;
> 
>     case VMXNET3_CMD_QUIESCE_DEV:
> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> -        vmxnet3_deactivate_device(s);
> +        if (s->device_active & VMXNET3_DEV_ACTIVE) {
> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> +            vmxnet3_pause_device(s);
> +        } else if (s->device_active & VMXNET3_DEV_PAUSE) {
> +            VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device");
> +            vmxnet3_resume_device(s);
> +        }

According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip paused/active states.
It always disables device, see vmxnet3_resume() for example (https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423 <https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423>):

Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and then performs activate sequence to launch the device. 

So the correct fix should:

1. On device activation: check if device is active - do nothing
2. In all places that set device_active to false, i.e. device quiesce, reset and VMXNET3_REG_DSAL set to zero: deallocate tx/rx packets as done in vmxnet3_net_uninit():

net_tx_pkt_reset(s->tx_pkt);
net_tx_pkt_uninit(s->tx_pkt);
net_rx_pkt_uninit(s->rx_pkt);

It could be a good idea to extend vmxnet3_deactivate_device() with those lines and call it from every place that sets device_active to false or frees TX/RX packets.

Best Regards,
Dmitry

>         break;
> 
>     case VMXNET3_CMD_GET_CONF_INTR:
> @@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
> 
>     switch (s->last_command) {
>     case VMXNET3_CMD_ACTIVATE_DEV:
> -        ret = (s->device_active) ? 0 : -1;
> +        ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1;
>         VMW_CFPRN("Device active: %" PRIx64, ret);
>         break;
> 
> -    case VMXNET3_CMD_RESET_DEV:
>     case VMXNET3_CMD_QUIESCE_DEV:
> +        ret = (s->device_active & VMXNET3_DEV_PAUSE) ? 0 : -1;
> +        VMW_CFPRN("Device pause: %" PRIx64, ret);
> +        break;
> +
> +    case VMXNET3_CMD_RESET_DEV:
>     case VMXNET3_CMD_GET_QUEUE_STATUS:
>         ret = 0;
>         break;
> @@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque,
>          * shared address only after we get the high part
>          */
>         if (val == 0) {
> -            s->device_active = false;
> +            s->device_active = VMXNET3_DEV_DEACTIVE;
>         }
>         s->temp_shared_guest_driver_memory = val;
>         s->drv_shmem = 0;
> @@ -1863,7 +1890,7 @@ static int
> vmxnet3_can_receive(NetClientState *nc)
> {
>     VMXNET3State *s = qemu_get_nic_opaque(nc);
> -    return s->device_active &&
> +    return (s->device_active & VMXNET3_DEV_ACTIVE) &&
>            VMXNET_FLAG_IS_SET(s->link_status_and_speed, VMXNET3_LINK_STATUS_UP);
> }
> 
> diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h
> index f7006af..71a7861 100644
> --- a/hw/net/vmxnet3.h
> +++ b/hw/net/vmxnet3.h
> @@ -202,6 +202,12 @@ enum {
>     VMXNET3_CMD_GET_ADAPTIVE_RING_INFO                    /* 0xF00D0009 */
> };
> 
> +enum {
> +    VMXNET3_DEV_DEACTIVE = 0x00,  /* device deactive */
> +    VMXNET3_DEV_ACTIVE = 0x01,    /* device active  */
> +    VMXNET3_DEV_PAUSE = 0x02      /* device pause */
> +};
> +
> /* Adaptive Ring Info Flags */
> #define VMXNET3_DISABLE_ADAPTIVE_RING 1
> 
> -- 
> 2.4.3
> === 
> 
> 
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F


[-- Attachment #2: Type: text/html, Size: 12060 bytes --]

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Dmitry, Jason

+-- On Sun, 13 Dec 2015, Dmitry Fleytman wrote --+
| According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip 
| paused/active states. It always disables device, see vmxnet3_resume() for 
|
| <https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423>):
| 
| Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and then 
| performs activate sequence to launch the device.

  Yes, I did look through it. But it wasn't clear how it does flow control. As 
it resets the device on pause and loses any outstanding data. Whereas in the 
vmxnet3 emulator, upon deactivation it merely sets the 's->active_device' flag 
to be false, and the same is checked before receiving new packets. Do either 
of the vmxnet3 implementations perform flow control?(to avoid congestion)
 
| So the correct fix should:
| 
| 1. On device activation: check if device is active - do nothing
| 2. In all places that set device_active to false, i.e. device quiesce, reset and VMXNET3_REG_DSAL set to zero: deallocate tx/rx packets as done in vmxnet3_net_uninit():
| 
| net_tx_pkt_reset(s->tx_pkt);
| net_tx_pkt_uninit(s->tx_pkt);
| net_rx_pkt_uninit(s->rx_pkt);
| 
| It could be a good idea to extend vmxnet3_deactivate_device() with those 
| lines and call it from every place that sets device_active to false or frees 
| TX/RX packets.

  Right. Please see below a new tested patch which does this and fixes the 
host memory leakage issue. Does it look good?

===
>From d4b277788d518e915cc6c20488d587cb5716e96a Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Mon, 14 Dec 2015 16:56:52 +0530
Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device

Vmxnet3 device emulator does not check if the device is active
before activating it, also it did not free the transmit & receive
buffers while deactivating the device, thus resulting in memory
leakage on the host. This patch fixes both these issues to avoid
host memory leakage.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/vmxnet3.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..3936f12 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1195,6 +1195,9 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
 static void vmxnet3_deactivate_device(VMXNET3State *s)
 {
     VMW_CBPRN("Deactivating vmxnet3...");
+    vmxnet_tx_pkt_reset(s->tx_pkt);
+    vmxnet_tx_pkt_uninit(s->tx_pkt);
+    vmxnet_rx_pkt_uninit(s->rx_pkt);
     s->device_active = false;
 }
 
@@ -1204,7 +1207,6 @@ static void vmxnet3_reset(VMXNET3State *s)
 
     vmxnet3_deactivate_device(s);
     vmxnet3_reset_interrupt_states(s);
-    vmxnet_tx_pkt_reset(s->tx_pkt);
     s->drv_shmem = 0;
     s->tx_sop = true;
     s->skip_current_tx_pkt = false;
@@ -1431,6 +1433,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
         return;
     }
 
+    /* Verify if device is active */
+    if (s->device_active) {
+        VMW_CFPRN("Vmxnet3 device is active");
+        return;
+    }
+
     vmxnet3_adjust_by_guest_type(s);
     vmxnet3_update_features(s);
     vmxnet3_update_pm_state(s);
@@ -1627,7 +1635,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
         break;
 
     case VMXNET3_CMD_QUIESCE_DEV:
-        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
+        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device");
         vmxnet3_deactivate_device(s);
         break;
 
@@ -1741,7 +1749,7 @@ vmxnet3_io_bar1_write(void *opaque,
          * shared address only after we get the high part
          */
         if (val == 0) {
-            s->device_active = false;
+            vmxnet3_deactivate_device(s);
         }
         s->temp_shared_guest_driver_memory = val;
         s->drv_shmem = 0;
@@ -2021,9 +2029,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 static void vmxnet3_net_uninit(VMXNET3State *s)
 {
     g_free(s->mcast_list);
-    vmxnet_tx_pkt_reset(s->tx_pkt);
-    vmxnet_tx_pkt_uninit(s->tx_pkt);
-    vmxnet_rx_pkt_uninit(s->rx_pkt);
+    vmxnet3_deactivate_device(s);
     qemu_del_nic(s->nic);
 }
 
-- 
2.4.3
===


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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

Hello Prasad,

> On 14 Dec 2015, at 13:58 PM, P J P <ppandit@redhat.com> wrote:
> 
>  Hello Dmitry, Jason
> 
> +-- On Sun, 13 Dec 2015, Dmitry Fleytman wrote --+
> | According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip 
> | paused/active states. It always disables device, see vmxnet3_resume() for 
> |
> | <https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423>):
> | 
> | Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and then 
> | performs activate sequence to launch the device.
> 
>  Yes, I did look through it. But it wasn't clear how it does flow control. As 
> it resets the device on pause and loses any outstanding data. Whereas in the 
> vmxnet3 emulator, upon deactivation it merely sets the 's->active_device' flag 
> to be false, and the same is checked before receiving new packets. Do either 
> of the vmxnet3 implementations perform flow control?(to avoid congestion)

It’s hard to say as spec is not available.

> 
> | So the correct fix should:
> | 
> | 1. On device activation: check if device is active - do nothing
> | 2. In all places that set device_active to false, i.e. device quiesce, reset and VMXNET3_REG_DSAL set to zero: deallocate tx/rx packets as done in vmxnet3_net_uninit():
> | 
> | net_tx_pkt_reset(s->tx_pkt);
> | net_tx_pkt_uninit(s->tx_pkt);
> | net_rx_pkt_uninit(s->rx_pkt);
> | 
> | It could be a good idea to extend vmxnet3_deactivate_device() with those 
> | lines and call it from every place that sets device_active to false or frees 
> | TX/RX packets.
> 
>  Right. Please see below a new tested patch which does this and fixes the 
> host memory leakage issue. Does it look good?
> 
> ===
> From d4b277788d518e915cc6c20488d587cb5716e96a Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Mon, 14 Dec 2015 16:56:52 +0530
> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
> 
> Vmxnet3 device emulator does not check if the device is active
> before activating it, also it did not free the transmit & receive
> buffers while deactivating the device, thus resulting in memory
> leakage on the host. This patch fixes both these issues to avoid
> host memory leakage.
> 
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/net/vmxnet3.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..3936f12 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1195,6 +1195,9 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
> static void vmxnet3_deactivate_device(VMXNET3State *s)
> {
>     VMW_CBPRN("Deactivating vmxnet3...");
> +    vmxnet_tx_pkt_reset(s->tx_pkt);
> +    vmxnet_tx_pkt_uninit(s->tx_pkt);
> +    vmxnet_rx_pkt_uninit(s->rx_pkt);
>     s->device_active = false;
> }
> 
> @@ -1204,7 +1207,6 @@ static void vmxnet3_reset(VMXNET3State *s)
> 
>     vmxnet3_deactivate_device(s);
>     vmxnet3_reset_interrupt_states(s);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
>     s->drv_shmem = 0;
>     s->tx_sop = true;
>     s->skip_current_tx_pkt = false;
> @@ -1431,6 +1433,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>         return;
>     }
> 
> +    /* Verify if device is active */
> +    if (s->device_active) {
> +        VMW_CFPRN("Vmxnet3 device is active");
> +        return;
> +    }
> +
>     vmxnet3_adjust_by_guest_type(s);
>     vmxnet3_update_features(s);
>     vmxnet3_update_pm_state(s);
> @@ -1627,7 +1635,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>         break;
> 
>     case VMXNET3_CMD_QUIESCE_DEV:
> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> +        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device");
>         vmxnet3_deactivate_device(s);
>         break;
> 
> @@ -1741,7 +1749,7 @@ vmxnet3_io_bar1_write(void *opaque,
>          * shared address only after we get the high part
>          */
>         if (val == 0) {
> -            s->device_active = false;
> +            vmxnet3_deactivate_device(s);
>         }
>         s->temp_shared_guest_driver_memory = val;
>         s->drv_shmem = 0;
> @@ -2021,9 +2029,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
> static void vmxnet3_net_uninit(VMXNET3State *s)
> {
>     g_free(s->mcast_list);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
> -    vmxnet_tx_pkt_uninit(s->tx_pkt);
> -    vmxnet_rx_pkt_uninit(s->rx_pkt);
> +    vmxnet3_deactivate_device(s);
>     qemu_del_nic(s->nic);
> }

The patch looks basically good.
The only issue I can think of is that now vmxnet_tx_pkt_uninit and vmxnet_rx_pkt_uninit may be called a few times in a row. For example guest may quiesce device and then shutdown. In this case vmxnet_tx_pkt_uninit may try to free the same memory twice. Could you take a look at this please?

Thanks for working on this,
Dmitry

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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

   Hello Dmitry,

+-- On Mon, 14 Dec 2015, Dmitry Fleytman wrote --+
| The patch looks basically good.
| The only issue I can think of is that now vmxnet_tx_pkt_uninit and 
| vmxnet_rx_pkt_uninit may be called a few times in a row. For example guest 
| may quiesce device and then shutdown. In this case vmxnet_tx_pkt_uninit may 
| try to free the same memory twice. Could you take a look at this please?

  Yes, I've added a check in vmxnet3_deactivate_device() to avoid double free. 
Please see below an updated patch.

===
>From 3ef66b01874fcc2fe3bfc73d2b61ee3a5b29fdb6 Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Tue, 15 Dec 2015 12:17:28 +0530
Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device

Vmxnet3 device emulator does not check if the device is active
before activating it, also it did not free the transmit & receive
buffers while deactivating the device, thus resulting in memory
leakage on the host. This patch fixes both these issues to avoid
host memory leakage.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/vmxnet3.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..2b4aad7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1194,8 +1194,13 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
 
 static void vmxnet3_deactivate_device(VMXNET3State *s)
 {
-    VMW_CBPRN("Deactivating vmxnet3...");
-    s->device_active = false;
+    if (s->device_active) {
+        VMW_CBPRN("Deactivating vmxnet3...");
+        vmxnet_tx_pkt_reset(s->tx_pkt);
+        vmxnet_tx_pkt_uninit(s->tx_pkt);
+        vmxnet_rx_pkt_uninit(s->rx_pkt);
+        s->device_active = false;
+    }
 }
 
 static void vmxnet3_reset(VMXNET3State *s)
@@ -1204,7 +1209,6 @@ static void vmxnet3_reset(VMXNET3State *s)
 
     vmxnet3_deactivate_device(s);
     vmxnet3_reset_interrupt_states(s);
-    vmxnet_tx_pkt_reset(s->tx_pkt);
     s->drv_shmem = 0;
     s->tx_sop = true;
     s->skip_current_tx_pkt = false;
@@ -1431,6 +1435,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
         return;
     }
 
+    /* Verify if device is active */
+    if (s->device_active) {
+        VMW_CFPRN("Vmxnet3 device is active");
+        return;
+    }
+
     vmxnet3_adjust_by_guest_type(s);
     vmxnet3_update_features(s);
     vmxnet3_update_pm_state(s);
@@ -1627,7 +1637,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
         break;
 
     case VMXNET3_CMD_QUIESCE_DEV:
-        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
+        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device");
         vmxnet3_deactivate_device(s);
         break;
 
@@ -1741,7 +1751,7 @@ vmxnet3_io_bar1_write(void *opaque,
          * shared address only after we get the high part
          */
         if (val == 0) {
-            s->device_active = false;
+            vmxnet3_deactivate_device(s);
         }
         s->temp_shared_guest_driver_memory = val;
         s->drv_shmem = 0;
@@ -2021,9 +2031,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 static void vmxnet3_net_uninit(VMXNET3State *s)
 {
     g_free(s->mcast_list);
-    vmxnet_tx_pkt_reset(s->tx_pkt);
-    vmxnet_tx_pkt_uninit(s->tx_pkt);
-    vmxnet_rx_pkt_uninit(s->rx_pkt);
+    vmxnet3_deactivate_device(s);
     qemu_del_nic(s->nic);
 }
 
-- 
2.4.3
===


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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Dmitry Fleytman]

[-- Attachment #1: Type: text/plain, Size: 4071 bytes --]

Hello Prasad,

Looks good.

Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>

Regards,
Dmitry

> On 15 Dec 2015, at 08:57 AM, P J P <ppandit@redhat.com> wrote:
> 
>   Hello Dmitry,
> 
> +-- On Mon, 14 Dec 2015, Dmitry Fleytman wrote --+
> | The patch looks basically good.
> | The only issue I can think of is that now vmxnet_tx_pkt_uninit and 
> | vmxnet_rx_pkt_uninit may be called a few times in a row. For example guest 
> | may quiesce device and then shutdown. In this case vmxnet_tx_pkt_uninit may 
> | try to free the same memory twice. Could you take a look at this please?
> 
>  Yes, I've added a check in vmxnet3_deactivate_device() to avoid double free. 
> Please see below an updated patch.
> 
> ===
> From 3ef66b01874fcc2fe3bfc73d2b61ee3a5b29fdb6 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Tue, 15 Dec 2015 12:17:28 +0530
> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
> 
> Vmxnet3 device emulator does not check if the device is active
> before activating it, also it did not free the transmit & receive
> buffers while deactivating the device, thus resulting in memory
> leakage on the host. This patch fixes both these issues to avoid
> host memory leakage.
> 
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/net/vmxnet3.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..2b4aad7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1194,8 +1194,13 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
> 
> static void vmxnet3_deactivate_device(VMXNET3State *s)
> {
> -    VMW_CBPRN("Deactivating vmxnet3...");
> -    s->device_active = false;
> +    if (s->device_active) {
> +        VMW_CBPRN("Deactivating vmxnet3...");
> +        vmxnet_tx_pkt_reset(s->tx_pkt);
> +        vmxnet_tx_pkt_uninit(s->tx_pkt);
> +        vmxnet_rx_pkt_uninit(s->rx_pkt);
> +        s->device_active = false;
> +    }
> }
> 
> static void vmxnet3_reset(VMXNET3State *s)
> @@ -1204,7 +1209,6 @@ static void vmxnet3_reset(VMXNET3State *s)
> 
>     vmxnet3_deactivate_device(s);
>     vmxnet3_reset_interrupt_states(s);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
>     s->drv_shmem = 0;
>     s->tx_sop = true;
>     s->skip_current_tx_pkt = false;
> @@ -1431,6 +1435,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>         return;
>     }
> 
> +    /* Verify if device is active */
> +    if (s->device_active) {
> +        VMW_CFPRN("Vmxnet3 device is active");
> +        return;
> +    }
> +
>     vmxnet3_adjust_by_guest_type(s);
>     vmxnet3_update_features(s);
>     vmxnet3_update_pm_state(s);
> @@ -1627,7 +1637,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>         break;
> 
>     case VMXNET3_CMD_QUIESCE_DEV:
> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> +        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device");
>         vmxnet3_deactivate_device(s);
>         break;
> 
> @@ -1741,7 +1751,7 @@ vmxnet3_io_bar1_write(void *opaque,
>          * shared address only after we get the high part
>          */
>         if (val == 0) {
> -            s->device_active = false;
> +            vmxnet3_deactivate_device(s);
>         }
>         s->temp_shared_guest_driver_memory = val;
>         s->drv_shmem = 0;
> @@ -2021,9 +2031,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
> static void vmxnet3_net_uninit(VMXNET3State *s)
> {
>     g_free(s->mcast_list);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
> -    vmxnet_tx_pkt_uninit(s->tx_pkt);
> -    vmxnet_rx_pkt_uninit(s->rx_pkt);
> +    vmxnet3_deactivate_device(s);
>     qemu_del_nic(s->nic);
> }
> 
> -- 
> 2.4.3
> ===
> 
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F


[-- Attachment #2: Type: text/html, Size: 8170 bytes --]

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

+-- On Tue, 15 Dec 2015, Dmitry Fleytman wrote --+
| Hello Prasad,
| 
| Looks good.
| Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>

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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Jason Wang]

On 12/15/2015 04:00 PM, P J P wrote:
> +-- On Tue, 15 Dec 2015, Dmitry Fleytman wrote --+
> | Hello Prasad,
> | 
> | Looks good.
> | Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
>
> Great! Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Patch looks good to me. Queued for 2.6 first.

If you want to make this for 2.5, you may probably want to send a formal
patch with my "Reviewed-by: " to Peter directly consider we are near to
release. And use "For 2.5" as a prefix.

Thanks

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Jason,

+-- On Tue, 15 Dec 2015, Jason Wang wrote --+
| Patch looks good to me. Queued for 2.6 first.
| 
| If you want to make this for 2.5, you may probably want to send a formal
| patch with my "Reviewed-by: " to Peter directly consider we are near to
| release. And use "For 2.5" as a prefix.

  Okay, I'll do that.

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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Miao Yan]

2015-12-14 19:58 GMT+08:00 P J P <ppandit@redhat.com>:
>   Hello Dmitry, Jason
>
> +-- On Sun, 13 Dec 2015, Dmitry Fleytman wrote --+
> | According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip
> | paused/active states. It always disables device, see vmxnet3_resume() for
> |
> | <https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423>):
> |
> | Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and then
> | performs activate sequence to launch the device.
>
>   Yes, I did look through it. But it wasn't clear how it does flow control. As
> it resets the device on pause and loses any outstanding data. Whereas in the
> vmxnet3 emulator, upon deactivation it merely sets the 's->active_device' flag
> to be false, and the same is checked before receiving new packets. Do either
> of the vmxnet3 implementations perform flow control?(to avoid congestion)

So far as I know, vmxnet3 doesn't have a flow control spec.
Same is true for e1000 emulation layer in esxi, writing
to flow control register bits is ignored. Maybe there are
some buffering or throttling layer in-between that do not rely on pause frame.


>
> | So the correct fix should:
> |
> | 1. On device activation: check if device is active - do nothing
> | 2. In all places that set device_active to false, i.e. device quiesce, reset and VMXNET3_REG_DSAL set to zero: deallocate tx/rx packets as done in vmxnet3_net_uninit():
> |
> | net_tx_pkt_reset(s->tx_pkt);
> | net_tx_pkt_uninit(s->tx_pkt);
> | net_rx_pkt_uninit(s->rx_pkt);
> |
> | It could be a good idea to extend vmxnet3_deactivate_device() with those
> | lines and call it from every place that sets device_active to false or frees
> | TX/RX packets.
>
>   Right. Please see below a new tested patch which does this and fixes the
> host memory leakage issue. Does it look good?
>
> ===
> From d4b277788d518e915cc6c20488d587cb5716e96a Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Mon, 14 Dec 2015 16:56:52 +0530
> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device
>
> Vmxnet3 device emulator does not check if the device is active
> before activating it, also it did not free the transmit & receive
> buffers while deactivating the device, thus resulting in memory
> leakage on the host. This patch fixes both these issues to avoid
> host memory leakage.
>
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/vmxnet3.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 37373e5..3936f12 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1195,6 +1195,9 @@ static void vmxnet3_reset_mac(VMXNET3State *s)
>  static void vmxnet3_deactivate_device(VMXNET3State *s)
>  {
>      VMW_CBPRN("Deactivating vmxnet3...");
> +    vmxnet_tx_pkt_reset(s->tx_pkt);
> +    vmxnet_tx_pkt_uninit(s->tx_pkt);
> +    vmxnet_rx_pkt_uninit(s->rx_pkt);
>      s->device_active = false;
>  }
>
> @@ -1204,7 +1207,6 @@ static void vmxnet3_reset(VMXNET3State *s)
>
>      vmxnet3_deactivate_device(s);
>      vmxnet3_reset_interrupt_states(s);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
>      s->drv_shmem = 0;
>      s->tx_sop = true;
>      s->skip_current_tx_pkt = false;
> @@ -1431,6 +1433,12 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>          return;
>      }
>
> +    /* Verify if device is active */
> +    if (s->device_active) {
> +        VMW_CFPRN("Vmxnet3 device is active");
> +        return;
> +    }
> +
>      vmxnet3_adjust_by_guest_type(s);
>      vmxnet3_update_features(s);
>      vmxnet3_update_pm_state(s);
> @@ -1627,7 +1635,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
>          break;
>
>      case VMXNET3_CMD_QUIESCE_DEV:
> -        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device");
> +        VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device");
>          vmxnet3_deactivate_device(s);
>          break;
>
> @@ -1741,7 +1749,7 @@ vmxnet3_io_bar1_write(void *opaque,
>           * shared address only after we get the high part
>           */
>          if (val == 0) {
> -            s->device_active = false;
> +            vmxnet3_deactivate_device(s);
>          }
>          s->temp_shared_guest_driver_memory = val;
>          s->drv_shmem = 0;
> @@ -2021,9 +2029,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
>  static void vmxnet3_net_uninit(VMXNET3State *s)
>  {
>      g_free(s->mcast_list);
> -    vmxnet_tx_pkt_reset(s->tx_pkt);
> -    vmxnet_tx_pkt_uninit(s->tx_pkt);
> -    vmxnet_rx_pkt_uninit(s->rx_pkt);
> +    vmxnet3_deactivate_device(s);
>      qemu_del_nic(s->nic);
>  }
>
> --
> 2.4.3
> ===
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

  Hello Miao,

+-- On Tue, 15 Dec 2015, Miao Yan wrote --+
| So far as I know, vmxnet3 doesn't have a flow control spec. Same is true for 
| e1000 emulation layer in esxi, writing to flow control register bits is 
| ignored. Maybe there are some buffering or throttling layer in-between that 
| do not rely on pause frame.

  I see, okay. Thanks much for sharing these details.

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

Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[Jason Wang]

On 12/02/2015 08:17 PM, P J P wrote:
>   Hello Dmitry, all
>
> A memory leakage issue was reported by Mr Qinghao Tang, CC'd here.
>
> In that, the Qemu VMXNET3 paravirtual device emulator does not check
> if the device is already active, before activating it. This leads to
> host memory leakage via calls to vmxnet_tx_pkt_init(), which calls
> g_malloc0().
>
> ===
> static void vmxnet3_activate_device(VMXNET3State *s)
> {
>    ...
>    /* Preallocate TX packet wrapper */
>    VMW_CFPRN("Max TX fragments is %u", s->max_tx_frags);
>    vmxnet_tx_pkt_init(&s->tx_pkt, s->max_tx_frags, s->peer_has_vhdr);
>    ...
> }
> ===
>
> A malicious guest driver could use this flaw to leak excessive memory
> on the host, eventually killing the Qemu process.
>
> Please see attached herein is a proposed (tested)patch which fixes
> this issue. Please let me know if it's okay or requires any changes.
>
> Thank you.
> -- 
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

What if, guest de-active the device before re-active it?

Looks like it could be done through methods:

1) VMXNET3_CMD_QUIESCE_DEV
2) VMXNET3_REG_DSAL

So looks like need to free both tx_pkt and rx_pkt during deactivating?