DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
@ 2021-04-07 15:56 Anatoly Burakov
  2021-04-07 16:10 ` David Hunt
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Anatoly Burakov @ 2021-04-07 15:56 UTC (permalink / raw)
  To: dev; +Cc: david.hunt, thomas

Previous fix has addressed the incorrect handling of `base_frequency`
file, but has added a use-after-free error due to the fact that all
further code paths will lead to an `fclose()` call at the end, so the
additional `fclose()` call right after processing the file was
unnecessary.

Coverity issue: 369901

Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 1cb0e4d917..ec745153d3 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -220,7 +220,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 
 		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
 				/ BUS_FREQ;
-		fclose(f_base);
 	}
 
 	/* Add MSR read to detect turbo status */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 15:56 [dpdk-dev] [PATCH] power: fix use-after-free in pstate code Anatoly Burakov
@ 2021-04-07 16:10 ` David Hunt
  2021-04-15 21:29   ` Thomas Monjalon
  2021-04-07 16:18 ` Liang Ma
  2021-04-07 16:31 ` Burakov, Anatoly
  2 siblings, 1 reply; 7+ messages in thread
From: David Hunt @ 2021-04-07 16:10 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: thomas

Hi Anatoly,

On 7/4/2021 4:56 PM, Anatoly Burakov wrote:
> Previous fix has addressed the incorrect handling of `base_frequency`
> file, but has added a use-after-free error due to the fact that all
> further code paths will lead to an `fclose()` call at the end, so the
> additional `fclose()` call right after processing the file was
> unnecessary.
>
> Coverity issue: 369901
>
> Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_power/power_pstate_cpufreq.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> index 1cb0e4d917..ec745153d3 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -220,7 +220,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
>   
>   		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
>   				/ BUS_FREQ;
> -		fclose(f_base);
>   	}
>   
>   	/* Add MSR read to detect turbo status */


Yes, removing the fclose will do it. Either that or add an "f_base = 
NULL" after the fclose, but the fclose removal is fine.

Acked-by: David Hunt <david.hunt@intel.com>





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 15:56 [dpdk-dev] [PATCH] power: fix use-after-free in pstate code Anatoly Burakov
  2021-04-07 16:10 ` David Hunt
@ 2021-04-07 16:18 ` Liang Ma
  2021-04-07 16:31 ` Burakov, Anatoly
  2 siblings, 0 replies; 7+ messages in thread
From: Liang Ma @ 2021-04-07 16:18 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, david.hunt, thomas

Reviewed-by: Liang Ma <liangma@liangbit.com>

On Wed, Apr 07, 2021 at 03:56:42PM +0000, Anatoly Burakov wrote:
> Previous fix has addressed the incorrect handling of `base_frequency`
> file, but has added a use-after-free error due to the fact that all
> further code paths will lead to an `fclose()` call at the end, so the
> additional `fclose()` call right after processing the file was
> unnecessary.
> 
> Coverity issue: 369901
> 
> Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_power/power_pstate_cpufreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> index 1cb0e4d917..ec745153d3 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -220,7 +220,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
>  
>  		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
>  				/ BUS_FREQ;
> -		fclose(f_base);
>  	}
>  
>  	/* Add MSR read to detect turbo status */
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 15:56 [dpdk-dev] [PATCH] power: fix use-after-free in pstate code Anatoly Burakov
  2021-04-07 16:10 ` David Hunt
  2021-04-07 16:18 ` Liang Ma
@ 2021-04-07 16:31 ` Burakov, Anatoly
  2021-04-07 16:53   ` Burakov, Anatoly
  2 siblings, 1 reply; 7+ messages in thread
From: Burakov, Anatoly @ 2021-04-07 16:31 UTC (permalink / raw)
  To: dev; +Cc: david.hunt, thomas

On 07-Apr-21 4:56 PM, Anatoly Burakov wrote:
> Previous fix has addressed the incorrect handling of `base_frequency`
> file, but has added a use-after-free error due to the fact that all
> further code paths will lead to an `fclose()` call at the end, so the
> additional `fclose()` call right after processing the file was
> unnecessary.
> 
> Coverity issue: 369901
> 
> Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Actually, self-nack, because this:

	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
			pi->lcore_id);
	f_min = fopen(fullpath_min, "rw+");
	FOPEN_OR_ERR_RET(f_min, -1);

	snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
			pi->lcore_id);
	f_max = fopen(fullpath_max, "rw+");
	if (f_max == NULL)
		fclose(f_min);
	FOPEN_OR_ERR_RET(f_max, -1);

comes after, and will leak the f_base descriptor. Closing it and setting 
it to NULL seems like a better solution.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 16:31 ` Burakov, Anatoly
@ 2021-04-07 16:53   ` Burakov, Anatoly
  2021-04-07 17:08     ` Liang Ma
  0 siblings, 1 reply; 7+ messages in thread
From: Burakov, Anatoly @ 2021-04-07 16:53 UTC (permalink / raw)
  To: dev; +Cc: david.hunt, thomas

On 07-Apr-21 5:31 PM, Burakov, Anatoly wrote:
> On 07-Apr-21 4:56 PM, Anatoly Burakov wrote:
>> Previous fix has addressed the incorrect handling of `base_frequency`
>> file, but has added a use-after-free error due to the fact that all
>> further code paths will lead to an `fclose()` call at the end, so the
>> additional `fclose()` call right after processing the file was
>> unnecessary.
>>
>> Coverity issue: 369901
>>
>> Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
> 
> Actually, self-nack, because this:
> 
>      snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
>              pi->lcore_id);
>      f_min = fopen(fullpath_min, "rw+");
>      FOPEN_OR_ERR_RET(f_min, -1);
> 
>      snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
>              pi->lcore_id);
>      f_max = fopen(fullpath_max, "rw+");
>      if (f_max == NULL)
>          fclose(f_min);
>      FOPEN_OR_ERR_RET(f_max, -1);
> 
> comes after, and will leak the f_base descriptor. Closing it and setting 
> it to NULL seems like a better solution.
> 

Actually no, scratch that, it doesn't :) that's before. So, patch should 
be OK.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 16:53   ` Burakov, Anatoly
@ 2021-04-07 17:08     ` Liang Ma
  0 siblings, 0 replies; 7+ messages in thread
From: Liang Ma @ 2021-04-07 17:08 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev, david.hunt, thomas

On Wed, Apr 07, 2021 at 05:53:48PM +0100, Burakov, Anatoly wrote:
> On 07-Apr-21 5:31 PM, Burakov, Anatoly wrote:
> > On 07-Apr-21 4:56 PM, Anatoly Burakov wrote:
> > > Previous fix has addressed the incorrect handling of `base_frequency`
> > > file, but has added a use-after-free error due to the fact that all
> > > further code paths will lead to an `fclose()` call at the end, so the
> > > additional `fclose()` call right after processing the file was
> > > unnecessary.
> > > 
> > > Coverity issue: 369901
> > > 
> > > Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
> > > 
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > ---
> > 
> > Actually, self-nack, because this:
> > 
> >      snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
> >              pi->lcore_id);
> >      f_min = fopen(fullpath_min, "rw+");
> >      FOPEN_OR_ERR_RET(f_min, -1);
> > 
> >      snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
> >              pi->lcore_id);
> >      f_max = fopen(fullpath_max, "rw+");
> >      if (f_max == NULL)
> >          fclose(f_min);
> >      FOPEN_OR_ERR_RET(f_max, -1);
> > 
> > comes after, and will leak the f_base descriptor. Closing it and setting
> > it to NULL seems like a better solution.
> > 
> 
> Actually no, scratch that, it doesn't :) that's before. So, patch should be
> OK.
A Cup Coffee will help ;-) 
> 
> -- 
> Thanks,
> Anatoly


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] power: fix use-after-free in pstate code
  2021-04-07 16:10 ` David Hunt
@ 2021-04-15 21:29   ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2021-04-15 21:29 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, David Hunt

07/04/2021 18:10, David Hunt:
> Hi Anatoly,
> 
> On 7/4/2021 4:56 PM, Anatoly Burakov wrote:
> > Previous fix has addressed the incorrect handling of `base_frequency`
> > file, but has added a use-after-free error due to the fact that all
> > further code paths will lead to an `fclose()` call at the end, so the
> > additional `fclose()` call right after processing the file was
> > unnecessary.
> >
> > Coverity issue: 369901
> >
> > Fixes: 8a5febaac4f7 ("power: fix P-state base frequency handling")
> >
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Yes, removing the fclose will do it. Either that or add an "f_base = 
> NULL" after the fclose, but the fclose removal is fine.
> 
> Acked-by: David Hunt <david.hunt@intel.com>

Applied, thanks



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-04-15 21:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 15:56 [dpdk-dev] [PATCH] power: fix use-after-free in pstate code Anatoly Burakov
2021-04-07 16:10 ` David Hunt
2021-04-15 21:29   ` Thomas Monjalon
2021-04-07 16:18 ` Liang Ma
2021-04-07 16:31 ` Burakov, Anatoly
2021-04-07 16:53   ` Burakov, Anatoly
2021-04-07 17:08     ` Liang Ma

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git