DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
@ 2020-05-12 15:19 Wei Zhao
  2020-05-15  2:24 ` Zhao1, Wei
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Wei Zhao @ 2020-05-12 15:19 UTC (permalink / raw)
  To: dev; +Cc: stable, beilei.xing, Wei Zhao

In i40e PMD code of function i40e_res_pool_free(), if valid_entry is
freed by "rte_free(valid_entry);" in the following code:

if (prev != NULL) {
 ........................

   if (insert == 1) {
     LIST_REMOVE(valid_entry, next);
     rte_free(valid_entry);
    } else {
     rte_free(valid_entry);
     insert = 1;
    }
  }

then the following code for pool update may still use the wild pointer
"valid_entry":

" pool->num_free += valid_entry->len;
  pool->num_alloc -= valid_entry>len;
"
it seems to be a security bug, we should avoid this risk.

Cc: stable@dpdk.org
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 749d85f54..7f8ea5309 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	}
 
 	insert = 0;
+	pool->num_free += valid_entry->len;
+	pool->num_alloc -= valid_entry->len;
+
 	/* Try to merge with next one*/
 	if (next != NULL) {
 		/* Merge with next one */
@@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
 	}
 
-	pool->num_free += valid_entry->len;
-	pool->num_alloc -= valid_entry->len;
-
 	return 0;
 }
 
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
  2020-05-12 15:19 [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation Wei Zhao
@ 2020-05-15  2:24 ` Zhao1, Wei
  2020-05-15  6:32 ` Jeff Guo
  2020-05-18  5:10 ` [dpdk-dev] [PATCH v2] net/i40e: fix the core dump " Wei Zhao
  2 siblings, 0 replies; 12+ messages in thread
From: Zhao1, Wei @ 2020-05-15  2:24 UTC (permalink / raw)
  To: dev; +Cc: stable, Xing, Beilei, Guo, Jia

Can any one view for this patch?
Thanks!


> -----Original Message-----
> From: Zhao1, Wei <wei.zhao1@intel.com>
> Sent: Tuesday, May 12, 2020 11:19 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH] net/i40e: fix the security risk of wild pointer operation
> 
> In i40e PMD code of function i40e_res_pool_free(), if valid_entry is freed by
> "rte_free(valid_entry);" in the following code:
> 
> if (prev != NULL) {
>  ........................
> 
>    if (insert == 1) {
>      LIST_REMOVE(valid_entry, next);
>      rte_free(valid_entry);
>     } else {
>      rte_free(valid_entry);
>      insert = 1;
>     }
>   }
> 
> then the following code for pool update may still use the wild pointer
> "valid_entry":
> 
> " pool->num_free += valid_entry->len;
>   pool->num_alloc -= valid_entry>len;
> "
> it seems to be a security bug, we should avoid this risk.
> 
> Cc: stable@dpdk.org
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 749d85f54..7f8ea5309 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info
> *pool,
>  	}
> 
>  	insert = 0;
> +	pool->num_free += valid_entry->len;
> +	pool->num_alloc -= valid_entry->len;
> +
>  	/* Try to merge with next one*/
>  	if (next != NULL) {
>  		/* Merge with next one */
> @@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info
> *pool,
>  			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
>  	}
> 
> -	pool->num_free += valid_entry->len;
> -	pool->num_alloc -= valid_entry->len;
> -
>  	return 0;
>  }
> 
> --
> 2.17.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
  2020-05-12 15:19 [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation Wei Zhao
  2020-05-15  2:24 ` Zhao1, Wei
@ 2020-05-15  6:32 ` Jeff Guo
  2020-05-15  7:28   ` Ye Xiaolong
  2020-05-18  5:10 ` [dpdk-dev] [PATCH v2] net/i40e: fix the core dump " Wei Zhao
  2 siblings, 1 reply; 12+ messages in thread
From: Jeff Guo @ 2020-05-15  6:32 UTC (permalink / raw)
  To: Wei Zhao, dev; +Cc: stable, beilei.xing

hi, zhaowei

On 5/12/2020 11:19 PM, Wei Zhao wrote:
> In i40e PMD code of function i40e_res_pool_free(), if valid_entry is
> freed by "rte_free(valid_entry);" in the following code:
>
> if (prev != NULL) {
>   ........................
>
>     if (insert == 1) {
>       LIST_REMOVE(valid_entry, next);
>       rte_free(valid_entry);
>      } else {
>       rte_free(valid_entry);
>       insert = 1;
>      }
>    }
>
> then the following code for pool update may still use the wild pointer
> "valid_entry":
>
> " pool->num_free += valid_entry->len;
>    pool->num_alloc -= valid_entry>len;
> "
> it seems to be a security bug, we should avoid this risk.
>
> Cc: stable@dpdk.org
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 749d85f54..7f8ea5309 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   	}
>   
>   	insert = 0;
> +	pool->num_free += valid_entry->len;
> +	pool->num_alloc -= valid_entry->len;
> +


Shouldn't the pool count update after the pool->free_list updated by 
"LIST_INSERT_HEAD(&pool->free_list, valid_entry, next)"?

If so, you could use a variable to restore  valid_entry->len at the 
begin and use it update pool count and other place.


>   	/* Try to merge with next one*/
>   	if (next != NULL) {
>   		/* Merge with next one */
> @@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
>   	}
>   
> -	pool->num_free += valid_entry->len;
> -	pool->num_alloc -= valid_entry->len;
> -
>   	return 0;
>   }
>   

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

* Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
  2020-05-15  6:32 ` Jeff Guo
@ 2020-05-15  7:28   ` Ye Xiaolong
  2020-05-18  5:24     ` Zhao1, Wei
  0 siblings, 1 reply; 12+ messages in thread
From: Ye Xiaolong @ 2020-05-15  7:28 UTC (permalink / raw)
  To: Jeff Guo; +Cc: Wei Zhao, dev, stable, beilei.xing

On 05/15, Jeff Guo wrote:
>hi, zhaowei
>
>On 5/12/2020 11:19 PM, Wei Zhao wrote:
>> In i40e PMD code of function i40e_res_pool_free(), if valid_entry is
>> freed by "rte_free(valid_entry);" in the following code:
>> 
>> if (prev != NULL) {
>>   ........................
>> 
>>     if (insert == 1) {
>>       LIST_REMOVE(valid_entry, next);
>>       rte_free(valid_entry);
>>      } else {
>>       rte_free(valid_entry);
>>       insert = 1;
>>      }
>>    }
>> 
>> then the following code for pool update may still use the wild pointer
>> "valid_entry":
>> 
>> " pool->num_free += valid_entry->len;
>>    pool->num_alloc -= valid_entry>len;
>> "
>> it seems to be a security bug, we should avoid this risk.
>> 
>> Cc: stable@dpdk.org
>> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>> 
>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>> ---
>>   drivers/net/i40e/i40e_ethdev.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>> index 749d85f54..7f8ea5309 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>>   	}
>>   	insert = 0;
>> +	pool->num_free += valid_entry->len;
>> +	pool->num_alloc -= valid_entry->len;
>> +
>
>
>Shouldn't the pool count update after the pool->free_list updated by
>"LIST_INSERT_HEAD(&pool->free_list, valid_entry, next)"?
>
>If so, you could use a variable to restore  valid_entry->len at the begin and
>use it update pool count and other place.

Either way works from function point of view, but I do agree with Jeff that uses 
local variable to store the valid_entry->len at the beginning, and then updates
the pool->num_free/num_alloc at the end. 

Also I think it needs to set valid_entry to NULL after free it, it can avoid
wild pointer case like this, if there is dereference of this pointer after setting
it to NULL, program would crash directly and we can solve it early.

Thanks,
Xiaolong

>
>
>>   	/* Try to merge with next one*/
>>   	if (next != NULL) {
>>   		/* Merge with next one */
>> @@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>>   			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
>>   	}
>> -	pool->num_free += valid_entry->len;
>> -	pool->num_alloc -= valid_entry->len;
>> -
>>   	return 0;
>>   }

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

* [dpdk-dev] [PATCH v2] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-12 15:19 [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation Wei Zhao
  2020-05-15  2:24 ` Zhao1, Wei
  2020-05-15  6:32 ` Jeff Guo
@ 2020-05-18  5:10 ` Wei Zhao
  2020-05-18  6:43   ` [dpdk-dev] [PATCH v3] " Wei Zhao
  2 siblings, 1 reply; 12+ messages in thread
From: Wei Zhao @ 2020-05-18  5:10 UTC (permalink / raw)
  To: dev; +Cc: stable, xiaolong.ye, jia.guo, Wei Zhao

In i40e PMD code of function i40e_res_pool_free(), if valid_entry
is freed by "rte_free(valid_entry);" in the code, then the following
code for pool update may still use the wild pointer "valid_entry"
for pool info update. It seems has the risk of core dump for
using wild pointer operation, we should avoid this risk.

Cc: stable@dpdk.org
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
update commit log
---
 drivers/net/i40e/i40e_ethdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 91dcd0ebf..09c2efef8 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	}
 
 	insert = 0;
+	pool->num_free += valid_entry->len;
+	pool->num_alloc -= valid_entry->len;
+
 	/* Try to merge with next one*/
 	if (next != NULL) {
 		/* Merge with next one */
@@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
 	}
 
-	pool->num_free += valid_entry->len;
-	pool->num_alloc -= valid_entry->len;
-
 	return 0;
 }
 
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
  2020-05-15  7:28   ` Ye Xiaolong
@ 2020-05-18  5:24     ` Zhao1, Wei
  2020-05-18  5:32       ` Ye Xiaolong
  0 siblings, 1 reply; 12+ messages in thread
From: Zhao1, Wei @ 2020-05-18  5:24 UTC (permalink / raw)
  To: Ye, Xiaolong, Guo, Jia; +Cc: dev, stable, Xing, Beilei

HI, Xiaolong & guojia

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Friday, May 15, 2020 3:28 PM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org; stable@dpdk.org;
> Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer
> operation
> 
> On 05/15, Jeff Guo wrote:
> >hi, zhaowei
> >
> >On 5/12/2020 11:19 PM, Wei Zhao wrote:
> >> In i40e PMD code of function i40e_res_pool_free(), if valid_entry is
> >> freed by "rte_free(valid_entry);" in the following code:
> >>
> >> if (prev != NULL) {
> >>   ........................
> >>
> >>     if (insert == 1) {
> >>       LIST_REMOVE(valid_entry, next);
> >>       rte_free(valid_entry);
> >>      } else {
> >>       rte_free(valid_entry);
> >>       insert = 1;
> >>      }
> >>    }
> >>
> >> then the following code for pool update may still use the wild
> >> pointer
> >> "valid_entry":
> >>
> >> " pool->num_free += valid_entry->len;
> >>    pool->num_alloc -= valid_entry>len; "
> >> it seems to be a security bug, we should avoid this risk.
> >>
> >> Cc: stable@dpdk.org
> >> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> >>
> >> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >> ---
> >>   drivers/net/i40e/i40e_ethdev.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/i40e/i40e_ethdev.c
> >> b/drivers/net/i40e/i40e_ethdev.c index 749d85f54..7f8ea5309 100644
> >> --- a/drivers/net/i40e/i40e_ethdev.c
> >> +++ b/drivers/net/i40e/i40e_ethdev.c
> >> @@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info
> *pool,
> >>   	}
> >>   	insert = 0;
> >> +	pool->num_free += valid_entry->len;
> >> +	pool->num_alloc -= valid_entry->len;
> >> +
> >
> >
> >Shouldn't the pool count update after the pool->free_list updated by
> >"LIST_INSERT_HEAD(&pool->free_list, valid_entry, next)"?
> >
> >If so, you could use a variable to restore  valid_entry->len at the
> >begin and use it update pool count and other place.
> 
> Either way works from function point of view, but I do agree with Jeff that uses
> local variable to store the valid_entry->len at the beginning, and then updates
> the pool->num_free/num_alloc at the end.
> 
> Also I think it needs to set valid_entry to NULL after free it, it can avoid wild
> pointer case like this, if there is dereference of this pointer after setting it to
> NULL, program would crash directly and we can solve it early.
> 
> Thanks,
> Xiaolong

We must update it after find the proper one in the pool->free_list at once,  if we use a local pointer to store it,
The proper entry may has been freed in the following code, and merge with other free resource prev or next.


> 
> >
> >
> >>   	/* Try to merge with next one*/
> >>   	if (next != NULL) {
> >>   		/* Merge with next one */
> >> @@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info
> *pool,
> >>   			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
> >>   	}
> >> -	pool->num_free += valid_entry->len;
> >> -	pool->num_alloc -= valid_entry->len;
> >> -
> >>   	return 0;
> >>   }

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

* Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation
  2020-05-18  5:24     ` Zhao1, Wei
@ 2020-05-18  5:32       ` Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2020-05-18  5:32 UTC (permalink / raw)
  To: Zhao1, Wei; +Cc: Guo, Jia, dev, stable, Xing, Beilei

Hi, Wei

On 05/18, Zhao1, Wei wrote:
>HI, Xiaolong & guojia
>
>> -----Original Message-----
>> From: Ye, Xiaolong <xiaolong.ye@intel.com>
>> Sent: Friday, May 15, 2020 3:28 PM
>> To: Guo, Jia <jia.guo@intel.com>
>> Cc: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org; stable@dpdk.org;
>> Xing, Beilei <beilei.xing@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer
>> operation
>>
>> On 05/15, Jeff Guo wrote:
>> >hi, zhaowei
>> >
>> >On 5/12/2020 11:19 PM, Wei Zhao wrote:
>> >> In i40e PMD code of function i40e_res_pool_free(), if valid_entry is
>> >> freed by "rte_free(valid_entry);" in the following code:
>> >>
>> >> if (prev != NULL) {
>> >>   ........................
>> >>
>> >>     if (insert == 1) {
>> >>       LIST_REMOVE(valid_entry, next);
>> >>       rte_free(valid_entry);
>> >>      } else {
>> >>       rte_free(valid_entry);
>> >>       insert = 1;
>> >>      }
>> >>    }
>> >>
>> >> then the following code for pool update may still use the wild
>> >> pointer
>> >> "valid_entry":
>> >>
>> >> " pool->num_free += valid_entry->len;
>> >>    pool->num_alloc -= valid_entry>len; "
>> >> it seems to be a security bug, we should avoid this risk.
>> >>
>> >> Cc: stable@dpdk.org
>> >> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>> >>
>> >> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>> >> ---
>> >>   drivers/net/i40e/i40e_ethdev.c | 6 +++---
>> >>   1 file changed, 3 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/net/i40e/i40e_ethdev.c
>> >> b/drivers/net/i40e/i40e_ethdev.c index 749d85f54..7f8ea5309 100644
>> >> --- a/drivers/net/i40e/i40e_ethdev.c
>> >> +++ b/drivers/net/i40e/i40e_ethdev.c
>> >> @@ -4973,6 +4973,9 @@ i40e_res_pool_free(struct i40e_res_pool_info
>> *pool,
>> >>   }
>> >>   insert = 0;
>> >> +pool->num_free += valid_entry->len;
>> >> +pool->num_alloc -= valid_entry->len;
>> >> +
>> >
>> >
>> >Shouldn't the pool count update after the pool->free_list updated by
>> >"LIST_INSERT_HEAD(&pool->free_list, valid_entry, next)"?
>> >
>> >If so, you could use a variable to restore  valid_entry->len at the
>> >begin and use it update pool count and other place.
>>
>> Either way works from function point of view, but I do agree with Jeff that uses
>> local variable to store the valid_entry->len at the beginning, and then updates
>> the pool->num_free/num_alloc at the end.
>>
>> Also I think it needs to set valid_entry to NULL after free it, it can avoid wild
>> pointer case like this, if there is dereference of this pointer after setting it to
>> NULL, program would crash directly and we can solve it early.
>>
>> Thanks,
>> Xiaolong
>
>We must update it after find the proper one in the pool->free_list at once,  if we use a local pointer to store it,
>The proper entry may has been freed in the following code, and merge with other free resource prev or next.

I think Jia's point is to store the valid_entry->len to a local var, not use
a local pointer to store valid_entry.
And please set valid_entry to NULL after free in the new version.

Thanks,
Xiaolong

>
>
>>
>> >
>> >
>> >>   /* Try to merge with next one*/
>> >>   if (next != NULL) {
>> >>   /* Merge with next one */
>> >> @@ -5010,9 +5013,6 @@ i40e_res_pool_free(struct i40e_res_pool_info
>> *pool,
>> >>   LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
>> >>   }
>> >> -pool->num_free += valid_entry->len;
>> >> -pool->num_alloc -= valid_entry->len;
>> >> -
>> >>   return 0;
>> >>   }

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

* [dpdk-dev] [PATCH v3] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-18  5:10 ` [dpdk-dev] [PATCH v2] net/i40e: fix the core dump " Wei Zhao
@ 2020-05-18  6:43   ` Wei Zhao
  2020-05-18  7:43     ` [dpdk-dev] [PATCH v4] " Wei Zhao
  0 siblings, 1 reply; 12+ messages in thread
From: Wei Zhao @ 2020-05-18  6:43 UTC (permalink / raw)
  To: dev; +Cc: stable, xiaolong.ye, jia.guo, Wei Zhao

In i40e PMD code of function i40e_res_pool_free(), if valid_entry
is freed by "rte_free(valid_entry);" in the code, then the following
code for pool update may still use the wild pointer "valid_entry"
for pool info update. It seems has the risk of core dump for
using wild pointer operation, we should avoid this risk.

Cc: stable@dpdk.org
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
update commit log

v3:
set free pointer to NULL
---
 drivers/net/i40e/i40e_ethdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 749d85f54..c9601d683 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -4936,6 +4936,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	struct pool_entry *entry, *next, *prev, *valid_entry = NULL;
 	uint32_t pool_offset;
 	int insert;
+	uint16_t len;
 
 	if (pool == NULL) {
 		PMD_DRV_LOG(ERR, "Invalid parameter");
@@ -4973,6 +4974,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	}
 
 	insert = 0;
+	len = valid_entry->len;
 	/* Try to merge with next one*/
 	if (next != NULL) {
 		/* Merge with next one */
@@ -4993,8 +4995,10 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			if (insert == 1) {
 				LIST_REMOVE(valid_entry, next);
 				rte_free(valid_entry);
+				valid_entry = NULL;
 			} else {
 				rte_free(valid_entry);
+				valid_entry = NULL;
 				insert = 1;
 			}
 		}
@@ -5010,8 +5014,8 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
 	}
 
-	pool->num_free += valid_entry->len;
-	pool->num_alloc -= valid_entry->len;
+	pool->num_free += len;
+	pool->num_alloc -= len;
 
 	return 0;
 }
-- 
2.19.1


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

* [dpdk-dev] [PATCH v4] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-18  6:43   ` [dpdk-dev] [PATCH v3] " Wei Zhao
@ 2020-05-18  7:43     ` Wei Zhao
  2020-05-18  8:00       ` [dpdk-dev] [PATCH v5] " Wei Zhao
  0 siblings, 1 reply; 12+ messages in thread
From: Wei Zhao @ 2020-05-18  7:43 UTC (permalink / raw)
  To: dev; +Cc: stable, xiaolong.ye, jia.guo, Wei Zhao

In i40e PMD code of function i40e_res_pool_free(), if valid_entry
is freed by "rte_free(valid_entry);" in the code, then the following
code for pool update may still use the wild pointer "valid_entry"
for pool info update. It seems has the risk of core dump for
using wild pointer operation, we should avoid this risk.

Cc: stable@dpdk.org
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
update commit log

v3:
set free pointer to NULL

v4:
change code style
---
 drivers/net/i40e/i40e_ethdev.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 749d85f54..679d59e5d 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -4935,6 +4935,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 {
 	struct pool_entry *entry, *next, *prev, *valid_entry = NULL;
 	uint32_t pool_offset;
+	uint16_t len;
 	int insert;
 
 	if (pool == NULL) {
@@ -4973,12 +4974,13 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	}
 
 	insert = 0;
+	len = valid_entry->len;
 	/* Try to merge with next one*/
 	if (next != NULL) {
 		/* Merge with next one */
-		if (valid_entry->base + valid_entry->len == next->base) {
+		if (valid_entry->base + len == next->base) {
 			next->base = valid_entry->base;
-			next->len += valid_entry->len;
+			next->len += len;
 			rte_free(valid_entry);
 			valid_entry = next;
 			insert = 1;
@@ -4987,14 +4989,16 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 
 	if (prev != NULL) {
 		/* Merge with previous one */
-		if (prev->base + prev->len == valid_entry->base) {
-			prev->len += valid_entry->len;
+		if (prev->base + len == valid_entry->base) {
+			prev->len += len;
 			/* If it merge with next one, remove next node */
 			if (insert == 1) {
 				LIST_REMOVE(valid_entry, next);
 				rte_free(valid_entry);
+				valid_entry = NULL;
 			} else {
 				rte_free(valid_entry);
+				valid_entry = NULL;
 				insert = 1;
 			}
 		}
@@ -5010,8 +5014,8 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
 	}
 
-	pool->num_free += valid_entry->len;
-	pool->num_alloc -= valid_entry->len;
+	pool->num_free += len;
+	pool->num_alloc -= len;
 
 	return 0;
 }
-- 
2.19.1


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

* [dpdk-dev] [PATCH v5] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-18  7:43     ` [dpdk-dev] [PATCH v4] " Wei Zhao
@ 2020-05-18  8:00       ` Wei Zhao
  2020-05-18  8:45         ` Jeff Guo
  2020-05-19  1:28         ` Ye Xiaolong
  0 siblings, 2 replies; 12+ messages in thread
From: Wei Zhao @ 2020-05-18  8:00 UTC (permalink / raw)
  To: dev; +Cc: stable, xiaolong.ye, jia.guo, Wei Zhao

In i40e PMD code of function i40e_res_pool_free(), if valid_entry
is freed by "rte_free(valid_entry);" in the code, then the following
code for pool update may still use the wild pointer "valid_entry"
for pool info update. It seems has the risk of core dump for
using wild pointer operation, we should avoid this risk.

Cc: stable@dpdk.org
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
update commit log

v3:
set free pointer to NULL

v4:
change code style

v5:
fix an issue in v4
---
 drivers/net/i40e/i40e_ethdev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 749d85f54..00bb05179 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -4935,6 +4935,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 {
 	struct pool_entry *entry, *next, *prev, *valid_entry = NULL;
 	uint32_t pool_offset;
+	uint16_t len;
 	int insert;
 
 	if (pool == NULL) {
@@ -4973,12 +4974,13 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	}
 
 	insert = 0;
+	len = valid_entry->len;
 	/* Try to merge with next one*/
 	if (next != NULL) {
 		/* Merge with next one */
-		if (valid_entry->base + valid_entry->len == next->base) {
+		if (valid_entry->base + len == next->base) {
 			next->base = valid_entry->base;
-			next->len += valid_entry->len;
+			next->len += len;
 			rte_free(valid_entry);
 			valid_entry = next;
 			insert = 1;
@@ -4988,13 +4990,15 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 	if (prev != NULL) {
 		/* Merge with previous one */
 		if (prev->base + prev->len == valid_entry->base) {
-			prev->len += valid_entry->len;
+			prev->len += len;
 			/* If it merge with next one, remove next node */
 			if (insert == 1) {
 				LIST_REMOVE(valid_entry, next);
 				rte_free(valid_entry);
+				valid_entry = NULL;
 			} else {
 				rte_free(valid_entry);
+				valid_entry = NULL;
 				insert = 1;
 			}
 		}
@@ -5010,8 +5014,8 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
 	}
 
-	pool->num_free += valid_entry->len;
-	pool->num_alloc -= valid_entry->len;
+	pool->num_free += len;
+	pool->num_alloc -= len;
 
 	return 0;
 }
-- 
2.19.1


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

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-18  8:00       ` [dpdk-dev] [PATCH v5] " Wei Zhao
@ 2020-05-18  8:45         ` Jeff Guo
  2020-05-19  1:28         ` Ye Xiaolong
  1 sibling, 0 replies; 12+ messages in thread
From: Jeff Guo @ 2020-05-18  8:45 UTC (permalink / raw)
  To: Wei Zhao, dev; +Cc: stable, xiaolong.ye


On 5/18/2020 4:00 PM, Wei Zhao wrote:
> In i40e PMD code of function i40e_res_pool_free(), if valid_entry
> is freed by "rte_free(valid_entry);" in the code, then the following
> code for pool update may still use the wild pointer "valid_entry"
> for pool info update. It seems has the risk of core dump for
> using wild pointer operation, we should avoid this risk.
>
> Cc: stable@dpdk.org
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>
> ---
>
> v2:
> update commit log
>
> v3:
> set free pointer to NULL
>
> v4:
> change code style
>
> v5:
> fix an issue in v4
> ---
>   drivers/net/i40e/i40e_ethdev.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 749d85f54..00bb05179 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -4935,6 +4935,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   {
>   	struct pool_entry *entry, *next, *prev, *valid_entry = NULL;
>   	uint32_t pool_offset;
> +	uint16_t len;
>   	int insert;
>   
>   	if (pool == NULL) {
> @@ -4973,12 +4974,13 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   	}
>   
>   	insert = 0;
> +	len = valid_entry->len;
>   	/* Try to merge with next one*/
>   	if (next != NULL) {
>   		/* Merge with next one */
> -		if (valid_entry->base + valid_entry->len == next->base) {
> +		if (valid_entry->base + len == next->base) {
>   			next->base = valid_entry->base;
> -			next->len += valid_entry->len;
> +			next->len += len;
>   			rte_free(valid_entry);
>   			valid_entry = next;
>   			insert = 1;
> @@ -4988,13 +4990,15 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   	if (prev != NULL) {
>   		/* Merge with previous one */
>   		if (prev->base + prev->len == valid_entry->base) {
> -			prev->len += valid_entry->len;
> +			prev->len += len;
>   			/* If it merge with next one, remove next node */
>   			if (insert == 1) {
>   				LIST_REMOVE(valid_entry, next);
>   				rte_free(valid_entry);
> +				valid_entry = NULL;
>   			} else {
>   				rte_free(valid_entry);
> +				valid_entry = NULL;
>   				insert = 1;
>   			}
>   		}
> @@ -5010,8 +5014,8 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
>   			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
>   	}
>   
> -	pool->num_free += valid_entry->len;
> -	pool->num_alloc -= valid_entry->len;
> +	pool->num_free += len;
> +	pool->num_alloc -= len;
>   
>   	return 0;
>   }
Reviewed-by: Jeff Guo <jia.guo@intel.com>

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

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix the core dump risk of wild pointer operation
  2020-05-18  8:00       ` [dpdk-dev] [PATCH v5] " Wei Zhao
  2020-05-18  8:45         ` Jeff Guo
@ 2020-05-19  1:28         ` Ye Xiaolong
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2020-05-19  1:28 UTC (permalink / raw)
  To: Wei Zhao; +Cc: dev, stable, jia.guo

On 05/18, Wei Zhao wrote:
>In i40e PMD code of function i40e_res_pool_free(), if valid_entry
>is freed by "rte_free(valid_entry);" in the code, then the following
>code for pool update may still use the wild pointer "valid_entry"
>for pool info update. It seems has the risk of core dump for
>using wild pointer operation, we should avoid this risk.
>
>Cc: stable@dpdk.org
>Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
>Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>
>---
>
>v2:
>update commit log
>
>v3:
>set free pointer to NULL
>
>v4:
>change code style
>
>v5:
>fix an issue in v4
>---
> drivers/net/i40e/i40e_ethdev.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>index 749d85f54..00bb05179 100644
>--- a/drivers/net/i40e/i40e_ethdev.c
>+++ b/drivers/net/i40e/i40e_ethdev.c
>@@ -4935,6 +4935,7 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
> {
> 	struct pool_entry *entry, *next, *prev, *valid_entry = NULL;
> 	uint32_t pool_offset;
>+	uint16_t len;
> 	int insert;
> 
> 	if (pool == NULL) {
>@@ -4973,12 +4974,13 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
> 	}
> 
> 	insert = 0;
>+	len = valid_entry->len;
> 	/* Try to merge with next one*/
> 	if (next != NULL) {
> 		/* Merge with next one */
>-		if (valid_entry->base + valid_entry->len == next->base) {
>+		if (valid_entry->base + len == next->base) {
> 			next->base = valid_entry->base;
>-			next->len += valid_entry->len;
>+			next->len += len;
> 			rte_free(valid_entry);
> 			valid_entry = next;
> 			insert = 1;
>@@ -4988,13 +4990,15 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
> 	if (prev != NULL) {
> 		/* Merge with previous one */
> 		if (prev->base + prev->len == valid_entry->base) {
>-			prev->len += valid_entry->len;
>+			prev->len += len;
> 			/* If it merge with next one, remove next node */
> 			if (insert == 1) {
> 				LIST_REMOVE(valid_entry, next);
> 				rte_free(valid_entry);
>+				valid_entry = NULL;
> 			} else {
> 				rte_free(valid_entry);
>+				valid_entry = NULL;
> 				insert = 1;
> 			}
> 		}
>@@ -5010,8 +5014,8 @@ i40e_res_pool_free(struct i40e_res_pool_info *pool,
> 			LIST_INSERT_HEAD(&pool->free_list, valid_entry, next);
> 	}
> 
>-	pool->num_free += valid_entry->len;
>-	pool->num_alloc -= valid_entry->len;
>+	pool->num_free += len;
>+	pool->num_alloc -= len;
> 
> 	return 0;
> }
>-- 
>2.19.1
>

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel, Thanks.

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

end of thread, other threads:[~2020-05-19  1:37 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 15:19 [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation Wei Zhao
2020-05-15  2:24 ` Zhao1, Wei
2020-05-15  6:32 ` Jeff Guo
2020-05-15  7:28   ` Ye Xiaolong
2020-05-18  5:24     ` Zhao1, Wei
2020-05-18  5:32       ` Ye Xiaolong
2020-05-18  5:10 ` [dpdk-dev] [PATCH v2] net/i40e: fix the core dump " Wei Zhao
2020-05-18  6:43   ` [dpdk-dev] [PATCH v3] " Wei Zhao
2020-05-18  7:43     ` [dpdk-dev] [PATCH v4] " Wei Zhao
2020-05-18  8:00       ` [dpdk-dev] [PATCH v5] " Wei Zhao
2020-05-18  8:45         ` Jeff Guo
2020-05-19  1:28         ` Ye Xiaolong

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git