diff mbox

[dpdk-dev,v2] vhost: fix segfault on bad descriptor address

Message ID 1468484319-26906-1-git-send-email-i.maximets@samsung.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Ilya Maximets July 14, 2016, 8:18 a.m. UTC 
  In current implementation vhost will crash with segmentation fault
if malicious or buggy virtio application breaks addresses of descriptors.

Before commit 0823c1cb0a73 this crash was reproducible even with
normal DPDK application that tries to change number of virtqueues
dynamically inside VM.

Fix that by checking addresses of descriptors before using.

Also fixed return value on error for 'copy_mbuf_to_desc_mergeable()'
from '-1' to '0' because it returns unsigned value and it means
number of used descriptors.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
Version 2:
	* Rebased on top of current master.
	* host's address now checked in meargeable case,
	  because needed refactoring already done.
	* Commit-message changed because old issue with
	  virtio reload accidentially fixed by commit
	  0823c1cb0a73.

 lib/librte_vhost/vhost_rxtx.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

Yuanhan Liu July 15, 2016, 6:17 a.m. UTC | #1 
On Thu, Jul 14, 2016 at 11:18:39AM +0300, Ilya Maximets wrote:
> In current implementation vhost will crash with segmentation fault
> if malicious or buggy virtio application breaks addresses of descriptors.
> 
> Before commit 0823c1cb0a73 this crash was reproducible even with
> normal DPDK application that tries to change number of virtqueues
> dynamically inside VM.
> 
> Fix that by checking addresses of descriptors before using.
> 
> Also fixed return value on error for 'copy_mbuf_to_desc_mergeable()'
> from '-1' to '0' because it returns unsigned value and it means
> number of used descriptors.

Yeah, that's a good fix. Thanks.

Maybe you'd better make it a standalone patch.

> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> Version 2:
> 	* Rebased on top of current master.
> 	* host's address now checked in meargeable case,
> 	  because needed refactoring already done.
> 	* Commit-message changed because old issue with
> 	  virtio reload accidentially fixed by commit
> 	  0823c1cb0a73.
> 
>  lib/librte_vhost/vhost_rxtx.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 15ca956..31e8b58 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
>  
>  	desc = &vq->desc[desc_idx];
> -	if (unlikely(desc->len < dev->vhost_hlen))
> +	desc_addr = gpa_to_vva(dev, desc->addr);
> +	if (unlikely(desc->len < dev->vhost_hlen || !desc_addr))
>  		return -1;

So, you discards the workaround from Rich?

>  
> -	desc_addr = gpa_to_vva(dev, desc->addr);
>  	rte_prefetch0((void *)(uintptr_t)desc_addr);
>  
>  	virtio_enqueue_offload(m, &virtio_hdr.hdr);
> @@ -182,7 +182,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  				return -1;
>  
>  			desc = &vq->desc[desc->next];
> -			desc_addr   = gpa_to_vva(dev, desc->addr);
> +			desc_addr = gpa_to_vva(dev, desc->addr);
> +			if (unlikely(!desc_addr))
> +				return -1;
> +
>  			desc_offset = 0;
>  			desc_avail  = desc->len;
>  		}
> @@ -387,10 +390,10 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	LOG_DEBUG(VHOST_DATA, "(%d) current index %d | end index %d\n",
>  		dev->vid, cur_idx, end_idx);
>  
> -	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen)
> -		return -1;
> -
>  	desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
> +	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen || !desc_addr)
> +		return 0;
> +
>  	rte_prefetch0((void *)(uintptr_t)desc_addr);
>  
>  	virtio_hdr.num_buffers = end_idx - start_idx;
> @@ -425,6 +428,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  
>  			vec_idx++;
>  			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
> +			if (unlikely(!desc_addr))
> +				return 0;
>  
>  			/* Prefetch buffer address. */
>  			rte_prefetch0((void *)(uintptr_t)desc_addr);
> @@ -507,7 +512,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
>  		*(volatile uint16_t *)&vq->used->idx += nr_used;
>  		vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>  			sizeof(vq->used->idx));
> -		vq->last_used_idx = end;
> +		vq->last_used_idx += nr_used;

Ditto, this may deserve another patch, too.

	--yliu
Ilya Maximets July 15, 2016, 7:23 a.m. UTC | #2 
On 15.07.2016 09:17, Yuanhan Liu wrote:
> On Thu, Jul 14, 2016 at 11:18:39AM +0300, Ilya Maximets wrote:
>> In current implementation vhost will crash with segmentation fault
>> if malicious or buggy virtio application breaks addresses of descriptors.
>>
>> Before commit 0823c1cb0a73 this crash was reproducible even with
>> normal DPDK application that tries to change number of virtqueues
>> dynamically inside VM.
>>
>> Fix that by checking addresses of descriptors before using.
>>
>> Also fixed return value on error for 'copy_mbuf_to_desc_mergeable()'
>> from '-1' to '0' because it returns unsigned value and it means
>> number of used descriptors.
> 
> Yeah, that's a good fix. Thanks.
> 
> Maybe you'd better make it a standalone patch.

Ok. Maybe I should split this patch in two:
1. Fix return value + using of this value (vq->last_used_idx += nr_used;)
2. Check addresses of descriptors.
What do you think?

>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> Version 2:
>> 	* Rebased on top of current master.
>> 	* host's address now checked in meargeable case,
>> 	  because needed refactoring already done.
>> 	* Commit-message changed because old issue with
>> 	  virtio reload accidentially fixed by commit
>> 	  0823c1cb0a73.
>>
>>  lib/librte_vhost/vhost_rxtx.c | 28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
>> index 15ca956..31e8b58 100644
>> --- a/lib/librte_vhost/vhost_rxtx.c
>> +++ b/lib/librte_vhost/vhost_rxtx.c
>> @@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
>>  
>>  	desc = &vq->desc[desc_idx];
>> -	if (unlikely(desc->len < dev->vhost_hlen))
>> +	desc_addr = gpa_to_vva(dev, desc->addr);
>> +	if (unlikely(desc->len < dev->vhost_hlen || !desc_addr))
>>  		return -1;
> 
> So, you discards the workaround from Rich?

I can apply it, if you wish. Should I?

>>  
>> -	desc_addr = gpa_to_vva(dev, desc->addr);
>>  	rte_prefetch0((void *)(uintptr_t)desc_addr);
>>  
>>  	virtio_enqueue_offload(m, &virtio_hdr.hdr);
>> @@ -182,7 +182,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  				return -1;
>>  
>>  			desc = &vq->desc[desc->next];
>> -			desc_addr   = gpa_to_vva(dev, desc->addr);
>> +			desc_addr = gpa_to_vva(dev, desc->addr);
>> +			if (unlikely(!desc_addr))
>> +				return -1;
>> +
>>  			desc_offset = 0;
>>  			desc_avail  = desc->len;
>>  		}
>> @@ -387,10 +390,10 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  	LOG_DEBUG(VHOST_DATA, "(%d) current index %d | end index %d\n",
>>  		dev->vid, cur_idx, end_idx);
>>  
>> -	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen)
>> -		return -1;
>> -
>>  	desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
>> +	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen || !desc_addr)
>> +		return 0;
>> +
>>  	rte_prefetch0((void *)(uintptr_t)desc_addr);
>>  
>>  	virtio_hdr.num_buffers = end_idx - start_idx;
>> @@ -425,6 +428,8 @@ copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  
>>  			vec_idx++;
>>  			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
>> +			if (unlikely(!desc_addr))
>> +				return 0;
>>  
>>  			/* Prefetch buffer address. */
>>  			rte_prefetch0((void *)(uintptr_t)desc_addr);
>> @@ -507,7 +512,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
>>  		*(volatile uint16_t *)&vq->used->idx += nr_used;
>>  		vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>  			sizeof(vq->used->idx));
>> -		vq->last_used_idx = end;
>> +		vq->last_used_idx += nr_used;
> 
> Ditto, this may deserve another patch, too.
> 
> 	--yliu
> 
>
Yuanhan Liu July 15, 2016, 8:40 a.m. UTC | #3 
On Fri, Jul 15, 2016 at 10:23:12AM +0300, Ilya Maximets wrote:
> On 15.07.2016 09:17, Yuanhan Liu wrote:
> > On Thu, Jul 14, 2016 at 11:18:39AM +0300, Ilya Maximets wrote:
> >> In current implementation vhost will crash with segmentation fault
> >> if malicious or buggy virtio application breaks addresses of descriptors.
> >>
> >> Before commit 0823c1cb0a73 this crash was reproducible even with
> >> normal DPDK application that tries to change number of virtqueues
> >> dynamically inside VM.
> >>
> >> Fix that by checking addresses of descriptors before using.
> >>
> >> Also fixed return value on error for 'copy_mbuf_to_desc_mergeable()'
> >> from '-1' to '0' because it returns unsigned value and it means
> >> number of used descriptors.
> > 
> > Yeah, that's a good fix. Thanks.
> > 
> > Maybe you'd better make it a standalone patch.
> 
> Ok. Maybe I should split this patch in two:
> 1. Fix return value + using of this value (vq->last_used_idx += nr_used;)
> 2. Check addresses of descriptors.
> What do you think?

Good to me.

> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> ---
> >> Version 2:
> >> 	* Rebased on top of current master.
> >> 	* host's address now checked in meargeable case,
> >> 	  because needed refactoring already done.
> >> 	* Commit-message changed because old issue with
> >> 	  virtio reload accidentially fixed by commit
> >> 	  0823c1cb0a73.
> >>
> >>  lib/librte_vhost/vhost_rxtx.c | 28 +++++++++++++++++++++-------
> >>  1 file changed, 21 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> >> index 15ca956..31e8b58 100644
> >> --- a/lib/librte_vhost/vhost_rxtx.c
> >> +++ b/lib/librte_vhost/vhost_rxtx.c
> >> @@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
> >>  	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
> >>  
> >>  	desc = &vq->desc[desc_idx];
> >> -	if (unlikely(desc->len < dev->vhost_hlen))
> >> +	desc_addr = gpa_to_vva(dev, desc->addr);
> >> +	if (unlikely(desc->len < dev->vhost_hlen || !desc_addr))
> >>  		return -1;
> > 
> > So, you discards the workaround from Rich?
> 
> I can apply it, if you wish. Should I?

Yeah, it's hard to tell. The performace regression is weird after all.
I'm thinking we should appy it anyway: it saves 10% regression, which
is worthwhile. I think we should also add comments there.

	--yliu
diff mbox

Patch

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 15ca956..31e8b58 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -147,10 +147,10 @@  copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
 
 	desc = &vq->desc[desc_idx];
-	if (unlikely(desc->len < dev->vhost_hlen))
+	desc_addr = gpa_to_vva(dev, desc->addr);
+	if (unlikely(desc->len < dev->vhost_hlen || !desc_addr))
 		return -1;
 
-	desc_addr = gpa_to_vva(dev, desc->addr);
 	rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 	virtio_enqueue_offload(m, &virtio_hdr.hdr);
@@ -182,7 +182,10 @@  copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
 				return -1;
 
 			desc = &vq->desc[desc->next];
-			desc_addr   = gpa_to_vva(dev, desc->addr);
+			desc_addr = gpa_to_vva(dev, desc->addr);
+			if (unlikely(!desc_addr))
+				return -1;
+
 			desc_offset = 0;
 			desc_avail  = desc->len;
 		}
@@ -387,10 +390,10 @@  copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	LOG_DEBUG(VHOST_DATA, "(%d) current index %d | end index %d\n",
 		dev->vid, cur_idx, end_idx);
 
-	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen)
-		return -1;
-
 	desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
+	if (buf_vec[vec_idx].buf_len < dev->vhost_hlen || !desc_addr)
+		return 0;
+
 	rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 	virtio_hdr.num_buffers = end_idx - start_idx;
@@ -425,6 +428,8 @@  copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 			vec_idx++;
 			desc_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
+			if (unlikely(!desc_addr))
+				return 0;
 
 			/* Prefetch buffer address. */
 			rte_prefetch0((void *)(uintptr_t)desc_addr);
@@ -507,7 +512,7 @@  virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		*(volatile uint16_t *)&vq->used->idx += nr_used;
 		vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
 			sizeof(vq->used->idx));
-		vq->last_used_idx = end;
+		vq->last_used_idx += nr_used;
 	}
 
 	if (likely(pkt_idx)) {
@@ -688,6 +693,9 @@  copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		return -1;
 
 	desc_addr = gpa_to_vva(dev, desc->addr);
+	if (unlikely(!desc_addr))
+		return -1;
+
 	hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
 	rte_prefetch0(hdr);
 
@@ -701,6 +709,9 @@  copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		desc = &vq->desc[desc->next];
 
 		desc_addr = gpa_to_vva(dev, desc->addr);
+		if (unlikely(!desc_addr))
+			return -1;
+
 		rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 		desc_offset = 0;
@@ -737,6 +748,9 @@  copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			desc = &vq->desc[desc->next];
 
 			desc_addr = gpa_to_vva(dev, desc->addr);
+			if (unlikely(!desc_addr))
+				return -1;
+
 			rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 			desc_offset = 0;