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