* [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
* 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 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
* [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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).