From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id D734E5A35 for ; Wed, 18 Nov 2015 12:15:54 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 18 Nov 2015 03:15:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,312,1444719600"; d="scan'208";a="823015510" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 18 Nov 2015 03:15:29 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 18 Nov 2015 03:15:28 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.110.15) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 18 Nov 2015 03:15:28 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.83]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.223]) with mapi id 14.03.0248.002; Wed, 18 Nov 2015 19:15:26 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [PATCH] vhost: avoid buffer overflow in update_secure_len Thread-Index: AdEh1jN6iWSKtQdITxus5pSs6TsLqw== Date: Wed, 18 Nov 2015 11:15:25 +0000 Message-ID: References: <1447315353-42152-1-git-send-email-rlane@bigswitch.com> <20151112092305.GI2326@yliu-dev.sh.intel.com> <20151117132349.GT2326@yliu-dev.sh.intel.com> <20151118025655.GW2326@yliu-dev.sh.intel.com> <20151118084807.GD2326@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] vhost: avoid buffer overflow in update_secure_len X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2015 11:15:55 -0000 On 11/18/2015 4:47 PM, Yuanhan Liu wrote:=0A= > On Wed, Nov 18, 2015 at 07:53:24AM +0000, Xie, Huawei wrote:=0A= > ...=0A= >>> do {=0A= >>> + if (vec_id >=3D BUF_VECTOR_MAX)=0A= >>> + break;=0A= >>> +=0A= >>> next_desc =3D 0;=0A= >>> len +=3D vq->desc[idx].len;=0A= >>> vq->buf_vec[vec_id].buf_addr =3D vq->desc[idx].addr;=0A= >>> @@ -519,6 +526,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_= t queue_id,=0A= >>> goto merge_rx_exit;=0A= >>> } else {=0A= >>> update_secure_len(vq, res_cur_idx, &secure_len, &vec_idx);=0A= >>> + if (secure_len =3D=3D 0)=0A= >>> + goto merge_rx_exit;=0A= >> Why do we exit when secure_len is 0 rather than 1? :). Malicious guest= =0A= > I confess it's not a proper fix. Making it return an error code, as Rich= =0A= > suggested in early email, is better. It's generic enough, as we have to= =0A= > check the vec_buf overflow here.=0A= >=0A= > BTW, can we move the vec_buf outside `struct vhost_virtqueue'? It makes= =0A= > the structure huge.=0A= >=0A= >> could easily forge the desc len so that secure_len never reach pkt_len= =0A= >> even it is not zero so that host enters into dead loop here.=0A= >> Generally speaking, we shouldn't fix for a specific issue,=0A= > Agreed.=0A= >=0A= >> and the=0A= >> security checks should be as few as possible.=0A= > Idealy, yes.=0A= >=0A= >> We need to consider=0A= >> refactor the code here for the generic fix.=0A= > What's your thougths?=0A= Maybe we merge the update_secure_len with the outside loop into a simple=0A= inline function, in which we consider both the max vector number and=0A= desc count to avoid trapped into dead loop. This functions returns a buf=0A= vec with which we could copy securely afterwards.=0A= >=0A= > --yliu=0A= >>> res_cur_idx++;=0A= >>> }=0A= >>> } while (pkt_len > secure_len);=0A= >>> @@ -631,6 +640,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uin= t16_t queue_id,=0A= >>> uint8_t alloc_err =3D 0;=0A= >>> =0A= >>> desc =3D &vq->desc[head[entry_success]];=0A= >>> + if (desc->len =3D=3D 0)=0A= >>> + break;=0A= >>> =0A= >>> /* Discard first buffer as it is the virtio header */=0A= >>> if (desc->flags & VRING_DESC_F_NEXT) {=0A= >>> @@ -638,6 +649,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uin= t16_t queue_id,=0A= >>> vb_offset =3D 0;=0A= >>> vb_avail =3D desc->len;=0A= >>> } else {=0A= >>> + if (desc->len < vq->vhost_hlen)=0A= >>> + break;=0A= >>> vb_offset =3D vq->vhost_hlen;=0A= >>> vb_avail =3D desc->len - vb_offset;=0A= >>> }=0A= >>>=0A= =0A=