From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7K6E-0004D9-B5 for qemu-devel@nongnu.org; Fri, 11 Dec 2015 04:34:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a7K6A-0004Px-9R for qemu-devel@nongnu.org; Fri, 11 Dec 2015 04:34:50 -0500 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:36799) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7K69-0004Pr-W8 for qemu-devel@nongnu.org; Fri, 11 Dec 2015 04:34:46 -0500 Received: by mail-wm0-x229.google.com with SMTP id n186so20524657wmn.1 for ; Fri, 11 Dec 2015 01:34:45 -0800 (PST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (1.0) From: Dmitry Fleytman In-Reply-To: <566A930C.7030901@redhat.com> Date: Fri, 11 Dec 2015 11:34:40 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: <9F32E8E0-F4BF-4FEE-A18B-2060967DE568@daynix.com> References: <66A887B2-7CFF-45F9-AD7F-1381F8B1F318@daynix.com> <566105A2.6040508@redhat.com> <566A930C.7030901@redhat.com> Subject: Re: [Qemu-devel] net: vmxnet3: memory leakage issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: Qinghao Tang , qemu-devel@nongnu.org, P J P Sent from my iPhone > On 11 Dec 2015, at 11:10, Jason Wang wrote: >=20 >=20 >=20 >> On 12/09/2015 11:28 PM, P J P wrote: >> Hello Jason, Dmitry, >>=20 >> +-- On Tue, 8 Dec 2015, P J P wrote --+ >> | |1) VMXNET3_CMD_QUIESCE_DEV >> |=20 >> | IIUC, it is used to pause the device when the receiver end is unable t= o=20 >> | keee-up with the incoming flow. After a brief period, the operation cou= ld be=20 >> | resumed again. >> |=20 >> | |2) VMXNET3_REG_DSAL >> |=20 >> | Shared memory between a driver and the device appears to be set in t= wo=20 >> | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH)= . I=20 >> | guess 's->device_active' needs to be enabled again while setting the hi= gher=20 >> | part of the address. >>=20 >> Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_= DEV=20 >> case above. It's not clear if the device would be initialised and active w= hile=20 >> setting shared memory via VMXNET3_REG_DSAL/DSAH. >=20 > 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 Sund= ay. Thanks, Dmitry >=20 >>=20 >> =3D=3D=3D >> =46rom 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001 >> From: Prasad J Pandit >> Date: Wed, 9 Dec 2015 20:45:25 +0530 >> Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device >>=20 >> 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. >>=20 >> 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. >>=20 >> This patch adds a check to verify these device states and avoid >> memory leakage during activating the device. >>=20 >> Reported-by: Qinghao Tang >> Signed-off-by: Prasad J Pandit >> --- >> hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++-------- >> hw/net/vmxnet3.h | 6 ++++++ >> 2 files changed, 41 insertions(+), 8 deletions(-) >>=20 >> 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 =3D false; >> + s->device_active =3D VMXNET3_DEV_DEACTIVE; >> +} >> + >> +static void vmxnet3_pause_device(VMXNET3State *s) >> +{ >> + VMW_CBPRN("Pausing vmxnet3..."); >> + s->device_active =3D VMXNET3_DEV_PAUSE; >> +} >> + >> +static void vmxnet3_resume_device(VMXNET3State *s) >> +{ >> + VMW_CBPRN("Resuming vmxnet3..."); >> + s->device_active =3D VMXNET3_DEV_ACTIVE; >> } >>=20 >> static void vmxnet3_reset(VMXNET3State *s) >> @@ -1431,6 +1443,12 @@ static void vmxnet3_activate_device(VMXNET3State *= s) >> return; >> } >>=20 >> + /* Verify if device is active */ >> + if (s->device_active) { >> + VMW_CFPRN("Vmxnet3 device is active"); >> + return; >> + } >=20 > What if guest want to activate a paused device? >=20 >> + >> 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= ) >>=20 >> vmxnet3_reset_mac(s); >>=20 >> - s->device_active =3D true; >> + s->device_active =3D VMXNET3_DEV_ACTIVE; >> } >>=20 >> 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; >>=20 >> 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); >> + } >=20 > Not sure this is the correct behavior. Is there a link to the spec? >=20 >> break; >>=20 >> case VMXNET3_CMD_GET_CONF_INTR: >> @@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET= 3State *s) >>=20 >> switch (s->last_command) { >> case VMXNET3_CMD_ACTIVATE_DEV: >> - ret =3D (s->device_active) ? 0 : -1; >> + ret =3D (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1; >> VMW_CFPRN("Device active: %" PRIx64, ret); >> break; >>=20 >> - case VMXNET3_CMD_RESET_DEV: >> case VMXNET3_CMD_QUIESCE_DEV: >> + ret =3D (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 =3D 0; >> break; >> @@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque, >> * shared address only after we get the high part >> */ >> if (val =3D=3D 0) { >> - s->device_active =3D false; >> + s->device_active =3D VMXNET3_DEV_DEACTIVE; >> } >> s->temp_shared_guest_driver_memory =3D val; >> s->drv_shmem =3D 0; >> @@ -1863,7 +1890,7 @@ static int >> vmxnet3_can_receive(NetClientState *nc) >> { >> VMXNET3State *s =3D 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_STAT= US_UP); >> } >>=20 >> 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 *= / >> }; >>=20 >> +enum { >> + VMXNET3_DEV_DEACTIVE =3D 0x00, /* device deactive */ >> + VMXNET3_DEV_ACTIVE =3D 0x01, /* device active */ >> + VMXNET3_DEV_PAUSE =3D 0x02 /* device pause */ >> +}; >> + >> /* Adaptive Ring Info Flags */ >> #define VMXNET3_DISABLE_ADAPTIVE_RING 1 >=20