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 90DB55A68 for ; Wed, 18 Nov 2015 07:24:55 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 17 Nov 2015 22:24:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,311,1444719600"; d="scan'208";a="688214968" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga003.jf.intel.com with ESMTP; 17 Nov 2015 22:24:53 -0800 Date: Wed, 18 Nov 2015 14:25:50 +0800 From: Yuanhan Liu To: "Xie, Huawei" Message-ID: <20151118062550.GA2326@yliu-dev.sh.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 06:24:56 -0000 On Wed, Nov 18, 2015 at 06:13:08AM +0000, Xie, Huawei wrote: > On 11/18/2015 10:56 AM, Yuanhan Liu wrote: > > 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). > > Good point. > > > >> 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. > Rich, exactly, that has been in our list for a long time. We should > ensure that "Any malicious guest couldn't crash host through vrings" > otherwise this vhost implementation couldn't be deployed into production > environment. > There are many other known security holes in current dpdk vhost in my mind. > A very simple example is we don't check the gpa_to_vva return value, so > you could easily put a invalid GPA to vring entry to crash vhost. > My plan is to review the vhost implementation, fix all the possible > issues in one single patch set, and make the fix performance First of all, there is no way you could find all of them out at once, for we simply make mistakes, and may miss something here and there. And, fixing them in one single patch is not a good pratice; fixing them with one issue per patch is. That will make patch eaiser to review, yet easier to revert if it's a wrong fix. And it's friendly to bisect as well, if it breaks something. --yliu > optimization friendly rather than fix them here and there. > > > Yeah, I agree with you: though we could fix this issue in the source > > side, we also should do some defend here. > > > > How about following patch then? > > > > Note that the vec_id overflow check should be done before referencing > > it, but not after. Hence I moved it ahead. > > > > --yliu > > > > --- > > 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, > > > > /* Get descriptor from available ring */ > > desc = &vq->desc[head[packet_success]]; > > + if (desc->len == 0) > > + break; > > > > buff = pkts[packet_success]; > > > > @@ -153,6 +155,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id, > > /* Buffer address translation. */ > > buff_addr = gpa_to_vva(dev, desc->addr); > > } else { > > + if (desc->len < vq->vhost_hlen) > > + break; > > vb_offset += vq->vhost_hlen; > > hdr = 1; > > } > > @@ -446,6 +450,9 @@ update_secure_len(struct vhost_virtqueue *vq, uint32_t id, > > uint32_t vec_id = *vec_idx; > > > > do { > > + if (vec_id >= BUF_VECTOR_MAX) > > + break; > > + > > next_desc = 0; > > len += vq->desc[idx].len; > > vq->buf_vec[vec_id].buf_addr = 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 == 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 = 0; > > > > desc = &vq->desc[head[entry_success]]; > > + if (desc->len == 0) > > + break; > > > > /* 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 = 0; > > vb_avail = desc->len; > > } else { > > + if (desc->len < vq->vhost_hlen) > > + break; > > vb_offset = vq->vhost_hlen; > > vb_avail = desc->len - vb_offset; > > } > > >