答复: [PATCH] net/virtio-user: fix overflow

[Wenfeng Liu <liuwf@arraynetworks.com.cn>]

Hi Yuanhan,

>On Mon, Mar 13, 2017 at 03:09:15PM +0000, Wenfeng Liu wrote:
>> This commit fixes an array overflow when number of queues is higher than
8.
>
>Firstly, this commit log could be a bit more informative, to something
>like:
>
>    virtio-user limits the qeueue number to 8 but provides no limit
>    check against the queue number input from user. If a bigger queue
>    number (> 8) is given, there is an overflow issue. Doing a sanity
>    check could avoid it.
>

Sure, I will revise the commit log accordingly.

>> 
>> Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")
>> 
>> Signed-off-by: Wenfeng Liu <liuwf@arraynetworks.com.cn>
>> ---
>>  drivers/net/virtio/virtio_pci.h                  | 3 ++-
>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-  
>> drivers/net/virtio/virtio_user/virtio_user_dev.h | 6 +++---
>>  drivers/net/virtio/virtio_user_ethdev.c          | 7 +++++++
>>  4 files changed, 13 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/virtio/virtio_pci.h 
>> b/drivers/net/virtio/virtio_pci.h index 59e45c4..bd940b4 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -160,7 +160,8 @@
>>  /*
>>   * Maximum number of virtqueues per device.
>>   */
>> -#define VIRTIO_MAX_VIRTQUEUES 8
>> +#define VIRTIO_MAX_VIRTQUEUE_PAIRS 8
>> +#define VIRTIO_MAX_VIRTQUEUES VIRTIO_MAX_VIRTQUEUE_PAIRS * 2 + 1
>>  
>>  /* Common configuration */
>>  #define VIRTIO_PCI_CAP_COMMON_CFG	1
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index e7fd65f..5b81676 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -234,7 +234,7 @@ int virtio_user_stop_device(struct virtio_user_dev
*dev)
>>  	uint32_t i, q;
>>  
>>  	dev->vhostfd = -1;
>> -	for (i = 0; i < VIRTIO_MAX_VIRTQUEUES * 2 + 1; ++i) {
>> +	for (i = 0; i < VIRTIO_MAX_VIRTQUEUES; ++i) {
>
>Right, we don't need setup callfd and kickfd for the ctrl-queue.

I did not remove the ctrl-queue. I just redefined the MACRO according to DRY
principle:
#define VIRTIO_MAX_VIRTQUEUES VIRTIO_MAX_VIRTQUEUE_PAIRS * 2 + 1
I noticed that I missed the bracket in the MACRO and will add it in next
version.

>
>>  		dev->kickfds[i] = -1;
>>  		dev->callfds[i] = -1;
>>  	}
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h 
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index 6ecb91e..ba80d05 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -49,8 +49,8 @@ struct virtio_user_dev {
>>  	int		*tapfds;
>>  
>>  	/* for both vhost_user and vhost_kernel */
>> -	int		callfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1];
>> -	int		kickfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1];
>> +	int		callfds[VIRTIO_MAX_VIRTQUEUES];
>> +	int		kickfds[VIRTIO_MAX_VIRTQUEUES];
>>  	int		mac_specified;
>>  	uint32_t	max_queue_pairs;
>>  	uint32_t	queue_pairs;
>> @@ -62,7 +62,7 @@ struct virtio_user_dev {
>>  	uint8_t		status;
>>  	uint8_t		mac_addr[ETHER_ADDR_LEN];
>>  	char		path[PATH_MAX];
>> -	struct vring	vrings[VIRTIO_MAX_VIRTQUEUES * 2 + 1];
>> +	struct vring	vrings[VIRTIO_MAX_VIRTQUEUES];
>
>Have you actually tested your patch? You are removing the vring for
ctrl-queue, which is wrong to me.
>
>>  	struct virtio_user_backend_ops *ops;  };
>>  
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c 
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index 16d1526..d476a2d 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -433,6 +433,13 @@
>>  		goto end;
>>  	}
>>  
>> +	if (queues > VIRTIO_MAX_VIRTQUEUE_PAIRS) {
>> +		PMD_INIT_LOG(ERR, "arg %s %u exceeds the limit %u",
>> +			VIRTIO_USER_ARG_QUEUES_NUM, queues,
>> +			VIRTIO_MAX_VIRTQUEUE_PAIRS);
>> +		goto end;
>> +	}
>
>Yes, we need this check. So, to me, you were actually doing two things in
this patch:
>
>- check the queue number, to avoid overflow
>- remove the callfds and kickfds for ctrl-queue.
>
>That said, please do them in two patches.
>
>Thanks.
>
>	--yliu
>> +
>>  	eth_dev = virtio_user_eth_dev_alloc(name);
>>  	if (!eth_dev) {
>>  		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
>> --
>> 1.8.3.1

Regards,
liuwf