power: fix use-after-free in pstate code
Checks
Commit Message
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
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>
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
>
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.
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.
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
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
@@ -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 */