From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 6567B3787 for ; Wed, 18 Nov 2015 09:13:35 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 18 Nov 2015 00:13:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,312,1444719600"; d="scan'208";a="822894217" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 18 Nov 2015 00:13:34 -0800 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 18 Nov 2015 00:13:33 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 18 Nov 2015 00:13:33 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.83]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.193]) with mapi id 14.03.0248.002; Wed, 18 Nov 2015 16:13:31 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [PATCH] vhost: avoid buffer overflow in update_secure_len Thread-Index: AdEhyDF4iWSKtQdITxus5pSs6TsLqw== Date: Wed, 18 Nov 2015 08:13:30 +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> <20151118062550.GA2326@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 08:13:36 -0000 On 11/18/2015 2:25 PM, Yuanhan Liu wrote:=0A= > On Wed, Nov 18, 2015 at 06:13:08AM +0000, Xie, Huawei wrote:=0A= >> On 11/18/2015 10:56 AM, Yuanhan Liu wrote:=0A= >>> On Tue, Nov 17, 2015 at 08:39:30AM -0800, Rich Lane wrote:=0A= >>>> I don't think that adding a SIGINT handler is the right solution, thou= gh. The=0A= >>>> guest app could be killed with another signal (SIGKILL).=0A= >>> Good point.=0A= >>>=0A= >>>> Worse, a malicious or=0A= >>>> buggy guest could write to just that field. vhost should not crash no = matter=0A= >>>> what the guest writes into the virtqueues.=0A= >> Rich, exactly, that has been in our list for a long time. We should=0A= >> ensure that "Any malicious guest couldn't crash host through vrings"=0A= >> otherwise this vhost implementation couldn't be deployed into production= =0A= >> environment.=0A= >> There are many other known security holes in current dpdk vhost in my mi= nd.=0A= >> A very simple example is we don't check the gpa_to_vva return value, so= =0A= >> you could easily put a invalid GPA to vring entry to crash vhost.=0A= >> My plan is to review the vhost implementation, fix all the possible=0A= >> issues in one single patch set, and make the fix performance=0A= > First of all, there is no way you could find all of them out at=0A= > once, for we simply make mistakes, and may miss something here=0A= > and there.=0A= Agree.=0A= >=0A= > And, fixing them in one single patch is not a good pratice; fixing=0A= > them with one issue per patch is. That will make patch eaiser to=0A= > review, yet easier to revert if it's a wrong fix. And it's friendly=0A= > to bisect as well, if it breaks something.=0A= One patch set, not one big patch. Anyway it isn't the key point.=0A= The key point i want to make is we re-review the dpdk vhost=0A= implementation from security point's review, from high level.=0A= Otherwise as i commented in another mail, we add checks here and there,=0A= but actually the fix isn't the generic fix, and some checks could be merged= .=0A= =0A= >=0A= > --yliu=0A= >=0A= >> optimization friendly rather than fix them here and there.=0A= >>=0A= >>> Yeah, I agree with you: though we could fix this issue in the source=0A= >>> side, we also should do some defend here.=0A= >>>=0A= >>> How about following patch then?=0A= >>>=0A= >>> Note that the vec_id overflow check should be done before referencing= =0A= >>> it, but not after. Hence I moved it ahead.=0A= >>>=0A= >>> --yliu=0A= >>>=0A= >>> ---=0A= >>> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxt= x.c=0A= >>> index 9322ce6..08f5942 100644=0A= >>> --- a/lib/librte_vhost/vhost_rxtx.c=0A= >>> +++ b/lib/librte_vhost/vhost_rxtx.c=0A= >>> @@ -132,6 +132,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queu= e_id,=0A= >>> =0A= >>> /* Get descriptor from available ring */=0A= >>> desc =3D &vq->desc[head[packet_success]];=0A= >>> + if (desc->len =3D=3D 0)=0A= >>> + break;=0A= >>> =0A= >>> buff =3D pkts[packet_success];=0A= >>> =0A= >>> @@ -153,6 +155,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queu= e_id,=0A= >>> /* Buffer address translation. */=0A= >>> buff_addr =3D gpa_to_vva(dev, desc->addr);=0A= >>> } else {=0A= >>> + if (desc->len < vq->vhost_hlen)=0A= >>> + break;=0A= >>> vb_offset +=3D vq->vhost_hlen;=0A= >>> hdr =3D 1;=0A= >>> }=0A= >>> @@ -446,6 +450,9 @@ update_secure_len(struct vhost_virtqueue *vq, uint3= 2_t id,=0A= >>> uint32_t vec_id =3D *vec_idx;=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= >>> 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=