| Message ID | 20230329133600.908723-1-yajun.deng@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: Fix policy->freq_table is NULL in __cpufreq_driver_target() | expand |
On Wed, Mar 29, 2023 at 3:36 PM Yajun Deng <yajun.deng@linux.dev> wrote: > > __resolve_freq() may be return target_freq if policy->freq_table is > NULL. In this case, it should return -EINVAL before __target_index(). Even so, __target_index() itself doesn't dereference freq_table AFAICS, so arguably the driver should be prepared to deal with a NULL freq_table which comes from it after all. Or, if you want to argue that drivers providing ->target_index() must also provide freq_table that is not NULL, a check to that effect needs to be done at the driver registration time IMO. > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index c0e5be0fe2d6..308a3df1a940 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2299,7 +2299,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, > return cpufreq_driver->target(policy, target_freq, relation); > } > > - if (!cpufreq_driver->target_index) > + if (!cpufreq_driver->target_index || !policy->freq_table) > return -EINVAL; > > return __target_index(policy, policy->cached_resolved_idx); > --
March 29, 2023 10:21 PM, "Rafael J. Wysocki" <rafael@kernel.org> wrote: > On Wed, Mar 29, 2023 at 3:36 PM Yajun Deng <yajun.deng@linux.dev> wrote: > >> __resolve_freq() may be return target_freq if policy->freq_table is >> NULL. In this case, it should return -EINVAL before __target_index(). > > Even so, __target_index() itself doesn't dereference freq_table > AFAICS, so arguably the driver should be prepared to deal with a NULL > freq_table which comes from it after all. > But there is a statement 'unsigned int newfreq = policy->freq_table[index].frequency;' in __target_index(), if driver doesn't provide freq_table, __target_index() will fault before the driver itself. > Or, if you want to argue that drivers providing ->target_index() must > also provide freq_table that is not NULL, a check to that effect needs > to be done at the driver registration time IMO. > >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >> --- >> drivers/cpufreq/cpufreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index c0e5be0fe2d6..308a3df1a940 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -2299,7 +2299,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, >> return cpufreq_driver->target(policy, target_freq, relation); >> } >> >> - if (!cpufreq_driver->target_index) >> + if (!cpufreq_driver->target_index || !policy->freq_table) >> return -EINVAL; >> >> return __target_index(policy, policy->cached_resolved_idx); >> --
On 30-03-23, 01:39, Yajun Deng wrote: > March 29, 2023 10:21 PM, "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > > On Wed, Mar 29, 2023 at 3:36 PM Yajun Deng <yajun.deng@linux.dev> wrote: > > > >> __resolve_freq() may be return target_freq if policy->freq_table is > >> NULL. In this case, it should return -EINVAL before __target_index(). > > > > Even so, __target_index() itself doesn't dereference freq_table > > AFAICS, so arguably the driver should be prepared to deal with a NULL > > freq_table which comes from it after all. > > > > But there is a statement 'unsigned int newfreq = policy->freq_table[index].frequency;' > in __target_index(), if driver doesn't provide freq_table, __target_index() > will fault before the driver itself. Driver must provide a freq table here.
On Thu, Mar 30, 2023 at 5:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 30-03-23, 01:39, Yajun Deng wrote: > > March 29, 2023 10:21 PM, "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > > > > On Wed, Mar 29, 2023 at 3:36 PM Yajun Deng <yajun.deng@linux.dev> wrote: > > > > > >> __resolve_freq() may be return target_freq if policy->freq_table is > > >> NULL. In this case, it should return -EINVAL before __target_index(). > > > > > > Even so, __target_index() itself doesn't dereference freq_table > > > AFAICS, so arguably the driver should be prepared to deal with a NULL > > > freq_table which comes from it after all. > > > > > > > But there is a statement 'unsigned int newfreq = policy->freq_table[index].frequency;' > > in __target_index(), if driver doesn't provide freq_table, __target_index() > > will fault before the driver itself. > > Driver must provide a freq table here. OK, so let's do the check when the driver gets registered.
On 29-03-23, 21:36, Yajun Deng wrote: > __resolve_freq() may be return target_freq if policy->freq_table is > NULL. In this case, it should return -EINVAL before __target_index(). > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index c0e5be0fe2d6..308a3df1a940 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2299,7 +2299,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, > return cpufreq_driver->target(policy, target_freq, relation); > } > > - if (!cpufreq_driver->target_index) > + if (!cpufreq_driver->target_index || !policy->freq_table) > return -EINVAL; Hi, I have sent an alternate patch [1] for this, please try it.
April 3, 2023 12:11 PM, "Viresh Kumar" <viresh.kumar@linaro.org> wrote: > On 29-03-23, 21:36, Yajun Deng wrote: > >> __resolve_freq() may be return target_freq if policy->freq_table is >> NULL. In this case, it should return -EINVAL before __target_index(). >> >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >> --- >> drivers/cpufreq/cpufreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index c0e5be0fe2d6..308a3df1a940 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -2299,7 +2299,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, >> return cpufreq_driver->target(policy, target_freq, relation); >> } >> >> - if (!cpufreq_driver->target_index) >> + if (!cpufreq_driver->target_index || !policy->freq_table) >> return -EINVAL; > > Hi, > > I have sent an alternate patch [1] for this, please try it. > Thanks, v2 is fine. > -- > viresh > > [1] > https://lore.kernel.org/all/53d4ed4e5b18a59a48790434f8146fb207e11c49.1680494945.git.viresh.kumar@lin > ro.org
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c0e5be0fe2d6..308a3df1a940 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2299,7 +2299,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, return cpufreq_driver->target(policy, target_freq, relation); } - if (!cpufreq_driver->target_index) + if (!cpufreq_driver->target_index || !policy->freq_table) return -EINVAL; return __target_index(policy, policy->cached_resolved_idx);
__resolve_freq() may be return target_freq if policy->freq_table is NULL. In this case, it should return -EINVAL before __target_index(). Signed-off-by: Yajun Deng <yajun.deng@linux.dev> --- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)