KASAN issues with idle / hotplug area (was: Re: [PATCH v5sub1 7/8] arm64: move kernel image to base of vmalloc area)
Mark Rutland
mark.rutland at arm.com
Wed Feb 17 09:56:56 PST 2016
On Wed, Feb 17, 2016 at 05:01:11PM +0000, Mark Rutland wrote:
> On Wed, Feb 17, 2016 at 02:39:51PM +0000, Mark Rutland wrote:
> > Perhaps the simplest option is to not instrument invoke_psci_fn_* and
> > psci_suspend_finisher. Do we have a per-function annotation to avoid
> > KASAN instrumentation, like notrace? I need to investigate, but we may
> > also need notrace for similar reasons.
>
> I came up with the patch below, per the reasoning above.
>
> It _changes_ the KASAN splats (I see errors in tick_program_event rather
> than find_busiest_group), but doesn't seem to get rid of them. I'm not
> sure if I've missed something, or if we also have another latent issue.
>
> Ideas?
I'd missed annotating __cpu_suspend_save. I've fixed that up locally
(along with s/virt_to_phys/__virt_to_phys due to the inlining issue).
I'm still missing somehing; I'm getting KASAN warnings in find_busiest_group
again, and the shadow looks like it's corrupt (the second batch of f3 /
KASAN_STACK_RIGHT don't have a matching f1 / KASAN_STACK_LEFT):
[ 13.138791] Memory state around the buggy address:
[ 13.143624] ffffffc936a7fb80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 13.150929] ffffffc936a7fc00: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00
[ 13.158232] >ffffffc936a7fc80: f3 f3 f3 f3 00 00 00 00 00 f4 f4 f4 f3 f3 f3 f3
[ 13.165530] ^
[ 13.169066] ffffffc936a7fd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 13.176369] ffffffc936a7fd80: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
This is turning into a whack-a-mole game...
Mark.
> ---->8----
> From 8f7ae44d8f8862f5300483d45617b5bd05fc652f Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland at arm.com>
> Date: Wed, 17 Feb 2016 15:38:22 +0000
> Subject: [PATCH] arm64/psci: avoid KASAN splats with idle
>
> When a CPU goes into a deep idle state, we store CPU context in
> __cpu_suspend_enter, then call psci_suspend_finisher to invoke the
> firmware. If we entered a deep idle state, we do not return directly,
> and instead start cold, restoring state in cpu_resume.
>
> Thus we may execute the prologue and body of psci_suspend_finisher and
> the PSCI invocation function, but not their epilogue. When using KASAN
> this means that we poison a region of shadow memory, but never unpoison
> it. After we resume, subsequent stack accesses may hit the stale poison
> values, leading to false positives from KASAN.
>
> To avoid this, we must ensure that functions called after the context
> save are not instrumented, and do not posion the shadow region, by
> annotating them with __no_sanitize_address. As common inlines they may
> call are not similarly annotated, and the compiler refuses to allow
> function attribute mismatches, we must also avoid calls to such
> functions.
>
> ARM is not affected, as it does not support KASAN. When CONFIG_KASAN is
> not selected, __no_sanitize_address expands to nothing, so the
> annotation should not be harmful.
>
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> ---
> arch/arm64/kernel/psci.c | 14 ++++++++------
> drivers/firmware/psci.c | 3 +++
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index f67f35b..8324ce8 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -32,12 +32,16 @@
>
> static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
>
> +static phys_addr_t cpu_resume_phys;
> +
> static int __maybe_unused cpu_psci_cpu_init_idle(unsigned int cpu)
> {
> int i, ret, count = 0;
> u32 *psci_states;
> struct device_node *state_node, *cpu_node;
>
> + cpu_resume_phys = virt_to_phys(cpu_resume);
> +
> cpu_node = of_get_cpu_node(cpu, NULL);
> if (!cpu_node)
> return -ENODEV;
> @@ -178,12 +182,10 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
> }
> #endif
>
> -static int psci_suspend_finisher(unsigned long index)
> +__no_sanitize_address
> +static int psci_suspend_finisher(unsigned long state)
> {
> - u32 *state = __this_cpu_read(psci_power_state);
> -
> - return psci_ops.cpu_suspend(state[index - 1],
> - virt_to_phys(cpu_resume));
> + return psci_ops.cpu_suspend(state, cpu_resume_phys);
> }
>
> static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
> @@ -200,7 +202,7 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
> if (!psci_power_state_loses_context(state[index - 1]))
> ret = psci_ops.cpu_suspend(state[index - 1], 0);
> else
> - ret = cpu_suspend(index, psci_suspend_finisher);
> + ret = cpu_suspend(state[index - 1], psci_suspend_finisher);
>
> return ret;
> }
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index f25cd79..e4e8dc1 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -106,6 +106,7 @@ bool psci_power_state_is_valid(u32 state)
> return !(state & ~valid_mask);
> }
>
> +__no_sanitize_address
> static unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
> unsigned long arg0, unsigned long arg1,
> unsigned long arg2)
> @@ -116,6 +117,7 @@ static unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
> return res.a0;
> }
>
> +__no_sanitize_address
> static unsigned long __invoke_psci_fn_smc(unsigned long function_id,
> unsigned long arg0, unsigned long arg1,
> unsigned long arg2)
> @@ -148,6 +150,7 @@ static u32 psci_get_version(void)
> return invoke_psci_fn(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> }
>
> +__no_sanitize_address
> static int psci_cpu_suspend(u32 state, unsigned long entry_point)
> {
> int err;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list