Re: [dpdk-dev] [PATCH] vhost: avoid buffer overflow in update_secure_len

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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;
> >  		}
> >
>