[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2 of 2 V3] x86/kexec: Change NMI and MCE handling on kexec path
On 10/12/12 09:32, Jan Beulich wrote: On 07.12.12 at 22:18, Andrew Cooper<andrew.cooper3@xxxxxxxxxx> wrote:On 07/12/2012 11:52, Jan Beulich wrote:On 06.12.12 at 22:42, Andrew Cooper<andrew.cooper3@xxxxxxxxxx> wrote:+ + /* Change NMI trap handlers. Non-crashing pcpus get nmi_crash which + * invokes do_nmi_crash (above), which cause them to write state and + * fall into a loop. The crashing pcpu gets the nop handler to + * cause it to return to this function ASAP. + */ + for ( i = 0; i< nr_cpu_ids; ++i ) + if ( idt_tables[i] ) + { + + if ( i == cpu ) + { + /* Disable the interrupt stack tables for this MCE and + * NMI handler (shortly to become a nop) as there is a 1 + * instruction race window where NMIs could be + * re-enabled and corrupt the exception frame, leaving + * us unable to continue on this crash path (which half + * defeats the point of using the nop handler in the + * first place). + * + * This update is safe from a security point of view, as + * this pcpu is never going to try to sysret back to a + * PV vcpu. + */ + set_ist(&idt_tables[i][TRAP_nmi], IST_NONE); + set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE); + + _set_gate(&idt_tables[i][TRAP_nmi], 14, 0,&trap_nop);This makes the first set_ist() above pointless, doesn't it?No. The set_ist() is to remove the possibility of stack corruption from reentrant NMIs, while the trap_nop handler is so we don't get diverted into the regular NMI handler. There is nothing the NMI handler would do which could alter the outcome, and there are many cases where the regular NMI handler would try to panic, starting us reentrantly on the kexec path again (where we would deadlock on the one_cpu_only() check).I think you didn't get the point of the question: _set_gate() clears the IST field of the descriptor anyway, so why clear it separately first, and then overwrite it again? Ah - my apologies. I indeed was not understanding the point.I will need to fix up the calls then. Having _set_gate() change the IST as well reintroduces the security vulnerability. I will create a new function similar to _set_gate() which only changes the handler and nothing else. + } + else + _set_gate(&idt_tables[i][TRAP_nmi], 14, 0,&nmi_crash); + } + /* Ensure the new callback function is set before sending out the NMI.*/wmb(); ... +/* Enable NMIs. No special register assumptions, and all preserved. */ +ENTRY(enable_nmis) + pushq %raxWhat's the point of saving %rax here, btw? JanBecause at the moment I believe I might need to call it from asm context, when doing some of the later fixes. I figured that it was better to make it safe now, rather than patch it up later.I don't think that's good practice - if you end up not calling the thing from assembly code, the question on the purpose of saving %rax will re-surface sooner or later. Plus the patch by itself wouldn't really explain either why this is so (which might be of interest in the context of backporting it to older trees). Jan Ok - will remove. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |