Re: [PATCH] net/virtio-user: fix overflow

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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.

> 
> 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.

>  		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