Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation

[Ye Xiaolong <xiaolong.ye@intel.com>]

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