From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id D7707A00C3; Fri, 15 May 2020 09:36:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A7D3F1DA5F; Fri, 15 May 2020 09:36:27 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7277E1DA0B; Fri, 15 May 2020 09:36:26 +0200 (CEST) IronPort-SDR: e1RF8PEh4l484ixlt8K00gzuPjpkpdTHCAe8/rW9DBqVHM886Sw8uR0DU6mtjCGNbosNzhGOiq +5ysP5lzchRQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2020 00:36:24 -0700 IronPort-SDR: ZlYFbn2PJsKHsbJ8KseUG28LSSW2i0zRPdq3iXRzjJ03X8p4x2WySk2CTv5BOYCofRb4YHOPx+ ZnJ7XN0hgQIg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,394,1583222400"; d="scan'208";a="287701072" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.116.183]) by fmsmga004.fm.intel.com with ESMTP; 15 May 2020 00:36:22 -0700 Date: Fri, 15 May 2020 15:28:06 +0800 From: Ye Xiaolong To: Jeff Guo Cc: Wei Zhao , dev@dpdk.org, stable@dpdk.org, beilei.xing@intel.com Message-ID: <20200515072806.GC1064@intel.com> References: <20200512151915.105152-1-wei.zhao1@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix the security risk of wild pointer operation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> --- >> 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; >> }