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 4F9F55A92 for ; Wed, 18 Nov 2015 03:56:02 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 17 Nov 2015 18:56:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,310,1444719600"; d="scan'208";a="822715177" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga001.jf.intel.com with ESMTP; 17 Nov 2015 18:55:59 -0800 Date: Wed, 18 Nov 2015 10:56:55 +0800 From: Yuanhan Liu To: Rich Lane Message-ID: <20151118025655.GW2326@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> 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 02:56:02 -0000 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. 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; }