linux-kernel - Re: [PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Oct 2017 15:59:58 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Vaneet narang <v.narang@...sung.com>
Cc:     Maninder Singh <maninder1.s@...sung.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "chris@...is-wilson.co.uk" <chris@...is-wilson.co.uk>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        PANKAJ MISHRA <pankaj.m@...sung.com>,
        AMIT SAHRAWAT <a.sahrawat@...sung.com>
Subject: Re: [PATCH v2] stackdepot: ignore junk last entry in case of switch
 from user mode.

On Thu, Oct 12, 2017 at 1:10 PM, Vaneet Narang <v.narang@...sung.com> wrote:
> Hi Dmitry,
>
>>diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>>index f87d138..a045748 100644
>>--- a/lib/stackdepot.c
>>+++ b/lib/stackdepot.c
>>@@ -214,6 +214,13 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>>       if (unlikely(trace->nr_entries == 0))
>>               goto fast_exit;
>>
>>+      /*
>>+       * Ignore last entry if it belongs to user space
>>+       * in case of switch from user mode.
>>+       */
>>+      if (!kernel_text_address(trace->entries[trace->nr_entries - 1]))
>
> can we also add this check here:-
> if (!kernel_text_address(trace->entries[trace->nr_entries - 1]) || in_interrupt())
>
> Because on further checking , last frame in case of svc to interrupt mode is some random core kernel / Kernel Module address
> like below:-
>
> irq_exit+0xe4/0x140
> __handle_domain_irq+0x9c/0x130
> gic_handle_irq+0x40/0x80
> __irq_svc+0x44/0x7c
> hdmi_ocm_rx_get_scdc_sts+0x160/0x170 [sdp_hdmi]   >> we need this frme??
>
> This generates mismatch with existing entires and new entries get created and depot_index
> value keep on increasing slowly and reaches max value (STACK_ALLOC_SLABS_CAP) but after this change
> depot_index gets a stable value after some time. (Tested on ARM)


I think you need to piggy-back on irq stack support in kasan. See
filter_irq_stacks in mm/kasan/kasan.c. If you put __irq_svc (and
potentially other irq entry points) into irq/softirq section, then
they will be filtered automatically.
Also note that at least on x86 stack unwinder unwinds whole irq stack
and then whole kernel thread stack, so generally we need to filter not
just the last entry, but everything below irq entry point.

And I think this whole change need to be moved into kasan's save_stack
function. There is already a similar workarounds for x86. Currently
stackdepot merely memorizes the input data, regardless of what it is
(theoretically, not necessary even a stack trace).