Re: [PATCH] vhost: avoid buffer overflow in update_secure_len

["Xie, Huawei" <huawei.xie@intel.com>]

On 11/18/2015 2:25 PM, Yuanhan Liu wrote:
> 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.
Agree.
>
> 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.
One patch set, not one big patch. Anyway it isn't the key point.
The key point i want to make is we re-review the dpdk vhost
implementation from security point's review, from high level.
Otherwise as i commented in another mail, we add checks here and there,
but actually the fix isn't the generic fix, and some checks could be merged.

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