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

Message ID 20191010113937.15962-2-ulf.hansson@linaro.org
State New
Headers show
Series
  • cpuidle: psci: Support hierarchical CPU arrangement
Related show

Commit Message

Ulf Hansson Oct. 10, 2019, 11:39 a.m.
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(-)

-- 
2.17.1

Comments

Lorenzo Pieralisi Oct. 18, 2019, 9:38 a.m. | #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. | #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, 4:47 p.m. | #3
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

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 = {