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.
> 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;
Why do we exit when secure_len is 0 rather than 1? :). Malicious guest
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, and the
security checks should be as few as possible. We need to consider
refactor the code here for the generic fix.
> 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;
> }
>
