diff mbox series

[01/13] cpuidle: psci: Fix potential access to unmapped memory

Message ID 20191010113937.15962-2-ulf.hansson@linaro.org (mailing list archive)
State Superseded
Headers show
Series cpuidle: psci: Support hierarchical CPU arrangement | expand

Commit Message

Ulf Hansson Oct. 10, 2019, 11:39 a.m. UTC
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@linaro.org>
---
 drivers/cpuidle/cpuidle-psci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Lorenzo Pieralisi Oct. 18, 2019, 9:38 a.m. UTC | #1
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@linaro.org>
> ---
>  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.

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).

Thanks,
Lorenzo

>  }
>  
>  static struct cpuidle_driver psci_idle_driver __initdata = {
> -- 
> 2.17.1
>
Ulf Hansson Oct. 18, 2019, 9:51 a.m. UTC | #2
On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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@linaro.org>
> > ---
> >  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?

>
> 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]?

Kind regards
Uffe
Lorenzo Pieralisi Oct. 18, 2019, 10:03 a.m. UTC | #3
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@arm.com> 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@linaro.org>
> > > ---
> > >  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
Ulf Hansson Oct. 18, 2019, 10:29 a.m. UTC | #4
On Fri, 18 Oct 2019 at 12:03, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> 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@arm.com> 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@linaro.org>
> > > > ---
> > > >  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.

Ahh, got it, thanks!

>
> > > 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.

Okay, let me do the change - and it seems like it doesn't even have to
be sent as a fix then. Right?

Kind regards
Uffe
Lorenzo Pieralisi Oct. 18, 2019, 4:47 p.m. UTC | #5
On Fri, Oct 18, 2019 at 12:29:54PM +0200, Ulf Hansson wrote:

[...]

> > 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.
> 
> Okay, let me do the change - and it seems like it doesn't even have to
> be sent as a fix then. Right?

No it does not (even though I agree that's misleading and "fixing"
it for v5.4 would not hurt either).

Lorenzo
diff mbox series

Patch

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);
 }
 
 static struct cpuidle_driver psci_idle_driver __initdata = {