patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/tap: fix potential buffer overrun
@ 2019-04-25 16:47 Herakliusz Lipiec
  2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
  2019-04-29 17:31 ` [dpdk-stable] [PATCH v3] " Herakliusz Lipiec
  0 siblings, 2 replies; 10+ messages in thread
From: Herakliusz Lipiec @ 2019-04-25 16:47 UTC (permalink / raw)
  To: Keith Wiles; +Cc: dev, Herakliusz Lipiec, rasland, stable

When secondary to primary process synchronization occours
there is no check for number of fds which could cause buffer overrun.

Bugzilla ID: 252
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..b985b1c84 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
 
 	/* Attach the queues from received file descriptors */
+	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds){
+		TAP_LOG(ERR, "Unexpected number of fds received");
+		return -1;
+	}
 	dev->data->nb_rx_queues = reply_param->rxq_count;
 	dev->data->nb_tx_queues = reply_param->txq_count;
 	fd_iterator = 0;
@@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
 	/* Fill file descriptors for all queues */
 	reply.num_fds = 0;
 	reply_param->rxq_count = 0;
+	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
+			RTE_MP_MAX_FD_NUM){
+		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
+		return -1;
+	}
 	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
 		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
 		reply_param->rxq_count++;
 	}
 	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
 	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
 
 	reply_param->txq_count = 0;
@@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
 		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
 		reply_param->txq_count++;
 	}
-
+	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
 	/* Send reply */
 	strlcpy(reply.name, request->name, sizeof(reply.name));
 	strlcpy(reply_param->port_name, request_param->port_name,
-- 
2.17.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-stable] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-25 16:47 [dpdk-stable] [PATCH] net/tap: fix potential buffer overrun Herakliusz Lipiec
@ 2019-04-25 17:17 ` Herakliusz Lipiec
  2019-04-29 13:32   ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
                     ` (2 more replies)
  2019-04-29 17:31 ` [dpdk-stable] [PATCH v3] " Herakliusz Lipiec
  1 sibling, 3 replies; 10+ messages in thread
From: Herakliusz Lipiec @ 2019-04-25 17:17 UTC (permalink / raw)
  To: Keith Wiles; +Cc: dev, Herakliusz Lipiec, rasland, stable

When secondary to primary process synchronization occours
there is no check for number of fds which could cause buffer overrun.

Bugzilla ID: 252
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..4a2ef5ce7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
 
 	/* Attach the queues from received file descriptors */
+	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
+		TAP_LOG(ERR, "Unexpected number of fds received");
+		return -1;
+	}
 	dev->data->nb_rx_queues = reply_param->rxq_count;
 	dev->data->nb_tx_queues = reply_param->txq_count;
 	fd_iterator = 0;
@@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
 	/* Fill file descriptors for all queues */
 	reply.num_fds = 0;
 	reply_param->rxq_count = 0;
+	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
+			RTE_MP_MAX_FD_NUM){
+		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
+		return -1;
+	}
 	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
 		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
 		reply_param->rxq_count++;
 	}
 	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
 	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
 
 	reply_param->txq_count = 0;
@@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
 		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
 		reply_param->txq_count++;
 	}
-
+	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
+	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
 	/* Send reply */
 	strlcpy(reply.name, request->name, sizeof(reply.name));
 	strlcpy(reply_param->port_name, request_param->port_name,
-- 
2.17.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
@ 2019-04-29 13:32   ` Burakov, Anatoly
  2019-04-29 13:53   ` Ferruh Yigit
  2019-04-29 13:58   ` [dpdk-stable] " Wiles, Keith
  2 siblings, 0 replies; 10+ messages in thread
From: Burakov, Anatoly @ 2019-04-29 13:32 UTC (permalink / raw)
  To: Herakliusz Lipiec, Keith Wiles; +Cc: dev, rasland, stable

On 25-Apr-19 6:17 PM, Herakliusz Lipiec wrote:
> When secondary to primary process synchronization occours
> there is no check for number of fds which could cause buffer overrun.
> 
> Bugzilla ID: 252
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index e9fda8cf6..4a2ef5ce7 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>   	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
>   
>   	/* Attach the queues from received file descriptors */
> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> +		TAP_LOG(ERR, "Unexpected number of fds received");
> +		return -1;
> +	}
>   	dev->data->nb_rx_queues = reply_param->rxq_count;
>   	dev->data->nb_tx_queues = reply_param->txq_count;
>   	fd_iterator = 0;
> @@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>   	/* Fill file descriptors for all queues */
>   	reply.num_fds = 0;
>   	reply_param->rxq_count = 0;
> +	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> +			RTE_MP_MAX_FD_NUM){
> +		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> +		return -1;
> +	}
>   	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
>   		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
>   		reply_param->rxq_count++;
>   	}
>   	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> -	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>   	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
>   
>   	reply_param->txq_count = 0;
> @@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>   		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
>   		reply_param->txq_count++;
>   	}
> -
> +	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
>   	/* Send reply */
>   	strlcpy(reply.name, request->name, sizeof(reply.name));
>   	strlcpy(reply_param->port_name, request_param->port_name,
> 

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
  2019-04-29 13:32   ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
@ 2019-04-29 13:53   ` Ferruh Yigit
  2019-04-29 14:02     ` Burakov, Anatoly
  2019-04-29 13:58   ` [dpdk-stable] " Wiles, Keith
  2 siblings, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2019-04-29 13:53 UTC (permalink / raw)
  To: Herakliusz Lipiec, Keith Wiles; +Cc: dev, rasland, stable, Anatoly Burakov

On 4/25/2019 6:17 PM, Herakliusz Lipiec wrote:
> When secondary to primary process synchronization occours
> there is no check for number of fds which could cause buffer overrun.
> 
> Bugzilla ID: 252
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index e9fda8cf6..4a2ef5ce7 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>  	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
>  
>  	/* Attach the queues from received file descriptors */
> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> +		TAP_LOG(ERR, "Unexpected number of fds received");
> +		return -1;
> +	}

Is there a way this can happen? If not I suggest remove the check.

>  	dev->data->nb_rx_queues = reply_param->rxq_count;
>  	dev->data->nb_tx_queues = reply_param->txq_count;
>  	fd_iterator = 0;
> @@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>  	/* Fill file descriptors for all queues */
>  	reply.num_fds = 0;
>  	reply_param->rxq_count = 0;
> +	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> +			RTE_MP_MAX_FD_NUM){
> +		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> +		return -1;
> +	}

+1 for the check.
But what it does when return "-1", not send a message at all? If so would it be
better to send and error message back instead of waiting the receiver to timeout?

>  	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
>  		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
>  		reply_param->rxq_count++;
>  	}
>  	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> -	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>  	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);

Since there is dynamic check above for "RTE_MP_MAX_FD_NUM", we can remove this
assert I think.

>  
>  	reply_param->txq_count = 0;
> @@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>  		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
>  		reply_param->txq_count++;
>  	}
> -
> +	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);

Same for this assert, we can remove it.
And as syntax, please keep the empty line before next block.

>  	/* Send reply */
>  	strlcpy(reply.name, request->name, sizeof(reply.name));
>  	strlcpy(reply_param->port_name, request_param->port_name,
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
  2019-04-29 13:32   ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
  2019-04-29 13:53   ` Ferruh Yigit
@ 2019-04-29 13:58   ` Wiles, Keith
  2019-04-29 14:05     ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
  2 siblings, 1 reply; 10+ messages in thread
From: Wiles, Keith @ 2019-04-29 13:58 UTC (permalink / raw)
  To: Lipiec, Herakliusz; +Cc: dpdk-dev, rasland, stable



> On Apr 25, 2019, at 10:17 AM, Lipiec, Herakliusz <herakliusz.lipiec@intel.com> wrote:
> 
> When secondary to primary process synchronization occours
> there is no check for number of fds which could cause buffer overrun.
> 
> Bugzilla ID: 252
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index e9fda8cf6..4a2ef5ce7 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> 	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
> 
> 	/* Attach the queues from received file descriptors */
> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
> +		TAP_LOG(ERR, "Unexpected number of fds received");
> +		return -1;
> +	}

This check is reasonable, but why is this being done on the receive side and not checked on the send side. There may need to be a check for num_fds being zero or greater than 8 as that is the limit to the number of FDs that can be handle by the IPC.

In a different thread for Bug-258 we need to return an indicator that the receive side detected an error by returning 0 for num_fds and I have patch for that one.
https://bugs.dpdk.org/show_bug.cgi?id=258

I would have expected the sender to make sure they match and then this test is not needed, but a test for num_fds being zero or > 8 is needed if you want to detect the failure here or not if you do not care as long as nb_[r/t]x_queues is zero too.

> 	dev->data->nb_rx_queues = reply_param->rxq_count;
> 	dev->data->nb_tx_queues = reply_param->txq_count;
> 	fd_iterator = 0;
> @@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> 	/* Fill file descriptors for all queues */
> 	reply.num_fds = 0;
> 	reply_param->rxq_count = 0;
> +	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
> +			RTE_MP_MAX_FD_NUM){
> +		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
> +		return -1;
> +	}
> 	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
> 		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
> 		reply_param->rxq_count++;
> 	}
> 	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
> -	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> 	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
> 
> 	reply_param->txq_count = 0;
> @@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
> 		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
> 		reply_param->txq_count++;
> 	}
> -
> +	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
> 	/* Send reply */
> 	strlcpy(reply.name, request->name, sizeof(reply.name));
> 	strlcpy(reply_param->port_name, request_param->port_name,
> -- 
> 2.17.2
> 

Regards,
Keith


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-29 13:53   ` Ferruh Yigit
@ 2019-04-29 14:02     ` Burakov, Anatoly
  2019-04-30 10:42       ` Ferruh Yigit
  0 siblings, 1 reply; 10+ messages in thread
From: Burakov, Anatoly @ 2019-04-29 14:02 UTC (permalink / raw)
  To: Ferruh Yigit, Herakliusz Lipiec, Keith Wiles; +Cc: dev, rasland, stable

On 29-Apr-19 2:53 PM, Ferruh Yigit wrote:
> On 4/25/2019 6:17 PM, Herakliusz Lipiec wrote:
>> When secondary to primary process synchronization occours
>> there is no check for number of fds which could cause buffer overrun.
>>
>> Bugzilla ID: 252
>> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
>> Cc: rasland@mellanox.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
>> ---
>>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index e9fda8cf6..4a2ef5ce7 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>>   	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
>>   
>>   	/* Attach the queues from received file descriptors */
>> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
>> +		TAP_LOG(ERR, "Unexpected number of fds received");
>> +		return -1;
>> +	}
> 
> Is there a way this can happen? If not I suggest remove the check.

Normally no, but theoretically this can trigger a buffer overrun if not 
checked. After all, something could either fail on the other side, or 
someone could send a fake message :) This data is coming from an 
external source, so we need to sanity-check it.

> 
>>   	dev->data->nb_rx_queues = reply_param->rxq_count;
>>   	dev->data->nb_tx_queues = reply_param->txq_count;
>>   	fd_iterator = 0;
>> @@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>>   	/* Fill file descriptors for all queues */
>>   	reply.num_fds = 0;
>>   	reply_param->rxq_count = 0;
>> +	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
>> +			RTE_MP_MAX_FD_NUM){
>> +		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
>> +		return -1;
>> +	}
> 
> +1 for the check.
> But what it does when return "-1", not send a message at all? If so would it be
> better to send and error message back instead of waiting the receiver to timeout?

There will be a different patch fixing this specific issue. Probably 
this patch would need to be rebased on top of that.

> 
>>   	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
>>   		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
>>   		reply_param->rxq_count++;
>>   	}
>>   	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
>> -	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>>   	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
> 
> Since there is dynamic check above for "RTE_MP_MAX_FD_NUM", we can remove this
> assert I think.
> 
>>   
>>   	reply_param->txq_count = 0;
>> @@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>>   		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
>>   		reply_param->txq_count++;
>>   	}
>> -
>> +	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
> 
> Same for this assert, we can remove it.
> And as syntax, please keep the empty line before next block.
> 
>>   	/* Send reply */
>>   	strlcpy(reply.name, request->name, sizeof(reply.name));
>>   	strlcpy(reply_param->port_name, request_param->port_name,
>>
> 
> 


-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-29 13:58   ` [dpdk-stable] " Wiles, Keith
@ 2019-04-29 14:05     ` Burakov, Anatoly
  0 siblings, 0 replies; 10+ messages in thread
From: Burakov, Anatoly @ 2019-04-29 14:05 UTC (permalink / raw)
  To: Wiles, Keith, Lipiec, Herakliusz; +Cc: dpdk-dev, rasland, stable

On 29-Apr-19 2:58 PM, Wiles, Keith wrote:
> 
> 
>> On Apr 25, 2019, at 10:17 AM, Lipiec, Herakliusz <herakliusz.lipiec@intel.com> wrote:
>>
>> When secondary to primary process synchronization occours
>> there is no check for number of fds which could cause buffer overrun.
>>
>> Bugzilla ID: 252
>> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
>> Cc: rasland@mellanox.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index e9fda8cf6..4a2ef5ce7 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>> 	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
>>
>> 	/* Attach the queues from received file descriptors */
>> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
>> +		TAP_LOG(ERR, "Unexpected number of fds received");
>> +		return -1;
>> +	}
> 
> This check is reasonable, but why is this being done on the receive side and not checked on the send side. There may need to be a check for num_fds being zero or greater than 8 as that is the limit to the number of FDs that can be handle by the IPC.

It is done below on the send side as well. This check is for 
sanity-checking external input. It's a socket, so anything (with 
matching UID) can write to it.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-stable] [PATCH v3] net/tap: fix potential buffer overrun
  2019-04-25 16:47 [dpdk-stable] [PATCH] net/tap: fix potential buffer overrun Herakliusz Lipiec
  2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
@ 2019-04-29 17:31 ` Herakliusz Lipiec
  2019-05-02 16:31   ` Ferruh Yigit
  1 sibling, 1 reply; 10+ messages in thread
From: Herakliusz Lipiec @ 2019-04-29 17:31 UTC (permalink / raw)
  To: Keith Wiles; +Cc: dev, Herakliusz Lipiec, rasland, stable

When secondary to primary process synchronization occours
there is no check for number of fds which could cause buffer overrun.

Bugzilla ID: 252
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..780368bac 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2111,6 +2111,11 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
 
 	/* Attach the queues from received file descriptors */
+	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
+		TAP_LOG(ERR, "Unexpected number of fds received");
+		return -1;
+	}
+
 	dev->data->nb_rx_queues = reply_param->rxq_count;
 	dev->data->nb_tx_queues = reply_param->txq_count;
 	fd_iterator = 0;
@@ -2151,19 +2156,24 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
 	/* Fill file descriptors for all queues */
 	reply.num_fds = 0;
 	reply_param->rxq_count = 0;
+	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
+			RTE_MP_MAX_FD_NUM){
+		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
+		return -1;
+	}
+
 	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
 		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
 		reply_param->rxq_count++;
 	}
 	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
-	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
-	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
 
 	reply_param->txq_count = 0;
 	for (queue = 0; queue < dev->data->nb_tx_queues; queue++) {
 		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
 		reply_param->txq_count++;
 	}
+	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
 
 	/* Send reply */
 	strlcpy(reply.name, request->name, sizeof(reply.name));
-- 
2.17.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/tap: fix potential buffer overrun
  2019-04-29 14:02     ` Burakov, Anatoly
@ 2019-04-30 10:42       ` Ferruh Yigit
  0 siblings, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2019-04-30 10:42 UTC (permalink / raw)
  To: Burakov, Anatoly, Herakliusz Lipiec, Keith Wiles; +Cc: dev, rasland, stable

On 4/29/2019 3:02 PM, Burakov, Anatoly wrote:
> On 29-Apr-19 2:53 PM, Ferruh Yigit wrote:
>> On 4/25/2019 6:17 PM, Herakliusz Lipiec wrote:
>>> When secondary to primary process synchronization occours
>>> there is no check for number of fds which could cause buffer overrun.
>>>
>>> Bugzilla ID: 252
>>> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
>>> Cc: rasland@mellanox.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
>>> ---
>>>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index e9fda8cf6..4a2ef5ce7 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -2111,6 +2111,10 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>>>   	TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name);
>>>   
>>>   	/* Attach the queues from received file descriptors */
>>> +	if (reply_param->rxq_count + reply_param->txq_count != reply->num_fds) {
>>> +		TAP_LOG(ERR, "Unexpected number of fds received");
>>> +		return -1;
>>> +	}
>>
>> Is there a way this can happen? If not I suggest remove the check.
> 
> Normally no, but theoretically this can trigger a buffer overrun if not 
> checked. After all, something could either fail on the other side, or 
> someone could send a fake message :) This data is coming from an 
> external source, so we need to sanity-check it.

Both sender and receiver are in the same driver, primary and secondary
application paths, there is no communication with external source,
and I don't see any code path that will cause this failure.

After above said, this is just an additional reasonable check and not in the
data path, so having this won't hurt, I don't object to have it.

> 
>>
>>>   	dev->data->nb_rx_queues = reply_param->rxq_count;
>>>   	dev->data->nb_tx_queues = reply_param->txq_count;
>>>   	fd_iterator = 0;
>>> @@ -2151,12 +2155,16 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>>>   	/* Fill file descriptors for all queues */
>>>   	reply.num_fds = 0;
>>>   	reply_param->rxq_count = 0;
>>> +	if (dev->data->nb_rx_queues + dev->data->nb_tx_queues >
>>> +			RTE_MP_MAX_FD_NUM){
>>> +		TAP_LOG(ERR, "Number of rx/tx queues exceeds max number of fds");
>>> +		return -1;
>>> +	}
>>
>> +1 for the check.
>> But what it does when return "-1", not send a message at all? If so would it be
>> better to send and error message back instead of waiting the receiver to timeout?
> 
> There will be a different patch fixing this specific issue. Probably 
> this patch would need to be rebased on top of that.

+1 to fix this issue but I assume it won't be for this release, so can get this
patch now and this part can be updated with mentioned patch.

> 
>>
>>>   	for (queue = 0; queue < dev->data->nb_rx_queues; queue++) {
>>>   		reply.fds[reply.num_fds++] = process_private->rxq_fds[queue];
>>>   		reply_param->rxq_count++;
>>>   	}
>>>   	RTE_ASSERT(reply_param->rxq_count == dev->data->nb_rx_queues);
>>> -	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>>>   	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
>>
>> Since there is dynamic check above for "RTE_MP_MAX_FD_NUM", we can remove this
>> assert I think.
>>
>>>   
>>>   	reply_param->txq_count = 0;
>>> @@ -2164,7 +2172,8 @@ tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer)
>>>   		reply.fds[reply.num_fds++] = process_private->txq_fds[queue];
>>>   		reply_param->txq_count++;
>>>   	}
>>> -
>>> +	RTE_ASSERT(reply_param->txq_count == dev->data->nb_tx_queues);
>>> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
>>
>> Same for this assert, we can remove it.
>> And as syntax, please keep the empty line before next block.
>>
>>>   	/* Send reply */
>>>   	strlcpy(reply.name, request->name, sizeof(reply.name));
>>>   	strlcpy(reply_param->port_name, request_param->port_name,
>>>
>>
>>
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [PATCH v3] net/tap: fix potential buffer overrun
  2019-04-29 17:31 ` [dpdk-stable] [PATCH v3] " Herakliusz Lipiec
@ 2019-05-02 16:31   ` Ferruh Yigit
  0 siblings, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2019-05-02 16:31 UTC (permalink / raw)
  To: Herakliusz Lipiec, Keith Wiles; +Cc: dev, rasland, stable

On 4/29/2019 6:31 PM, Herakliusz Lipiec wrote:
> When secondary to primary process synchronization occours
> there is no check for number of fds which could cause buffer overrun.
> 
> Bugzilla ID: 252
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>

Carrying from prev version:
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-05-02 16:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-25 16:47 [dpdk-stable] [PATCH] net/tap: fix potential buffer overrun Herakliusz Lipiec
2019-04-25 17:17 ` [dpdk-stable] [PATCH v2] " Herakliusz Lipiec
2019-04-29 13:32   ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
2019-04-29 13:53   ` Ferruh Yigit
2019-04-29 14:02     ` Burakov, Anatoly
2019-04-30 10:42       ` Ferruh Yigit
2019-04-29 13:58   ` [dpdk-stable] " Wiles, Keith
2019-04-29 14:05     ` [dpdk-stable] [dpdk-dev] " Burakov, Anatoly
2019-04-29 17:31 ` [dpdk-stable] [PATCH v3] " Herakliusz Lipiec
2019-05-02 16:31   ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).