Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On 08.02.2018 22:00, Matthew Wilcox wrote: > On Thu, Feb 08, 2018 at 11:20:26AM -0600, Josh Poimboeuf wrote: >> The patch description is confusing. It talks about "crappy drivers irq >> handlers when they access wrong memory on the stack". But if I >> understand correctly, the patch doesn't actually protect against that >> case, because irq handlers run on the irq stack, and this patch only >> affects code which *isn't* running on the irq stack. > > This would catch a crappy driver which allocates some memory on the > irq stack, squirrels the pointer to it away in a data structure, then > returns to process (or softirq) context and dereferences the pointer. Yes, this is exactly what I mean. The patch allows stack modifications for interrupt time, and catches wrong accesses from another contexts/cpus (when there is no interrupt executing in parallel). It's possible to catch wrong accesses in interrupt time also, but we need to unmap irq stacks on another cpus to do that, which is not KASAN thing. But, I hope we may be lucky and catch such situations even if we only check for accesses, which are going not in interrupt time. > I have no idea if that's the case that Kirill is tracking down, but it's > something I can imagine someone doing. Kirill
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On Thu, Feb 08, 2018 at 11:20:26AM -0600, Josh Poimboeuf wrote: > The patch description is confusing. It talks about "crappy drivers irq > handlers when they access wrong memory on the stack". But if I > understand correctly, the patch doesn't actually protect against that > case, because irq handlers run on the irq stack, and this patch only > affects code which *isn't* running on the irq stack. This would catch a crappy driver which allocates some memory on the irq stack, squirrels the pointer to it away in a data structure, then returns to process (or softirq) context and dereferences the pointer. I have no idea if that's the case that Kirill is tracking down, but it's something I can imagine someone doing.
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On Thu, Feb 08, 2018 at 05:41:19PM +0100, Dmitry Vyukov wrote: > On Thu, Feb 8, 2018 at 5:30 PM, Josh Poimboeuf wrote: > > On Thu, Feb 08, 2018 at 01:03:49PM +0300, Kirill Tkhai wrote: > >> On 07.02.2018 21:38, Dave Hansen wrote: > >> > 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? > >> > >> I can't say actual driver making this, because I'm still investigating the > >> guilty one. > >> But I have couple of crash dumps with the crash inside > >> update_sd_lb_stats() function, > >> where stack variable sg becomes corrupted. This time all scheduler-related > >> not-stack > >> variables are in ideal state. And update_sd_lb_stats() is the function, > >> which can't > >> corrupt its own stack. So, I thought this functionality may be useful for > >> something else, > >> especially because of irq stack is one of the last stacks, which are not > >> sanitized. > >> Task's stacks are already covered, as I know > >> > >> [1595450.678971] Call Trace: > >> [1595450.683991] > >> [1595450.684038] > >> [1595450.688926] [] cpumask_next_and+0x35/0x50 > >> [1595450.693984] [] find_busiest_group+0x143/0x950 > >> [1595450.699088] [] load_balance+0x19a/0xc20 > >> [1595450.704289] [] ? sched_clock_cpu+0x85/0xc0 > >> [1595450.709457] [] ? update_rq_clock.part.88+0x1a/0x150 > >> [1595450.714711] [] rebalance_domains+0x170/0x2b0 > >> [1595450.719997] [] run_rebalance_domains+0x122/0x1e0 > >> [1595450.725321] [] __do_softirq+0x10f/0x2aa > >> [1595450.730746] [] call_softirq+0x1c/0x30 > >> [1595450.736169] [] do_softirq+0x65/0xa0 > >> [1595450.741754] [] irq_exit+0x105/0x110 > >> [1595450.747279] [] smp_apic_timer_interrupt+0x42/0x50 > >> [1595450.752905] [] apic_timer_interrupt+0x232/0x240 > >> [1595450.758519] > >> [1595450.758569] > >> [1595450.764100] [] ? cpuidle_enter_state+0x52/0xc0 > >> [1595450.769652] [] cpuidle_idle_call+0xd8/0x210 > >> [1595450.775198] [] arch_cpu_idle+0xe/0x30 > >> [1595450.780813] [] cpu_startup_entry+0x14a/0x1c0 > >> [1595450.786286] [] start_secondary+0x1d6/0x250 > > > > I'm not seeing how this patch would help. If you're running on the irq > > stack, the *entire* irq stack would be unpoisoned. So there's still no > > KASAN protection. Or am I missing something? > > > > Seems like it would be more useful for KASAN to detect redzone accesses > > on the irq stack (if it's not doing that already). > > KASAN should do this already (unless there is something terribly > broken). Compiler instrumentation adds redzones around all stack > variables and injects code to poision/unpoison these redzones on > function entry/exit. > KASAN can also detect use-after-scope bugs for stack variables, but > this requires a more recent gcc (6 or 7, don't remember exactly now) > and CONFIG_KASAN_EXTRA since recently. > User-space ASAN can also detect so called use-after-return bugs > (dangling references to stack variables), but this requires manual > management of stack frames and quarantine for stack frames. This is > more tricky to do inside of kernel, so this was never implemented in > KASAN. KASAN still can detect some of these, if it will happen so that > the dangling reference happen to point to a redzone in a new frame. Ok, that's good. And it seems this patch doesn't change that. So it looks like the purpose of the patch is to protect the irq stack from code which is *not* running on the irq stack. Which seems a bit far-fetched and theoretical. Though I don't see any harm in it. The patch description is confusing. It talks about "crappy drivers irq handlers when they access wrong memory on the stack". But if I understand correctly, the patch doesn't actually protect against that case, because irq handlers run on the irq stack, and this patch only affects code which *isn't* running on the irq stack. -- Josh
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On Thu, Feb 8, 2018 at 5:30 PM, Josh Poimboeuf wrote: > On Thu, Feb 08, 2018 at 01:03:49PM +0300, Kirill Tkhai wrote: >> On 07.02.2018 21:38, Dave Hansen wrote: >> > 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? >> >> I can't say actual driver making this, because I'm still investigating the >> guilty one. >> But I have couple of crash dumps with the crash inside update_sd_lb_stats() >> function, >> where stack variable sg becomes corrupted. This time all scheduler-related >> not-stack >> variables are in ideal state. And update_sd_lb_stats() is the function, >> which can't >> corrupt its own stack. So, I thought this functionality may be useful for >> something else, >> especially because of irq stack is one of the last stacks, which are not >> sanitized. >> Task's stacks are already covered, as I know >> >> [1595450.678971] Call Trace: >> [1595450.683991] >> [1595450.684038] >> [1595450.688926] [] cpumask_next_and+0x35/0x50 >> [1595450.693984] [] find_busiest_group+0x143/0x950 >> [1595450.699088] [] load_balance+0x19a/0xc20 >> [1595450.704289] [] ? sched_clock_cpu+0x85/0xc0 >> [1595450.709457] [] ? update_rq_clock.part.88+0x1a/0x150 >> [1595450.714711] [] rebalance_domains+0x170/0x2b0 >> [1595450.719997] [] run_rebalance_domains+0x122/0x1e0 >> [1595450.725321] [] __do_softirq+0x10f/0x2aa >> [1595450.730746] [] call_softirq+0x1c/0x30 >> [1595450.736169] [] do_softirq+0x65/0xa0 >> [1595450.741754] [] irq_exit+0x105/0x110 >> [1595450.747279] [] smp_apic_timer_interrupt+0x42/0x50 >> [1595450.752905] [] apic_timer_interrupt+0x232/0x240 >> [1595450.758519] >> [1595450.758569] >> [1595450.764100] [] ? cpuidle_enter_state+0x52/0xc0 >> [1595450.769652] [] cpuidle_idle_call+0xd8/0x210 >> [1595450.775198] [] arch_cpu_idle+0xe/0x30 >> [1595450.780813] [] cpu_startup_entry+0x14a/0x1c0 >> [1595450.786286] [] start_secondary+0x1d6/0x250 > > I'm not seeing how this patch would help. If you're running on the irq > stack, the *entire* irq stack would be unpoisoned. So there's still no > KASAN protection. Or am I missing something? > > Seems like it would be more useful for KASAN to detect redzone accesses > on the irq stack (if it's not doing that already). KASAN should do this already (unless there is something terribly broken). Compiler instrumentation adds redzones around all stack variables and injects code to poision/unpoison these redzones on function entry/exit. KASAN can also detect use-after-scope bugs for stack variables, but this requires a more recent gcc (6 or 7, don't remember exactly now) and CONFIG_KASAN_EXTRA since recently. User-space ASAN can also detect so called use-after-return bugs (dangling references to stack variables), but this requires manual management of stack frames and quarantine for stack frames. This is more tricky to do inside of kernel, so this was never implemented in KASAN. KASAN still can detect some of these, if it will happen so that the dangling reference happen to point to a redzone in a new frame.
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On Thu, Feb 08, 2018 at 01:03:49PM +0300, Kirill Tkhai wrote: > On 07.02.2018 21:38, Dave Hansen wrote: > > 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? > > I can't say actual driver making this, because I'm still investigating the > guilty one. > But I have couple of crash dumps with the crash inside update_sd_lb_stats() > function, > where stack variable sg becomes corrupted. This time all scheduler-related > not-stack > variables are in ideal state. And update_sd_lb_stats() is the function, which > can't > corrupt its own stack. So, I thought this functionality may be useful for > something else, > especially because of irq stack is one of the last stacks, which are not > sanitized. > Task's stacks are already covered, as I know > > [1595450.678971] Call Trace: > [1595450.683991] > [1595450.684038] > [1595450.688926] [] cpumask_next_and+0x35/0x50 > [1595450.693984] [] find_busiest_group+0x143/0x950 > [1595450.699088] [] load_balance+0x19a/0xc20 > [1595450.704289] [] ? sched_clock_cpu+0x85/0xc0 > [1595450.709457] [] ? update_rq_clock.part.88+0x1a/0x150 > [1595450.714711] [] rebalance_domains+0x170/0x2b0 > [1595450.719997] [] run_rebalance_domains+0x122/0x1e0 > [1595450.725321] [] __do_softirq+0x10f/0x2aa > [1595450.730746] [] call_softirq+0x1c/0x30 > [1595450.736169] [] do_softirq+0x65/0xa0 > [1595450.741754] [] irq_exit+0x105/0x110 > [1595450.747279] [] smp_apic_timer_interrupt+0x42/0x50 > [1595450.752905] [] apic_timer_interrupt+0x232/0x240 > [1595450.758519] > [1595450.758569] > [1595450.764100] [] ? cpuidle_enter_state+0x52/0xc0 > [1595450.769652] [] cpuidle_idle_call+0xd8/0x210 > [1595450.775198] [] arch_cpu_idle+0xe/0x30 > [1595450.780813] [] cpu_startup_entry+0x14a/0x1c0 > [1595450.786286] [] start_secondary+0x1d6/0x250 I'm not seeing how this patch would help. If you're running on the irq stack, the *entire* irq stack would be unpoisoned. So there's still no KASAN protection. Or am I missing something? Seems like it would be more useful for KASAN to detect redzone accesses on the irq stack (if it's not doing that already). -- Josh
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On 07.02.2018 21:38, Dave Hansen wrote:
> 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?
I can't say actual driver making this, because I'm still investigating the
guilty one.
But I have couple of crash dumps with the crash inside update_sd_lb_stats()
function,
where stack variable sg becomes corrupted. This time all scheduler-related
not-stack
variables are in ideal state. And update_sd_lb_stats() is the function, which
can't
corrupt its own stack. So, I thought this functionality may be useful for
something else,
especially because of irq stack is one of the last stacks, which are not
sanitized.
Task's stacks are already covered, as I know
[1595450.678971] Call Trace:
[1595450.683991]
[1595450.684038]
[1595450.688926] [] cpumask_next_and+0x35/0x50
[1595450.693984] [] find_busiest_group+0x143/0x950
[1595450.699088] [] load_balance+0x19a/0xc20
[1595450.704289] [] ? sched_clock_cpu+0x85/0xc0
[1595450.709457] [] ? update_rq_clock.part.88+0x1a/0x150
[1595450.714711] [] rebalance_domains+0x170/0x2b0
[1595450.719997] [] run_rebalance_domains+0x122/0x1e0
[1595450.725321] [] __do_softirq+0x10f/0x2aa
[1595450.730746] [] call_softirq+0x1c/0x30
[1595450.736169] [] do_softirq+0x65/0xa0
[1595450.741754] [] irq_exit+0x105/0x110
[1595450.747279] [] smp_apic_timer_interrupt+0x42/0x50
[1595450.752905] [] apic_timer_interrupt+0x232/0x240
[1595450.758519]
[1595450.758569]
[1595450.764100] [] ? cpuidle_enter_state+0x52/0xc0
[1595450.769652] [] cpuidle_idle_call+0xd8/0x210
[1595450.775198] [] arch_cpu_idle+0xe/0x30
[1595450.780813] [] cpu_startup_entry+0x14a/0x1c0
[1595450.786286] [] start_secondary+0x1d6/0x250
>> 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.
It's touched in several more places (e.g., in __switch_to_asm()), and I'm not
sure KASAN is OK with this. Does it?
Also gs_base is touched from load_percpu_segment(), which could be called from
different cpu, and this seems it would required some synchronization between
the handlers and this primitive.
>> +#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
I haven't met NMI stack corrupted, so I don't have ideas about this. If
we need to check them too, one more patch should be introduced on top of
this.
Kirill
Re: [PATCH RFC] x86: KASAN: Sanitize unauthorized irq stack access
On Wed, Feb 7, 2018 at 7:38 PM, Dave Hansen wrote:
> 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?
Correct.
kasan_poison/unpoison_shadow effectively memset separate "shadow"
memory range, which is then checked by memory accesses to understand
if it's OK to access corresponding memory.
>> +#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?
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?
