diff mbox series

power: fix use-after-free in pstate code

Message ID 20210407155642.435964-1-anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series power: fix use-after-free in pstate code |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Burakov, Anatoly April 7, 2021, 3:56 p.m. UTC 
  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(-)

Comments

Hunt, David April 7, 2021, 4:10 p.m. UTC | #1 
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>
Liang Ma April 7, 2021, 4:18 p.m. UTC | #2 
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
>
Burakov, Anatoly April 7, 2021, 4:31 p.m. UTC | #3 
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.
Burakov, Anatoly April 7, 2021, 4:53 p.m. UTC | #4 
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.
Liang Ma April 7, 2021, 5:08 p.m. UTC | #5 
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
Thomas Monjalon April 15, 2021, 9:29 p.m. UTC | #6 
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
diff mbox series

Patch

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 */