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

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

On 11/18/2015 4:47 PM, Yuanhan Liu wrote:
> On Wed, Nov 18, 2015 at 07:53:24AM +0000, Xie, Huawei wrote:
> ...
>>>  	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;
>> Why do we exit when secure_len is 0 rather than 1? :). Malicious guest
> I confess it's not a proper fix. Making it return an error code, as Rich
> suggested in early email, is better. It's generic enough, as we have to
> check the vec_buf overflow here.
>
> BTW, can we move the vec_buf outside `struct vhost_virtqueue'? It makes
> the structure huge.
>
>> could easily forge the desc len so that secure_len never reach pkt_len
>> even it is not zero so that host enters into dead loop here.
>> Generally speaking, we shouldn't fix for a specific issue,
> Agreed.
>
>> and the
>> security checks should be as few as possible.
> Idealy, yes.
>
>> We need to consider
>> refactor the code here for the generic fix.
> What's your thougths?
Maybe we merge the update_secure_len with the outside loop into a simple
inline function, in which we consider both the max vector number and
desc count to avoid trapped into dead loop. This functions returns a buf
vec with which we could copy securely afterwards.
>
> 	--yliu
>>>  					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;
>>>  		}
>>>