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

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Lina Iyer <ilina@codeaurora.org>,
	linux-pm@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kevin Hilman <khilman@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 01/13] cpuidle: psci: Fix potential access to unmapped memory
Date: Fri, 18 Oct 2019 10:38:39 +0100	[thread overview]
Message-ID: <20191018093839.GB25918@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <20191010113937.15962-2-ulf.hansson@linaro.org>

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
> 