linux-kernel - Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access

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:   Wed, 7 Feb 2018 10:38:35 -0800
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>, tglx@...utronix.de,
        mingo@...hat.com, hpa@...or.com, aryabinin@...tuozzo.com,
        glider@...gle.com, dvyukov@...gle.com, luto@...nel.org,
        bp@...en8.de, jpoimboe@...hat.com, jgross@...e.com,
        kirill.shutemov@...ux.intel.com, keescook@...omium.org,
        minipli@...glemail.com, gregkh@...uxfoundation.org,
        kstewart@...uxfoundation.org, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, linux-mm@...ck.org
Subject: Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access

On 02/07/2018 08:14 AM, Kirill Tkhai wrote:
> Sometimes it is possible to meet a situation,
> when irq stack is corrupted, while innocent
> callback function is being executed. This may
> happen because of crappy drivers irq handlers,
> when they access wrong memory on the irq stack.

Can you be more clear about the actual issue?  Which drivers do this?
How do they even find an IRQ stack pointer?

> This patch aims to catch such the situations
> and adds checks of unauthorized stack access.

I think I forgot how KASAN did this.  KASAN has metadata that says which
areas of memory are good or bad to access, right?  So, this just tags
IRQ stacks as bad when we are not _in_ an interrupt?

> +#define KASAN_IRQ_STACK_SIZE \
> +	(sizeof(union irq_stack_union) - \
> +		(offsetof(union irq_stack_union, stack_canary) + 8))

Just curious, but why leave out the canary?  It shouldn't be accessed
either.

> +#ifdef CONFIG_KASAN
> +void __visible x86_poison_irq_stack(void)
> +{
> +	if (this_cpu_read(irq_count) == -1)
> +		kasan_poison_irq_stack();
> +}
> +void __visible x86_unpoison_irq_stack(void)
> +{
> +	if (this_cpu_read(irq_count) == -1)
> +		kasan_unpoison_irq_stack();
> +}
> +#endif

It might be handy to point out here that -1 means "not in an interrupt"
and >=0 means "in an interrupt".

Otherwise, this looks pretty straightforward.  Would it be something to
extend to the other stacks like the NMI or double-fault stacks?  Or are
those just not worth it?