| Message ID | 20210407155642.435964-1-anatoly.burakov@intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | power: fix use-after-free in pstate code | expand |
| Context | Check | Description |
|---|---|---|
| ci/intel-Testing | success | Testing PASS |
| ci/iol-testing | success | Testing PASS |
| ci/iol-abi-testing | success | Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/github-robot | success | github build: passed |
| ci/travis-robot | success | travis build: passed |
| ci/checkpatch | success | coding style OK |
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
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 */
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(-)