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