[PATCH] kprobes: unpoison stack in jprobe_return() for KASAN

[Dmitry Vyukov]

KASAN stack instrumentation poisons stack redzones on function entry
and unpoisons them on function exit. If a function exits abnormally
(e.g. with a longjmp like jprobe_return()), stack redzones are left
poisoned. Later this leads to random KASAN false reports.

Unpoison stack redzones in the frames we are going to jump over
before doing actual longjmp in jprobe_return().

Also change kasan_unpoison_remaining_stack() to not assume that
we are on a normal task stack, as jprobes can be used in interrupts.

Signed-off-by: Dmitry Vyukov <dvy...@google.com>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Alexander Potapenko <gli...@google.com>
Cc: Will Deacon <will....@arm.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Masami Hiramatsu <mhir...@kernel.org>
Cc: x...@kernel.org
Cc: kasa...@googlegroups.com

--

I observe false positives due to this in sctp code.
sctp uses jprobe_return() in jsctp_sf_eat_sack().
The stray 0xf4 in shadow memory are stack redzones.

[ 376.492209] ==================================================================
[ 376.500368] BUG: KASAN: stack-out-of-bounds in memcmp+0xe9/0x150 at addr ffff88005e48f480
[ 376.509522] Read of size 1 by task syz-executor/18535
[ 376.515249] page:ffffea00017923c0 count:0 mapcount:0 mapping: (null) index:0x0
[ 376.524377] flags: 0x1fffc0000000000()
[ 376.528645] page dumped because: kasan: bad access detected
[ 376.534939] CPU: 1 PID: 18535 Comm: syz-executor Not tainted 4.8.0+ #28
[ 376.542375] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[ 376.552669] ffff88005e48f2d0 ffffffff82d2b849 ffffffff0bc91e90 fffffbfff10971e8
[ 376.561599] ffffed000bc91e90 ffffed000bc91e90 0000000000000001 0000000000000000
[ 376.570486] ffff88005e48f480 ffff88005e48f350 ffffffff817d3169 ffff88005e48f370
[ 376.579348] Call Trace:
[ 376.582196] [<ffffffff82d2b849>] dump_stack+0x12e/0x185
[ 376.588190] [<ffffffff817d3169>] kasan_report+0x489/0x4b0
[ 376.594378] [<ffffffff82d49529>] ? memcmp+0xe9/0x150
[ 376.600099] [<ffffffff8126377e>] ? update_stack_state.constprop.4+0xde/0x150
[ 376.608187] [<ffffffff817d31a9>] __asan_report_load1_noabort+0x19/0x20
[ 376.615620] [<ffffffff82d49529>] memcmp+0xe9/0x150
[ 376.621148] [<ffffffff82df7486>] depot_save_stack+0x176/0x5c0
[ 376.627722] [<ffffffff85b03f19>] ? skb_free_head+0x79/0xb0
[ 376.634006] [<ffffffff817d2031>] save_stack+0xb1/0xd0
[ 376.639805] [<ffffffff811fc8ab>] ? save_stack_trace+0x1b/0x20
[ 376.646369] [<ffffffff817d1fc6>] ? save_stack+0x46/0xd0
[ 376.652366] [<ffffffff817d27f2>] ? kasan_slab_free+0x72/0xc0
[ 376.658837] [<ffffffff817d05b8>] ? kfree+0xc8/0x2a0
[ 376.664454] [<ffffffff85b03f19>] ? skb_free_head+0x79/0xb0
[ 376.670736] [<ffffffff85b0900a>] ? skb_release_data+0x37a/0x420
[ 376.677517] [<ffffffff85b090ff>] ? skb_release_all+0x4f/0x60
[ 376.683992] [<ffffffff85b11348>] ? consume_skb+0x138/0x370
[ 376.690284] [<ffffffff8676ad7b>] ? sctp_chunk_put+0xcb/0x180
[ 376.696761] [<ffffffff8676ae88>] ? sctp_chunk_free+0x58/0x70
[ 376.703234] [<ffffffff8677fa5f>] ? sctp_inq_pop+0x68f/0xef0
[ 376.709616] [<ffffffff8675ee36>] ? sctp_assoc_bh_rcv+0xd6/0x4b0
[ 376.716379] [<ffffffff8677f2c1>] ? sctp_inq_push+0x131/0x190
[ 376.722946] [<ffffffff867bad69>] ? sctp_backlog_rcv+0xe9/0xa20
[ 376.729615] [<ffffffff85af70dc>] ? __release_sock+0x12c/0x3a0
[ 376.736186] [<ffffffff85af73ae>] ? release_sock+0x5e/0x1c0
[ 376.742465] [<ffffffff8679fc62>] ? sctp_sendmsg+0xd82/0x2e00
[ 376.748935] [<ffffffff85f83c43>] ? inet_sendmsg+0x303/0x4c0
[ 376.755318] [<ffffffff85aeab0f>] ? sock_sendmsg+0xcf/0x110
[ 376.761599] [<ffffffff85aebc1d>] ? SYSC_sendto+0x20d/0x340
[ 376.767882] [<ffffffff85aee175>] ? SyS_sendto+0x45/0x60
[ 376.773885] [<ffffffff81008783>] ? do_syscall_64+0x1d3/0x620
[ 376.780372] [<ffffffff86d8e9cd>] ? entry_SYSCALL64_slow_path+0x25/0x25
[ 376.787832] [<ffffffff86751c79>] ? sctp_do_sm+0x3689/0x4e90
[ 376.794215] [<ffffffff81467040>] ? debug_check_no_locks_freed+0x3c0/0x3c0
[ 376.801948] [<ffffffff8674e5f0>] ? sctp_do_8_2_transport_strike.isra.19+0x900/0x900
[ 376.810918] [<ffffffff81466ed0>] ? debug_check_no_locks_freed+0x250/0x3c0
[ 376.818650] [<ffffffff817d27f2>] kasan_slab_free+0x72/0xc0
[ 376.824945] [<ffffffff817d05b8>] kfree+0xc8/0x2a0
[ 376.830374] [<ffffffff85b03f19>] skb_free_head+0x79/0xb0
[ 376.836466] [<ffffffff85b0900a>] skb_release_data+0x37a/0x420
[ 376.843036] [<ffffffff85b090ff>] skb_release_all+0x4f/0x60
[ 376.849319] [<ffffffff85b11348>] consume_skb+0x138/0x370
[ 376.855411] [<ffffffff8676ad7b>] sctp_chunk_put+0xcb/0x180
[ 376.861702] [<ffffffff8676ae88>] sctp_chunk_free+0x58/0x70
[ 376.867990] [<ffffffff8677fa5f>] sctp_inq_pop+0x68f/0xef0
[ 376.874182] [<ffffffff8675ee36>] sctp_assoc_bh_rcv+0xd6/0x4b0
[ 376.880759] [<ffffffff8677f2c1>] sctp_inq_push+0x131/0x190
[ 376.887055] [<ffffffff867bad69>] sctp_backlog_rcv+0xe9/0xa20
[ 376.893532] [<ffffffff814667dd>] ? trace_hardirqs_on+0xd/0x10
[ 376.900107] [<ffffffff81369bed>] ? __local_bh_enable_ip+0xad/0x190
[ 376.907161] [<ffffffff85af70dc>] __release_sock+0x12c/0x3a0
[ 376.913540] [<ffffffff85af73ae>] release_sock+0x5e/0x1c0
[ 376.919922] [<ffffffff8679fc62>] sctp_sendmsg+0xd82/0x2e00
[ 376.926209] [<ffffffff8679eee0>] ? sctp_id2assoc+0x330/0x330
[ 376.932696] [<ffffffff81467040>] ? debug_check_no_locks_freed+0x3c0/0x3c0
[ 376.940417] [<ffffffff814a6127>] ? debug_lockdep_rcu_enabled+0x77/0x90
[ 376.947858] [<ffffffff814a6127>] ? debug_lockdep_rcu_enabled+0x77/0x90
[ 376.955293] [<ffffffff85f83940>] ? inet_recvmsg+0x4b0/0x4b0
[ 376.961675] [<ffffffff85f83b48>] ? inet_sendmsg+0x208/0x4c0
[ 376.968064] [<ffffffff85f83c43>] inet_sendmsg+0x303/0x4c0
[ 376.974261] [<ffffffff85f839b8>] ? inet_sendmsg+0x78/0x4c0
[ 376.980546] [<ffffffff85f83940>] ? inet_recvmsg+0x4b0/0x4b0
[ 376.986929] [<ffffffff85aeab0f>] sock_sendmsg+0xcf/0x110
[ 376.993025] [<ffffffff85aebc1d>] SYSC_sendto+0x20d/0x340
[ 376.999125] [<ffffffff85aeba10>] ? SYSC_connect+0x2e0/0x2e0
[ 377.005510] [<ffffffff8188cc33>] ? __fd_install+0x233/0x560
[ 377.011894] [<ffffffff81007b93>] ? perf_trace_sys_enter+0x443/0x9d0
[ 377.019051] [<ffffffff81007750>] ? syscall_trace_enter+0xe00/0xe00
[ 377.026109] [<ffffffff8158887f>] ? __sanitizer_cov_trace_pc+0x4f/0x60
[ 377.033466] [<ffffffff85aee175>] SyS_sendto+0x45/0x60
[ 377.039278] [<ffffffff85aee130>] ? SyS_getpeername+0x30/0x30
[ 377.045752] [<ffffffff81008783>] do_syscall_64+0x1d3/0x620
[ 377.052035] [<ffffffff8100501a>] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 377.059281] [<ffffffff86d8e9cd>] entry_SYSCALL64_slow_path+0x25/0x25
[ 377.066509] Memory state around the buggy address:
[ 377.071935] ffff88005e48f380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 377.080024] ffff88005e48f400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 377.088117] >ffff88005e48f480: f4 f4 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 377.096205] ^
[ 377.099900] ffff88005e48f500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 377.107994] ffff88005e48f580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 377.116079] ==================================================================
---
arch/x86/kernel/kprobes/core.c | 4 ++++
include/linux/kasan.h | 2 ++
mm/kasan/kasan.c | 29 +++++++++++++++++------------
3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 28cee01..16496a7 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -50,6 +50,7 @@
#include <linux/kallsyms.h>
#include <linux/ftrace.h>
#include <linux/frame.h>
+#include <linux/kasan.h>

#include <asm/text-patching.h>
#include <asm/cacheflush.h>
@@ -1080,6 +1081,9 @@ void jprobe_return(void)
{
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();

+ /* Unpoison stack redzones in the frames we are going to jump over. */
+ kasan_unpoison_remaining_stack(kcb->jprobe_saved_sp);
+
asm volatile (
#ifdef CONFIG_X86_64
" xchg %%rbx,%%rsp \n"
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index d600303..fb02d23 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -44,6 +44,7 @@ static inline void kasan_disable_current(void)
void kasan_unpoison_shadow(const void *address, size_t size);

void kasan_unpoison_task_stack(struct task_struct *task);
+void kasan_unpoison_remaining_stack(const void *sp);

void kasan_alloc_pages(struct page *page, unsigned int order);
void kasan_free_pages(struct page *page, unsigned int order);
@@ -85,6 +86,7 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
static inline void kasan_unpoison_shadow(const void *address, size_t size) {}

static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
+static inline void kasan_unpoison_remaining_stack(const void *sp) {}

static inline void kasan_enable_current(void) {}
static inline void kasan_disable_current(void) {}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 88af13c..f3e1666 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -62,24 +62,29 @@ void kasan_unpoison_shadow(const void *address, size_t size)
}
}

-static void __kasan_unpoison_stack(struct task_struct *task, void *sp)
-{
- void *base = task_stack_page(task);
- size_t size = sp - base;
-
- kasan_unpoison_shadow(base, size);
-}
-
/* Unpoison the entire stack for a task. */
void kasan_unpoison_task_stack(struct task_struct *task)
{
- __kasan_unpoison_stack(task, task_stack_page(task) + THREAD_SIZE);
+ kasan_unpoison_shadow(task_stack_page(task), THREAD_SIZE);
}

-/* Unpoison the stack for the current task beyond a watermark sp value. */
-asmlinkage void kasan_unpoison_remaining_stack(void *sp)
+/*
+ * Unpoison stack starting from the current sp to the given watermark sp value.
+ * Works for task, irq, nmi, etc stacks.
+ */
+asmlinkage void kasan_unpoison_remaining_stack(const void *sp)
{
- __kasan_unpoison_stack(current, sp);
+ const void *base;
+ size_t size;
+
+ if ((char *)&sp > (char *)sp) {
+ base = sp;
+ size = (char *)&sp - (char *)sp;
+ } else {
+ base = &sp;
+ size = (char *)sp - (char *)&sp;
+ }
+ kasan_unpoison_shadow(base, size);
}

/*
--
2.8.0.rc3.226.g39d4020

[Dmitry Vyukov]

On Tue, Oct 11, 2016 at 2:59 PM, Dmitry Vyukov <dvy...@google.com> wrote:
> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().
>
> Also change kasan_unpoison_remaining_stack() to not assume that
> we are on a normal task stack, as jprobes can be used in interrupts.

Mark,

This changes kasan_unpoison_remaining_stack() that you introduced in:

commit 0d97e6d8024c71cc838b292c01d5bd951e080eba
Date: Wed Mar 9 14:08:21 2016 -0800
arm64: kasan: clear stale stack poison

I would expect that this new version still works for the case you handled.
But please double check. I also asked Alexander to retest this patch.

[Mark Rutland]

Hi,

On Tue, Oct 11, 2016 at 02:59:42PM +0200, Dmitry Vyukov wrote:
> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().
>
> Also change kasan_unpoison_remaining_stack() to not assume that
> we are on a normal task stack, as jprobes can be used in interrupts.

Unfortunately this specific change is incorrect, and will break arm64.

More details below; hopefully we can solve this without too much pain.

Taking the location of the sp parameter (i.e. &sp) is bogus. It *is not* the
base (which should be the lowest possible address the stack can have, i.e. the
base of the region allocated for use as the stack).

Depending on calling convention, that will be a location in the current stack
frame, or somewhere below it, and there could be poison for the remaining stack
beyond that.

As is, this will almost certainly break arm64's return-from-idle unpoisoning;
the path to idle is likely to use much more of the stack than the code above.
I think the same is true in the general case for jprobes, given a probe
function that uses sufficient stack.

To fix this correctly, we need to be able to find the base of the stack region
for a given SP -- we probably need an arch-specific callback for that. Either
that, or we need to be able to pass that explicitly from the caller.

Thanks,
Mark.

[Dmitry Vyukov]

Hi,

Are you sure that in the arm64's return-from-idle case there can be
poisoned frames below sp? How does that happen.
It should not be the case for jprobes and for any other longjmp-like
functions. When we get to jprobe_return, there are no frames below it
because it is the current leaf. There are also no poisoned redzones
below it, because normal execution flow does not leave poisoned
redzones and any other longjmp-like functions should have been already
fixed redzones (if that would not be the case, we are already in
trouble). Now the jprobe_return frame itself can contain poisoned
redzones, and if we take address of a local in it there can be
poisoned redzone below it (redzones are on both sides of a local). But
when we call kasan_unpoison_remaining_stack, sp is adjusted past all
poisoned redzones and kasan_unpoison_remaining_stack's frame is not
poisoned. So when we take address of a local in
kasan_unpoison_remaining_stack, it must be below all poisoned
redzones.
What am I missing?

[Mark Rutland]

Hi,

Yes. This is practically certain.

> How does that happen.

When we call (PSCI) idle, we save some state, then call C functions to go
entire the FW. We have a callstack like:

psci_cpu_suspend_enter()
-> cpu_suspend()
-> __cpu_suspend_enter() ; asm - stack (i.e. sp) snapshotted here
-> psci_suspend_finisher()
-> psci_cpu_suspend()
-> invoke_psci_fn()
-> arm_smccc_smc() ; asm - calls FW

... when we return (cold) to the kernel, we return to an asm function at the
same level as __cpu_suspend_enter. While &sp will be below *sp, &sp is not
necessarily below the stackframe for invoke_psci_fn, for example.

As we have no guarantees as to how large the stackframes are for those
functions, we clear the maximum possible, using task_stack_page(task) as the
base.

> It should not be the case for jprobes and for any other longjmp-like
> functions. When we get to jprobe_return, there are no frames below it
> because it is the current leaf. There are also no poisoned redzones
> below it, because normal execution flow does not leave poisoned
> redzones and any other longjmp-like functions should have been already
> fixed redzones (if that would not be the case, we are already in
> trouble). Now the jprobe_return frame itself can contain poisoned
> redzones, and if we take address of a local in it there can be
> poisoned redzone below it (redzones are on both sides of a local). But
> when we call kasan_unpoison_remaining_stack, sp is adjusted past all
> poisoned redzones and kasan_unpoison_remaining_stack's frame is not
> poisoned. So when we take address of a local in
> kasan_unpoison_remaining_stack, it must be below all poisoned
> redzones.

Ok, I think the x86 jprobes case would work, then.

As above, this is will regress arm64, however.

Please introduce a distinct stack poison clearing function to cater for the x86
jprobes case, that clearly defines its intent, e.g.

/*
* Clear all poison for the region between the current SP and a provided
* watermark value, as is sometimes required prior to hand-crafted asm function
* returns in the middle of functions.
*/
kasan_unpoison_stack_above_sp_to(const void *watermark)
{
const void *sp = (void *)current_stack_pointer();
size_t size = watermark - sp;

kasan_unpoison_shadow(sp, size);
}

I'm more than happy for kasan_unpoison_remaining_stack to be renamed to more
clearly express its intended use, e.g:

kasan_unpoison_task_stack_below(const void *watermark).

Thanks,
Mark.

[Dmitry Vyukov]

I see. It is even more special that longjmp.
Thanks for explaining.

>> It should not be the case for jprobes and for any other longjmp-like
>> functions. When we get to jprobe_return, there are no frames below it
>> because it is the current leaf. There are also no poisoned redzones
>> below it, because normal execution flow does not leave poisoned
>> redzones and any other longjmp-like functions should have been already
>> fixed redzones (if that would not be the case, we are already in
>> trouble). Now the jprobe_return frame itself can contain poisoned
>> redzones, and if we take address of a local in it there can be
>> poisoned redzone below it (redzones are on both sides of a local). But
>> when we call kasan_unpoison_remaining_stack, sp is adjusted past all
>> poisoned redzones and kasan_unpoison_remaining_stack's frame is not
>> poisoned. So when we take address of a local in
>> kasan_unpoison_remaining_stack, it must be below all poisoned
>> redzones.
>
> Ok, I think the x86 jprobes case would work, then.
>
> As above, this is will regress arm64, however.
>
> Please introduce a distinct stack poison clearing function to cater for the x86

That was my fallback plan to introduce another functions. But I wanted
to check if we can manage without bloating API.
Will add another function.

> jprobes case, that clearly defines its intent, e.g.
>
> /*
> * Clear all poison for the region between the current SP and a provided
> * watermark value, as is sometimes required prior to hand-crafted asm function
> * returns in the middle of functions.
> */
> kasan_unpoison_stack_above_sp_to(const void *watermark)
> {
> const void *sp = (void *)current_stack_pointer();
> size_t size = watermark - sp;
>
> kasan_unpoison_shadow(sp, size);
> }
>
> I'm more than happy for kasan_unpoison_remaining_stack to be renamed to more
> clearly express its intended use, e.g:
>
> kasan_unpoison_task_stack_below(const void *watermark).

Will do.

[Dmitry Vyukov]

KASAN stack instrumentation poisons stack redzones on function entry
and unpoisons them on function exit. If a function exits abnormally
(e.g. with a longjmp like jprobe_return()), stack redzones are left
poisoned. Later this leads to random KASAN false reports.

Unpoison stack redzones in the frames we are going to jump over
before doing actual longjmp in jprobe_return().

Signed-off-by: Dmitry Vyukov <dvy...@google.com>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Alexander Potapenko <gli...@google.com>
Cc: Will Deacon <will....@arm.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Masami Hiramatsu <mhir...@kernel.org>
Cc: x...@kernel.org
Cc: kasa...@googlegroups.com

--

Changes since v1:
- leave kasan_unpoison_remaining_stack() intact
- instead add kasan_unpoison_stack_above_sp_to()
- rename kasan_unpoison_remaining_stack() to kasan_unpoison_task_stack_below()

arch/arm64/kernel/sleep.S | 2 +-

arch/x86/kernel/kprobes/core.c | 4 ++++
include/linux/kasan.h | 2 ++

mm/kasan/kasan.c | 19 +++++++++++++++++--
4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index b8799e7..1bec41b 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -135,7 +135,7 @@ ENTRY(_cpu_resume)

#ifdef CONFIG_KASAN
mov x0, sp
- bl kasan_unpoison_remaining_stack
+ bl kasan_unpoison_task_stack_below
#endif

ldp x19, x20, [x29, #16]
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 28cee01..22a462a 100644

--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -50,6 +50,7 @@
#include <linux/kallsyms.h>
#include <linux/ftrace.h>
#include <linux/frame.h>
+#include <linux/kasan.h>

#include <asm/text-patching.h>
#include <asm/cacheflush.h>
@@ -1080,6 +1081,9 @@ void jprobe_return(void)
{
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();

+ /* Unpoison stack redzones in the frames we are going to jump over. */
+ kasan_unpoison_stack_above_sp_to(kcb->jprobe_saved_sp);

+
asm volatile (
#ifdef CONFIG_X86_64
" xchg %%rbx,%%rsp \n"
diff --git a/include/linux/kasan.h b/include/linux/kasan.h

index d600303..820c0ad 100644

--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -44,6 +44,7 @@ static inline void kasan_disable_current(void)
void kasan_unpoison_shadow(const void *address, size_t size);

void kasan_unpoison_task_stack(struct task_struct *task);

+void kasan_unpoison_stack_above_sp_to(const void *watermark);

void kasan_alloc_pages(struct page *page, unsigned int order);
void kasan_free_pages(struct page *page, unsigned int order);
@@ -85,6 +86,7 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
static inline void kasan_unpoison_shadow(const void *address, size_t size) {}

static inline void kasan_unpoison_task_stack(struct task_struct *task) {}

+static inline void kasan_unpoison_stack_above_sp_to(const void *watermark) {}

static inline void kasan_enable_current(void) {}
static inline void kasan_disable_current(void) {}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c

index 88af13c..45113b8 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -77,9 +77,24 @@ void kasan_unpoison_task_stack(struct task_struct *task)

}

/* Unpoison the stack for the current task beyond a watermark sp value. */
-asmlinkage void kasan_unpoison_remaining_stack(void *sp)

+asmlinkage void kasan_unpoison_task_stack_below(const void *watermark)
{
- __kasan_unpoison_stack(current, sp);
+ __kasan_unpoison_stack(current, watermark);
+}
+
+/*
+ * Clear all poison for the region between the current SP and a provided
+ * watermark value, as is sometimes required prior to hand-crafted asm function
+ * returns in the middle of functions.
+ */
+kasan_unpoison_stack_above_sp_to(const void *watermark)
+{
+ const void *sp = (void *)current_stack_pointer();
+ size_t size = watermark - sp;
+
+ if (WARN_ON(sp > watermark))
+ return;
+ kasan_unpoison_shadow(sp, size);
}

/*
--
2.8.0.rc3.226.g39d4020

[Masami Hiramatsu]

On Thu, 13 Oct 2016 12:11:51 +0200
Dmitry Vyukov <dvy...@google.com> wrote:

> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().

OK, this looks good to me.

Acked-by: Masami Hiramatsu <mhir...@kernel.org>

Thanks!

--
Masami Hiramatsu <mhir...@kernel.org>

[Dmitry Vyukov]

KASAN stack instrumentation poisons stack redzones on function entry
and unpoisons them on function exit. If a function exits abnormally
(e.g. with a longjmp like jprobe_return()), stack redzones are left
poisoned. Later this leads to random KASAN false reports.

Unpoison stack redzones in the frames we are going to jump over
before doing actual longjmp in jprobe_return().

Signed-off-by: Dmitry Vyukov <dvy...@google.com>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Alexander Potapenko <gli...@google.com>
Cc: Will Deacon <will....@arm.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Masami Hiramatsu <mhir...@kernel.org>
Cc: x...@kernel.org
Cc: kasa...@googlegroups.com

--

Changes since v1:
- leave kasan_unpoison_remaining_stack() intact
- instead add kasan_unpoison_stack_above_sp_to()
- rename kasan_unpoison_remaining_stack() to kasan_unpoison_task_stack_below()

Changes since v2:
- fix build by adding return type to kasan_unpoison_stack_above_sp_to
(tested v2 with it, but forgot to git add)

index 88af13c..52d7ff1 100644

--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -77,9 +77,24 @@ void kasan_unpoison_task_stack(struct task_struct *task)
}

/* Unpoison the stack for the current task beyond a watermark sp value. */
-asmlinkage void kasan_unpoison_remaining_stack(void *sp)
+asmlinkage void kasan_unpoison_task_stack_below(const void *watermark)
{
- __kasan_unpoison_stack(current, sp);
+ __kasan_unpoison_stack(current, watermark);
+}
+
+/*
+ * Clear all poison for the region between the current SP and a provided
+ * watermark value, as is sometimes required prior to hand-crafted asm function
+ * returns in the middle of functions.
+ */

+void kasan_unpoison_stack_above_sp_to(const void *watermark)

[Mark Rutland]

On Fri, Oct 14, 2016 at 12:53:56PM +0200, Dmitry Vyukov wrote:
> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().
>
> Signed-off-by: Dmitry Vyukov <dvy...@google.com>
> Cc: Mark Rutland <mark.r...@arm.com>
> Cc: Catalin Marinas <catalin...@arm.com>
> Cc: Andrey Ryabinin <ryabin...@gmail.com>
> Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
> Cc: Alexander Potapenko <gli...@google.com>
> Cc: Will Deacon <will....@arm.com>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: Andrew Morton <ak...@linux-foundation.org>
> Cc: Thomas Gleixner <tg...@linutronix.de>
> Cc: "H. Peter Anvin" <h...@zytor.com>
> Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
> Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
> Cc: "David S. Miller" <da...@davemloft.net>
> Cc: Masami Hiramatsu <mhir...@kernel.org>
> Cc: x...@kernel.org
> Cc: kasa...@googlegroups.com

Both the core and arm64 parts look right to me, so FWIW:

Reviewed-by: Mark Rutland <mark.r...@arm.com>

Catalin, Will, are you happy to ack the arm64 part? A core function got
renamed, and we have to update the call site in sleep.S, but there
should be no functional change. The rest of the patch adds some generic
infrastructure required by x86.

Thanks,
Mark.

[Will Deacon]

On Fri, Oct 14, 2016 at 12:53:56PM +0200, Dmitry Vyukov wrote:

I get build warnings with this patch applied and KASAN enabled:

mm/kasan/kasan.c: In function ‘kasan_unpoison_task_stack_below’:
mm/kasan/kasan.c:82:34: warning: passing argument 2 of ‘__kasan_unpoison_stack’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
__kasan_unpoison_stack(current, watermark);
^~~~~~~~~
mm/kasan/kasan.c:65:13: note: expected ‘void *’ but argument is of type ‘const void *’

static void __kasan_unpoison_stack(struct task_struct *task, void *sp)

^~~~~~~~~~~~~~~~~~~~~~
mm/kasan/kasan.c: In function ‘kasan_unpoison_stack_above_sp_to’:
mm/kasan/kasan.c:92:27: error: called object ‘current_stack_pointer’ is not a function or function pointer

const void *sp = (void *)current_stack_pointer();

^~~~~~~~~~~~~~~~~~~~~
In file included from ./include/linux/thread_info.h:54:0,
from ./include/asm-generic/preempt.h:4,
from ./arch/arm64/include/generated/asm/preempt.h:1,
from ./include/linux/preempt.h:59,
from ./include/linux/interrupt.h:8,
from mm/kasan/kasan.c:20:
./arch/arm64/include/asm/thread_info.h:69:24: note: declared here
register unsigned long current_stack_pointer asm ("sp");

Will

[Dmitry Vyukov]

KASAN stack instrumentation poisons stack redzones on function entry
and unpoisons them on function exit. If a function exits abnormally
(e.g. with a longjmp like jprobe_return()), stack redzones are left
poisoned. Later this leads to random KASAN false reports.

Unpoison stack redzones in the frames we are going to jump over
before doing actual longjmp in jprobe_return().

Signed-off-by: Dmitry Vyukov <dvy...@google.com>

Reviewed-by: Mark Rutland <mark.r...@arm.com>
Acked-by: Masami Hiramatsu <mhir...@kernel.org>

Cc: Mark Rutland <mark.r...@arm.com>
Cc: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Alexander Potapenko <gli...@google.com>
Cc: Will Deacon <will....@arm.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Masami Hiramatsu <mhir...@kernel.org>
Cc: x...@kernel.org
Cc: kasa...@googlegroups.com

--

Changes since v1:
- leave kasan_unpoison_remaining_stack() intact
- instead add kasan_unpoison_stack_above_sp_to()
- rename kasan_unpoison_remaining_stack() to kasan_unpoison_task_stack_below()

Changes since v2:

- fix build by adding return type to kasan_unpoison_stack_above_sp_to()

(tested v2 with it, but forgot to git add)

Changes since v3:
- fix build warning in kasan_unpoison_stack_above_sp_to() related to
void*/const void* conversion

---
arch/arm64/kernel/sleep.S | 2 +-
arch/x86/kernel/kprobes/core.c | 4 ++++
include/linux/kasan.h | 2 ++

mm/kasan/kasan.c | 21 ++++++++++++++++++---
4 files changed, 25 insertions(+), 4 deletions(-)

index 88af13c..4f0c98c 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -62,7 +62,7 @@ void kasan_unpoison_shadow(const void *address, size_t size)
}
}

-static void __kasan_unpoison_stack(struct task_struct *task, void *sp)
+static void __kasan_unpoison_stack(struct task_struct *task, const void *sp)
{
void *base = task_stack_page(task);

size_t size = sp - base;

[Dmitry Vyukov]

Mailed v4.

Thanks

[Mark Rutland]

On Fri, Oct 14, 2016 at 01:54:30PM +0200, Dmitry Vyukov wrote:
> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().
>
> Signed-off-by: Dmitry Vyukov <dvy...@google.com>
> Reviewed-by: Mark Rutland <mark.r...@arm.com>

... judging by the kbuild test robot I spoke too soon, and should have
been more thorough. :/

> +/*
> + * Clear all poison for the region between the current SP and a provided
> + * watermark value, as is sometimes required prior to hand-crafted asm function
> + * returns in the middle of functions.
> + */
> +void kasan_unpoison_stack_above_sp_to(const void *watermark)
> +{
> + const void *sp = (void *)current_stack_pointer();

Aargh; it seems current_stack_pointer() is only function-like on some
arches, and not on others (arm64 included). I should have known better;
sorry for the bad suggestion.

I'm not overjoyed about taking the address of a stack variable to
implement this ourselves. Can we use __builtin_frame_address(0) instead?
Or are there cases where that won't work on x86?

> + size_t size = watermark - sp;
> +
> + if (WARN_ON(sp > watermark))
> + return;

... not a new problem, but we should also include <linux/bug.h> for
WARN_ON().

Thanks,
Mark.

[Dmitry Vyukov]

KASAN stack instrumentation poisons stack redzones on function entry
and unpoisons them on function exit. If a function exits abnormally
(e.g. with a longjmp like jprobe_return()), stack redzones are left
poisoned. Later this leads to random KASAN false reports.

Unpoison stack redzones in the frames we are going to jump over
before doing actual longjmp in jprobe_return().

Signed-off-by: Dmitry Vyukov <dvy...@google.com>
Reviewed-by: Mark Rutland <mark.r...@arm.com>

Acked-by: Masami Hiramatsu <mhir...@kernel.org>

Cc: Mark Rutland <mark.r...@arm.com>
Cc: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Alexander Potapenko <gli...@google.com>
Cc: Will Deacon <will....@arm.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.kes...@intel.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Masami Hiramatsu <mhir...@kernel.org>
Cc: x...@kernel.org
Cc: kasa...@googlegroups.com

---

Changes since v1:
- leave kasan_unpoison_remaining_stack() intact
- instead add kasan_unpoison_stack_above_sp_to()
- rename kasan_unpoison_remaining_stack() to kasan_unpoison_task_stack_below()

Changes since v2:

- fix build by adding return type to kasan_unpoison_stack_above_sp_to()

(tested v2 with it, but forgot to git add)

Changes since v3:
- fix build warning in kasan_unpoison_stack_above_sp_to() related to
void*/const void* conversion

Changes since v4:
- use __builtin_frame_address(0) instead of current_stack_pointer().
The latter is x86-specific.
- add <linux/bug.h> include for WARN_ON

mm/kasan/kasan.c | 22 +++++++++++++++++++---
4 files changed, 26 insertions(+), 4 deletions(-)

index 88af13c..70c0097 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -34,6 +34,7 @@
#include <linux/string.h>
#include <linux/types.h>
#include <linux/vmalloc.h>
+#include <linux/bug.h>

#include "kasan.h"
#include "../slab.h"
@@ -62,7 +63,7 @@ void kasan_unpoison_shadow(const void *address, size_t size)

}
}

-static void __kasan_unpoison_stack(struct task_struct *task, void *sp)
+static void __kasan_unpoison_stack(struct task_struct *task, const void *sp)
{
void *base = task_stack_page(task);
size_t size = sp - base;

@@ -77,9 +78,24 @@ void kasan_unpoison_task_stack(struct task_struct *task)

}

/* Unpoison the stack for the current task beyond a watermark sp value. */
-asmlinkage void kasan_unpoison_remaining_stack(void *sp)
+asmlinkage void kasan_unpoison_task_stack_below(const void *watermark)
{
- __kasan_unpoison_stack(current, sp);
+ __kasan_unpoison_stack(current, watermark);
+}
+

+/*
+ * Clear all poison for the region between the current SP and a provided
+ * watermark value, as is sometimes required prior to hand-crafted asm function
+ * returns in the middle of functions.
+ */
+void kasan_unpoison_stack_above_sp_to(const void *watermark)
+{

+ const void *sp = __builtin_frame_address(0);

+ size_t size = watermark - sp;
+
+ if (WARN_ON(sp > watermark))
+ return;

[Dmitry Vyukov]

Mailed v5 with __builtin_frame_address(0).
Built mm/kasan/kasan.o for arm64.

I see that __builtin_frame_address(0) is used on several arches
including x86 and arm64.
So I hope we are good here.

>
>> + size_t size = watermark - sp;
>> +
>> + if (WARN_ON(sp > watermark))
>> + return;
>
> ... not a new problem, but we should also include <linux/bug.h> for
> WARN_ON().

Done

[Ingo Molnar]

* Dmitry Vyukov <dvy...@google.com> wrote:

> KASAN stack instrumentation poisons stack redzones on function entry
> and unpoisons them on function exit. If a function exits abnormally
> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> poisoned. Later this leads to random KASAN false reports.
>
> Unpoison stack redzones in the frames we are going to jump over
> before doing actual longjmp in jprobe_return().

Does this affect any other architecture besides arm64? If not then it might make
the most sense to merge this via the arm64 tree.

Thanks,

Ingo

[Dmitry Vyukov]

This is mostly for x86_64. This patch fixes KASAN false positives
related to jprobe on x86_64.
Arm64 related part is only a function rename. As I introduce a
function similar to an existing one, Mark asked to me rename the
existing function to clarify the difference between the two.

[Ingo Molnar]

* Dmitry Vyukov <dvy...@google.com> wrote:

> On Sat, Oct 15, 2016 at 8:30 AM, Ingo Molnar <mi...@kernel.org> wrote:
> >
> > * Dmitry Vyukov <dvy...@google.com> wrote:
> >
> >> KASAN stack instrumentation poisons stack redzones on function entry
> >> and unpoisons them on function exit. If a function exits abnormally
> >> (e.g. with a longjmp like jprobe_return()), stack redzones are left
> >> poisoned. Later this leads to random KASAN false reports.
> >>
> >> Unpoison stack redzones in the frames we are going to jump over
> >> before doing actual longjmp in jprobe_return().
> >
> > Does this affect any other architecture besides arm64? If not then it might make
> > the most sense to merge this via the arm64 tree.
>
>
> This is mostly for x86_64. This patch fixes KASAN false positives
> related to jprobe on x86_64.

Indeed: I should have read the patch beyond the diffstat.

> Arm64 related part is only a function rename. As I introduce a
> function similar to an existing one, Mark asked to me rename the
> existing function to clarify the difference between the two.

Fair enough!

Thanks,

Ingo