diff mbox

[dpdk-dev] net/virtio-user: fix overflow

Message ID 1489417755-17074-1-git-send-email-liuwf@arraynetworks.com.cn (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Wenfeng Liu March 13, 2017, 3:09 p.m. UTC 
  This commit fixes an array overflow when number of queues is higher than 8.

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(-)

Comments

Yuanhan Liu March 14, 2017, 6:44 a.m. UTC | #1 
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
Wenfeng Liu March 14, 2017, 8:20 a.m. UTC | #2 
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
Yuanhan Liu March 14, 2017, 8:28 a.m. UTC | #3 
On Tue, Mar 14, 2017 at 04:20:39PM +0800, Wenfeng Liu wrote:
> 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:

Oh, right, sorry, I overlooked it. Then for this patch, it's okay to me.

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

Yes, please. Also, please put a "Cc: stable@dpdk.org" before your
Signed-of-by: it's a candidate for stable releases.

	--yliu
diff mbox

Patch

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) {
 		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];
 	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;
+	}
+
 	eth_dev = virtio_user_eth_dev_alloc(name);
 	if (!eth_dev) {
 		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");