diff mbox series

net/i40e: fix the security risk of wild pointer operation

Message ID 20200512151915.105152-1-wei.zhao1@intel.com (mailing list archive)
State Superseded, archived
Delegated to: xiaolong ye
Headers
Series net/i40e: fix the security risk of wild pointer operation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Zhao1, Wei May 12, 2020, 3:19 p.m. UTC 
  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(-)

Comments

Zhao1, Wei May 15, 2020, 2:24 a.m. UTC | #1 
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
Guo, Jia May 15, 2020, 6:32 a.m. UTC | #2 
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;
>   }
>
Xiaolong Ye May 15, 2020, 7:28 a.m. UTC | #3 
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;
>>   }
Zhao1, Wei May 18, 2020, 5:24 a.m. UTC | #4 
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;
> >>   }
Xiaolong Ye May 18, 2020, 5:32 a.m. UTC | #5 
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;
>> >>   }
diff mbox series

Patch

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;
 }