* [Qemu-devel] net: vmxnet3: memory leakage issue
@ 2015-12-02 12:17 P J P
2015-12-03 7:17 ` Dmitry Fleytman
2015-12-04 2:22 ` Jason Wang
0 siblings, 2 replies; 21+ messages in thread
From: P J P @ 2015-12-02 12:17 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang
[-- 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-02 12:17 [Qemu-devel] net: vmxnet3: memory leakage issue P J P
@ 2015-12-03 7:17 ` Dmitry Fleytman
2015-12-03 11:20 ` P J P
2015-12-04 3:16 ` Jason Wang
2015-12-04 2:22 ` Jason Wang
1 sibling, 2 replies; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-03 7:17 UTC (permalink / raw)
To: P J P, Jason Wang; +Cc: Qinghao Tang, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-03 7:17 ` Dmitry Fleytman
@ 2015-12-03 11:20 ` P J P
2015-12-04 3:16 ` Jason Wang
1 sibling, 0 replies; 21+ messages in thread
From: P J P @ 2015-12-03 11:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Qinghao Tang, Dmitry Fleytman, Jason Wang
[-- 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-02 12:17 [Qemu-devel] net: vmxnet3: memory leakage issue P J P
2015-12-03 7:17 ` Dmitry Fleytman
@ 2015-12-04 2:22 ` Jason Wang
1 sibling, 0 replies; 21+ messages in thread
From: Jason Wang @ 2015-12-04 2:22 UTC (permalink / raw)
To: P J P, qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang
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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-03 7:17 ` Dmitry Fleytman
2015-12-03 11:20 ` P J P
@ 2015-12-04 3:16 ` Jason Wang
2015-12-08 10:17 ` P J P
1 sibling, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-12-04 3:16 UTC (permalink / raw)
To: Dmitry Fleytman, P J P; +Cc: Qinghao Tang, qemu-devel
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>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-04 3:16 ` Jason Wang
@ 2015-12-08 10:17 ` P J P
2015-12-09 15:28 ` P J P
0 siblings, 1 reply; 21+ messages in thread
From: P J P @ 2015-12-08 10:17 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-08 10:17 ` P J P
@ 2015-12-09 15:28 ` P J P
2015-12-11 9:10 ` Jason Wang
2015-12-13 9:45 ` Dmitry Fleytman
0 siblings, 2 replies; 21+ messages in thread
From: P J P @ 2015-12-09 15:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-09 15:28 ` P J P
@ 2015-12-11 9:10 ` Jason Wang
2015-12-11 9:34 ` Dmitry Fleytman
2015-12-11 10:04 ` P J P
2015-12-13 9:45 ` Dmitry Fleytman
1 sibling, 2 replies; 21+ messages in thread
From: Jason Wang @ 2015-12-11 9:10 UTC (permalink / raw)
To: P J P, qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-11 9:10 ` Jason Wang
@ 2015-12-11 9:34 ` Dmitry Fleytman
2015-12-11 10:04 ` P J P
1 sibling, 0 replies; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-11 9:34 UTC (permalink / raw)
To: Jason Wang; +Cc: Qinghao Tang, qemu-devel, P J P
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-11 9:10 ` Jason Wang
2015-12-11 9:34 ` Dmitry Fleytman
@ 2015-12-11 10:04 ` P J P
2015-12-13 8:27 ` Dmitry Fleytman
1 sibling, 1 reply; 21+ messages in thread
From: P J P @ 2015-12-11 10:04 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Qinghao Tang, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-11 10:04 ` P J P
@ 2015-12-13 8:27 ` Dmitry Fleytman
0 siblings, 0 replies; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-13 8:27 UTC (permalink / raw)
To: P J P; +Cc: Qinghao Tang, Jason Wang, qemu-devel
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-09 15:28 ` P J P
2015-12-11 9:10 ` Jason Wang
@ 2015-12-13 9:45 ` Dmitry Fleytman
2015-12-14 11:58 ` P J P
1 sibling, 1 reply; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-13 9:45 UTC (permalink / raw)
To: P J P; +Cc: Qinghao Tang, Jason Wang, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-13 9:45 ` Dmitry Fleytman
@ 2015-12-14 11:58 ` P J P
2015-12-14 17:27 ` Dmitry Fleytman
2015-12-15 8:43 ` Miao Yan
0 siblings, 2 replies; 21+ messages in thread
From: P J P @ 2015-12-14 11:58 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Qinghao Tang, Jason Wang, qemu-devel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-14 11:58 ` P J P
@ 2015-12-14 17:27 ` Dmitry Fleytman
2015-12-15 6:57 ` P J P
2015-12-15 8:43 ` Miao Yan
1 sibling, 1 reply; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-14 17:27 UTC (permalink / raw)
To: P J P; +Cc: Qinghao Tang, Jason Wang, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-14 17:27 ` Dmitry Fleytman
@ 2015-12-15 6:57 ` P J P
2015-12-15 7:01 ` Dmitry Fleytman
0 siblings, 1 reply; 21+ messages in thread
From: P J P @ 2015-12-15 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-15 6:57 ` P J P
@ 2015-12-15 7:01 ` Dmitry Fleytman
2015-12-15 8:00 ` P J P
0 siblings, 1 reply; 21+ messages in thread
From: Dmitry Fleytman @ 2015-12-15 7:01 UTC (permalink / raw)
To: P J P; +Cc: Qinghao Tang, Jason Wang, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-15 7:01 ` Dmitry Fleytman
@ 2015-12-15 8:00 ` P J P
2015-12-15 8:24 ` Jason Wang
0 siblings, 1 reply; 21+ messages in thread
From: P J P @ 2015-12-15 8:00 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Qinghao Tang, Jason Wang, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-15 8:00 ` P J P
@ 2015-12-15 8:24 ` Jason Wang
2015-12-15 8:50 ` P J P
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-12-15 8:24 UTC (permalink / raw)
To: P J P, Dmitry Fleytman; +Cc: Qinghao Tang, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-14 11:58 ` P J P
2015-12-14 17:27 ` Dmitry Fleytman
@ 2015-12-15 8:43 ` Miao Yan
2015-12-15 10:08 ` P J P
1 sibling, 1 reply; 21+ messages in thread
From: Miao Yan @ 2015-12-15 8:43 UTC (permalink / raw)
To: P J P; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang, qemu-devel
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-15 8:24 ` Jason Wang
@ 2015-12-15 8:50 ` P J P
0 siblings, 0 replies; 21+ messages in thread
From: P J P @ 2015-12-15 8:50 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Qinghao Tang, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] net: vmxnet3: memory leakage issue
2015-12-15 8:43 ` Miao Yan
@ 2015-12-15 10:08 ` P J P
0 siblings, 0 replies; 21+ messages in thread
From: P J P @ 2015-12-15 10:08 UTC (permalink / raw)
To: Miao Yan; +Cc: Dmitry Fleytman, Qinghao Tang, Jason Wang, qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-12-15 10:09 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-02 12:17 [Qemu-devel] net: vmxnet3: memory leakage issue P J P
2015-12-03 7:17 ` Dmitry Fleytman
2015-12-03 11:20 ` P J P
2015-12-04 3:16 ` Jason Wang
2015-12-08 10:17 ` P J P
2015-12-09 15:28 ` P J P
2015-12-11 9:10 ` Jason Wang
2015-12-11 9:34 ` Dmitry Fleytman
2015-12-11 10:04 ` P J P
2015-12-13 8:27 ` Dmitry Fleytman
2015-12-13 9:45 ` Dmitry Fleytman
2015-12-14 11:58 ` P J P
2015-12-14 17:27 ` Dmitry Fleytman
2015-12-15 6:57 ` P J P
2015-12-15 7:01 ` Dmitry Fleytman
2015-12-15 8:00 ` P J P
2015-12-15 8:24 ` Jason Wang
2015-12-15 8:50 ` P J P
2015-12-15 8:43 ` Miao Yan
2015-12-15 10:08 ` P J P
2015-12-04 2:22 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.