On Fri, Oct 18, 2019 at 11:51:11AM +0200, Ulf Hansson wrote:
> On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> wrote:
> >
> > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote:
> > > When the WFI state have been selected, the in-parameter idx to
> > > psci_enter_idle_state() is zero. In this case, we must not index the state
> > > array as "state[idx - 1]", as it means accessing data outside the array.
> > > Fix the bug by pre-checking if idx is zero.
> > >
> > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling")
> > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > ---
> > > drivers/cpuidle/cpuidle-psci.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index f3c1a2396f98..2e91c8d6c211 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > > static int psci_enter_idle_state(struct cpuidle_device *dev,
> > > struct cpuidle_driver *drv, int idx)
> > > {
> > > - u32 *state = __this_cpu_read(psci_power_state);
> > > + u32 *states = __this_cpu_read(psci_power_state);
> > > + u32 state = idx ? states[idx - 1] : 0;
> > >
> > > - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > > - idx, state[idx - 1]);
> > > + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> >
> > Technically we don't dereference that array entry but I agree this
> > is ugly and potentially broken.
>
> No sure understand the non-deference part.
>
> If the governor selects WFI, the idx will be 0 - and thus we end up
> using state[-1], doesn't that dereference an invalid address, no?
No because CPU_PM_CPU_IDLE_ENTER_PARAM is a macro, the code it
preprocesses to won't dereference state[idx - 1] if idx == 0.
I agree it is *very* ugly but technically code is not broken.
> > My preference is aligning it with ACPI code and allocate one more
> > entry in the psci_power_state array (useless for wfi, agreed but
> > at least we remove this (-1) handling from the code).
>
> I can do that, but sounds like a slightly bigger change. Are you fine
> if I do that on top, so we can get this sent as fix for v5.4-rc[n]?
Technically we are not fixing anything; it is not such a big
change, we need to allocate one entry more and update the array
indexing.
Lorenzo
