net/i40e: fix the security risk of wild pointer operation
Checks
Commit Message
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
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
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;
> }
>
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;
>> }
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;
> >> }
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;
>> >> }
@@ -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;
}