linux-kernel - Re: [PATCH 1/1] 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]

Message-ID: <CACT4Y+ZxZZbQYnBx_e1r1Y740nAkWzrHmkvN7gat32TgoK+Yiw@mail.gmail.com>
Date:   Wed, 11 Oct 2017 13:32:34 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Maninder Singh <maninder1.s@...sung.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        chris@...is-wilson.co.uk, LKML <linux-kernel@...r.kernel.org>,
        PANKAJ MISHRA <pankaj.m@...sung.com>, a.sahrawat@...sung.com,
        Vaneet Narang <v.narang@...sung.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 1/1] stackdepot: ignore junk last entry in case of switch
 from user mode.

On Wed, Oct 11, 2017 at 1:31 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> On Wed, Oct 11, 2017 at 1:24 PM, Maninder Singh <maninder1.s@...sung.com> wrote:
>> Issue observed on ARM.
>>
>> Whenever there is switch from user mode, we end up with invalid last entry
>> with some user space address as below:-
>>
>>  save_stack+0x40/0xec
>>  __set_page_owner+0x2c/0x64
>> ....
>> ....
>>  __handle_domain_irq+0x9c/0x130
>>  gic_handle_irq+0x40/0x80
>>  __irq_usr+0x4c/0x60
>>  0xb6507818
>>
>> So in this case last entry is not valid, which leads to allocated one more
>> new frame for stackdepot although having all above frames exactly same.
>>
>> (It increases depot_index drastically)
>>
>> So its better to ignore that last frame in case of switch.
>>  save_stack+0x40/0xec
>>  __set_page_owner+0x2c/0x64
>> ....
>> ....
>>  __handle_domain_irq+0x9c/0x130
>>  gic_handle_irq+0x40/0x80
>>  __irq_usr+0x4c/0x60
>>
>> Signed-off-by: Vaneet Narang <v.narang@...sung.com>
>> Signed-off-by: Maninder Singh <maninder1.s@...sung.com>
>> ---
>>  lib/stackdepot.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>> index f87d138..bb35b2c 100644
>> --- a/lib/stackdepot.c
>> +++ b/lib/stackdepot.c
>> @@ -214,6 +214,11 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>>         if (unlikely(trace->nr_entries == 0))
>>                 goto fast_exit;
>>
>> +       if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {
>
> I agree with general approach. But isn't kernel text below
> MODULES_VADDR on e.g. x86_64?
>
>> +               trace->entries[trace->nr_entries - 1] = ULONG_MAX;
>
> Do we need this?
>
>> +               trace->nr_entries--;
>> +       }
>> +
>>         hash = hash_stack(trace->entries, trace->nr_entries);
>>         bucket = &stack_table[hash & STACK_HASH_MASK];


+kasan-dev mailing list