From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D11135A68 for ; Wed, 18 Nov 2015 06:23:15 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 17 Nov 2015 21:23:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,311,1444719600"; d="scan'208";a="602293574" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 17 Nov 2015 21:23:14 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 17 Nov 2015 21:23:14 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 17 Nov 2015 21:23:14 -0800 Received: from shsmsx152.ccr.corp.intel.com ([169.254.6.193]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.223]) with mapi id 14.03.0248.002; Wed, 18 Nov 2015 13:23:12 +0800 From: "Wang, Zhihong" To: Yuanhan Liu , Rich Lane Thread-Topic: [PATCH] vhost: avoid buffer overflow in update_secure_len Thread-Index: AQHRITsbdEokz+9TVkypRkCdkeNMnZ6f5H0AgACsgYCAAKwrwA== Date: Wed, 18 Nov 2015 05:23:12 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE0941834472@SHSMSX152.ccr.corp.intel.com> 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> In-Reply-To: <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 05:23:16 -0000 > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > Sent: Wednesday, November 18, 2015 10:57 AM > To: Rich Lane > Cc: dev@dpdk.org; Xie, Huawei ; Wang, Zhihong > ; Richardson, Bruce > Subject: Re: [PATCH] vhost: avoid buffer overflow in update_secure_len >=20 > On Tue, Nov 17, 2015 at 08:39:30AM -0800, Rich Lane wrote: > > > > I don't think that adding a SIGINT handler is the right solution, > > though. The guest app could be killed with another signal (SIGKILL). >=20 > Good point. >=20 > > Worse, a malicious or > > buggy guest could write to just that field. vhost should not crash no > > matter what the guest writes into the virtqueues. >=20 > Yeah, I agree with you: though we could fix this issue in the source side= , we also > should do some defend here. >=20 Exactly, DPDK should be able to take care of both ends: # Provide interface for resource cleanup # Be prepared if the app doesn't shutdown properly > How about following patch then? >=20 > Note that the vec_id overflow check should be done before referencing it,= but > not after. Hence I moved it ahead. >=20 > --yliu >=20 > --- > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.= c index > 9322ce6..08f5942 100644 > --- a/lib/librte_vhost/vhost_rxtx.c > +++ b/lib/librte_vhost/vhost_rxtx.c > @@ -132,6 +132,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_= id, >=20 > /* Get descriptor from available ring */ > desc =3D &vq->desc[head[packet_success]]; > + if (desc->len =3D=3D 0) > + break; >=20 > buff =3D pkts[packet_success]; >=20 > @@ -153,6 +155,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_= id, > /* Buffer address translation. */ > buff_addr =3D gpa_to_vva(dev, desc->addr); > } else { > + if (desc->len < vq->vhost_hlen) > + break; > vb_offset +=3D vq->vhost_hlen; > hdr =3D 1; > } > @@ -446,6 +450,9 @@ update_secure_len(struct vhost_virtqueue *vq, uint32_= t > id, > uint32_t vec_id =3D *vec_idx; >=20 > do { > + if (vec_id >=3D BUF_VECTOR_MAX) > + break; > + > next_desc =3D 0; > len +=3D vq->desc[idx].len; > vq->buf_vec[vec_id].buf_addr =3D vq->desc[idx].addr; @@ -519,6 > +526,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id, > goto merge_rx_exit; > } else { > update_secure_len(vq, res_cur_idx, &secure_len, > &vec_idx); > + if (secure_len =3D=3D 0) > + goto merge_rx_exit; > res_cur_idx++; > } > } while (pkt_len > secure_len); > @@ -631,6 +640,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, > uint16_t queue_id, > uint8_t alloc_err =3D 0; >=20 > desc =3D &vq->desc[head[entry_success]]; > + if (desc->len =3D=3D 0) > + break; >=20 > /* Discard first buffer as it is the virtio header */ > if (desc->flags & VRING_DESC_F_NEXT) { @@ -638,6 +649,8 @@ > rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id, > vb_offset =3D 0; > vb_avail =3D desc->len; > } else { > + if (desc->len < vq->vhost_hlen) > + break; > vb_offset =3D vq->vhost_hlen; > vb_avail =3D desc->len - vb_offset; > }