From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 7B6725424 for ; Wed, 18 Nov 2015 08:54:25 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 17 Nov 2015 23:54:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,312,1444719600"; d="scan'208";a="602363030" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 17 Nov 2015 23:54:04 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 17 Nov 2015 23:53:55 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.83]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.88]) with mapi id 14.03.0248.002; Wed, 18 Nov 2015 15:53:25 +0800 From: "Xie, Huawei" To: Yuanhan Liu , Rich Lane Thread-Topic: [PATCH] vhost: avoid buffer overflow in update_secure_len Thread-Index: AdEh1jN6iWSKtQdITxus5pSs6TsLqw== Date: Wed, 18 Nov 2015 07:53:24 +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> 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 07:54:26 -0000 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, though= . 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 ma= tter=0A= >> what the guest writes into the virtqueues.=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_rxtx.= 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 queue_= 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 queue_= 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, uint32_= 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= Why do we exit when secure_len is 0 rather than 1? :). Malicious guest=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, and the=0A= security checks should be as few as possible. We need to consider=0A= refactor the code here for the generic fix.=0A= =0A= > res_cur_idx++;=0A= > }=0A= > } while (pkt_len > secure_len);=0A= > @@ -631,6 +640,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint1= 6_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, uint1= 6_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=