| Message ID | d1fca705-bfed-4370-a907-ca090dea58e5@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xenpm: sanitize allocations in show_cpufreq_para_by_cpuid() | expand |
On 25/03/2025 12:51 pm, Jan Beulich wrote: > malloc(), when passed zero size, may return NULL (the behavior is > implementation defined). More importantly, this is the Musl behaviour, so is how ~most of Gitlab CI behaves. > Extend the ->gov_num check to the other two > allocations as well. I'm not sure what you mean by this. Only one of these hunks has a ->gov_num check. > Don't chance then actually using a NULL in > print_cpufreq_para(). > > Fixes: 75e06d089d48 ("xenpm: add cpu frequency control interface, through which user can") > Signed-off-by: Jan Beulich <jbeulich@suse.com> The code change looks ok, so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> but I'd prefer a clarified commit message.
On 25.03.2025 15:20, Andrew Cooper wrote: > On 25/03/2025 12:51 pm, Jan Beulich wrote: >> malloc(), when passed zero size, may return NULL (the behavior is >> implementation defined). > > More importantly, this is the Musl behaviour, so is how ~most of Gitlab > CI behaves. > >> Extend the ->gov_num check to the other two >> allocations as well. > > I'm not sure what you mean by this. Only one of these hunks has a > ->gov_num check. Not sure I see a better way of wording this: What I mean to say is that the ->gov_num check that there is already is being replicated (with "gov" replaced accordingly) to the other two places where similar allocations happen. Maybe simply s/Extend/Mirror" would already help? >> Don't chance then actually using a NULL in >> print_cpufreq_para(). >> >> Fixes: 75e06d089d48 ("xenpm: add cpu frequency control interface, through which user can") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > The code change looks ok, so Acked-by: Andrew Cooper > <andrew.cooper3@citrix.com> but I'd prefer a clarified commit message. Thanks. Jan
On 25.03.2025 15:30, Jan Beulich wrote: > On 25.03.2025 15:20, Andrew Cooper wrote: >> On 25/03/2025 12:51 pm, Jan Beulich wrote: >>> malloc(), when passed zero size, may return NULL (the behavior is >>> implementation defined). >> >> More importantly, this is the Musl behaviour, so is how ~most of Gitlab >> CI behaves. >> >>> Extend the ->gov_num check to the other two >>> allocations as well. >> >> I'm not sure what you mean by this. Only one of these hunks has a >> ->gov_num check. > > Not sure I see a better way of wording this: What I mean to say is that > the ->gov_num check that there is already is being replicated (with "gov" > replaced accordingly) to the other two places where similar allocations > happen. Maybe simply s/Extend/Mirror" would already help? Oh, and to clarify: The ->gov_num check mentioned isn't the one visible in one of the hunks. It's entirely outside of patch context. It was put there (for [now] questionable reasons by the HWP work). Jan
On 2025-03-25 10:30, Jan Beulich wrote: > On 25.03.2025 15:20, Andrew Cooper wrote: >> On 25/03/2025 12:51 pm, Jan Beulich wrote: >>> malloc(), when passed zero size, may return NULL (the behavior is >>> implementation defined). >> >> More importantly, this is the Musl behaviour, so is how ~most of Gitlab >> CI behaves. >> >>> Extend the ->gov_num check to the other two >>> allocations as well. >> >> I'm not sure what you mean by this. Only one of these hunks has a >> ->gov_num check. > > Not sure I see a better way of wording this: What I mean to say is that > the ->gov_num check that there is already is being replicated (with "gov" > replaced accordingly) to the other two places where similar allocations > happen. Maybe simply s/Extend/Mirror" would already help? Yes, "Mirror" is better. >>> Don't chance then actually using a NULL in >>> print_cpufreq_para(). >>> >>> Fixes: 75e06d089d48 ("xenpm: add cpu frequency control interface, through which user can") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> The code change looks ok, so Acked-by: Andrew Cooper >> <andrew.cooper3@citrix.com> but I'd prefer a clarified commit message. Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
--- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -840,8 +840,9 @@ static void print_cpufreq_para(int cpuid } else { - printf("scaling_avail_gov : %s\n", - p_cpufreq->scaling_available_governors); + if ( p_cpufreq->gov_num ) + printf("scaling_avail_gov : %s\n", + p_cpufreq->scaling_available_governors); printf("current_governor : %s\n", p_cpufreq->u.s.scaling_governor); if ( !strncmp(p_cpufreq->u.s.scaling_governor, @@ -907,7 +908,8 @@ static int show_cpufreq_para_by_cpuid(xc p_cpufreq->scaling_available_frequencies = NULL; p_cpufreq->scaling_available_governors = NULL; - if (!(p_cpufreq->affected_cpus = + if (p_cpufreq->cpu_num && + !(p_cpufreq->affected_cpus = malloc(p_cpufreq->cpu_num * sizeof(uint32_t)))) { fprintf(stderr, @@ -916,7 +918,8 @@ static int show_cpufreq_para_by_cpuid(xc ret = -ENOMEM; goto out; } - if (!(p_cpufreq->scaling_available_frequencies = + if (p_cpufreq->freq_num && + !(p_cpufreq->scaling_available_frequencies = malloc(p_cpufreq->freq_num * sizeof(uint32_t)))) { fprintf(stderr,
malloc(), when passed zero size, may return NULL (the behavior is implementation defined). Extend the ->gov_num check to the other two allocations as well. Don't chance then actually using a NULL in print_cpufreq_para(). Fixes: 75e06d089d48 ("xenpm: add cpu frequency control interface, through which user can") Signed-off-by: Jan Beulich <jbeulich@suse.com>