Re: [PATCH v4] kprobes: unpoison stack in jprobe_return() for KASAN
On Fri, Oct 14, 2016 at 3:08 PM, Mark Rutland wrote:
> 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
>> Reviewed-by: Mark Rutland
>
> ... 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?
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 for
> WARN_ON().
Done
Re: [PATCH v4] kprobes: unpoison stack in jprobe_return() for KASAN
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
> Reviewed-by: Mark Rutland
... 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 for
WARN_ON().
Thanks,
Mark.
[PATCH v4] kprobes: unpoison stack in jprobe_return() for KASAN
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 Reviewed-by: Mark Rutland Acked-by: Masami Hiramatsu Cc: Mark Rutland Cc: Catalin Marinas Cc: Andrey Ryabinin Cc: Lorenzo Pieralisi Cc: Alexander Potapenko Cc: Will Deacon Cc: Ingo Molnar Cc: Andrew Morton Cc: Thomas Gleixner Cc: "H. Peter Anvin" Cc: Ananth N Mavinakayanahalli Cc: Anil S Keshavamurthy Cc: "David S. Miller" Cc: Masami Hiramatsu Cc: x...@kernel.org Cc: kasan-...@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 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 88005e48f480 [ 376.509522] Read of size 1 by task syz-executor/18535 [ 376.515249] page:ea00017923c0 count:0 mapcount:0 mapping: (null) index:0x0 [ 376.524377] flags: 0x1fffc00() [ 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] 88005e48f2d0 82d2b849 0bc91e90 fbfff10971e8 [ 376.561599] ed000bc91e90 ed000bc91e90 0001 [ 376.570486] 88005e48f480 88005e48f350 817d3169 88005e48f370 [ 376.579348] Call Trace: [ 376.582196] [] dump_stack+0x12e/0x185 [ 376.588190] [] kasan_report+0x489/0x4b0 [ 376.594378] [] ? memcmp+0xe9/0x150 [ 376.600099] [] ? update_stack_state.constprop.4+0xde/0x150 [ 376.608187] [] __asan_report_load1_noabort+0x19/0x20 [ 376.615620] [] memcmp+0xe9/0x150 [ 376.621148] [] depot_save_stack+0x176/0x5c0 [ 376.627722] [] ? skb_free_head+0x79/0xb0 [ 376.634006] [] save_stack+0xb1/0xd0 [ 376.639805] [] ? save_stack_trace+0x1b/0x20 [ 376.646369] [] ? save_stack+0x46/0xd0 [ 376.652366] [] ? kasan_slab_free+0x72/0xc0 [ 376.658837] [] ? kfree+0xc8/0x2a0 [ 376.664454] [] ? skb_free_head+0x79/0xb0 [ 376.670736] [] ? skb_release_data+0x37a/0x420 [ 376.677517] [] ? skb_release_all+0x4f/0x60 [ 376.683992] [] ? consume_skb+0x138/0x370 [ 376.690284] [] ? sctp_chunk_put+0xcb/0x180 [ 376.696761] [] ? sctp_chunk_free+0x58/0x70 [ 376.703234] [] ? sctp_inq_pop+0x68f/0xef0 [ 376.709616] [] ? sctp_assoc_bh_rcv+0xd6/0x4b0 [ 376.716379] [] ? sctp_inq_push+0x131/0x190 [ 376.722946] [] ? sctp_backlog_rcv+0xe9/0xa20 [ 376.729615] [] ? __release_sock+0x12c/0x3a0 [ 376.736186] [] ? release_sock+0x5e/0x1c0 [ 376.742465] [] ? sctp_sendmsg+0xd82/0x2e00 [ 376.748935] [] ? inet_sendmsg+0x303/0x4c0 [ 376.755318] [] ? sock_sendmsg+0xcf/0x110 [ 376.761599] [] ? SYSC_sendto+0x20d/0x340 [ 376.767882] [] ? SyS_sendto+0x45/0x60 [ 376.773885] [] ? do_syscall_64+0x1d3/0x620 [ 376.780372] [] ? entry_SYSCALL64_slow_path+0x25/0x25 [ 376.787832] [] ? sctp_do_sm+0x3689/0x4e90 [ 376.794215] [] ? debug_check_no_locks_freed+0x3c0/0x3c0 [ 376.801948] [] ? sctp_do_8_2_transport_strike.isra.19+0x900/0x900 [ 376.810918] [] ? debug_check_no_locks_freed+0x250/0x3c0 [ 376.818650] [] kasan_slab_free+0x72/0xc0 [ 376.824945] [] kfree+0xc8/0x2a0 [ 376.830374] [] skb_free_head+0x79/0xb0 [ 376.836466] [] skb_release_data+0x37a/0x420 [ 376.843036] [] skb_release_all+0x4f/0x60 [ 376.849319] [] consume_skb+0x138/0x370 [ 376.855411] [] sctp_chunk_put+0xcb/0x180 [ 376.861702] [] sctp_chunk_free+0x58/0x70 [ 376.867990] [] sctp_inq_pop+0x68f/0xef0 [ 376.874182] [] sctp_assoc_bh_rcv+0xd6/0x4b0 [ 376.880759] [] sctp_inq_push+0x131/0x190 [ 376.887055] [] sctp_backlog_rcv+0xe9/0xa20 [ 376.893532] [] ? trace_hardirqs_on+0xd/0x10 [ 376.900107] [] ? __local_bh_enable_ip+0xad/0x190 [ 376.907161] [] __release_sock+0x12c/0x3a0 [ 376.913540] [] release_sock+0x5e/0x1c0 [ 376.919922] [] sctp_sendmsg+0xd82/0x2e00 [ 376.926209] [] ? sctp_id2assoc+0x330/0x330 [ 376.932696] [] ? debug_check_no_locks_freed+0x3c0/0x3c0 [ 376.940417] [] ? debug_lockdep_rcu_enabled+0x77/0x90 [ 376.947858
