Re: [PATCH 3/3] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/02/15 11:02, Jan Beulich wrote:
>>>> On 10.02.15 at 18:12, <andrew.cooper3@citrix.com> wrote:
>> @@ -127,38 +128,26 @@ static void nmi_shootdown_cpus(void)
>>  
>>      cpumask_andnot(&waiting_to_crash, &cpu_online_map, cpumask_of(cpu));
>>  
>> -    /* 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.
>> +    /*
>> +     * Disable IST for MCEs to avoid stack corruption race conditions, and
>> +     * change the NMI hanlder to a nop to avoid deviation from this codepath.
> handler
>
>>       */
>> -    for ( i = 0; i < nr_cpu_ids; i++ )
>> -    {
>> -        if ( idt_tables[i] == NULL )
>> -            continue;
>> -
>> -        if ( i == cpu )
>> -        {
>> -            /*
>> -             * Disable the interrupt stack tables for this cpu's MCE and NMI 
>> -             * handlers, and alter the NMI handler to have no operation.  
>> -             * Disabling the stack tables prevents stack corruption race 
>> -             * conditions, while changing the handler helps prevent cascading 
>> -             * faults; we are certainly going to crash by this point.
>> -             *
>> -             * 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_gate_lower(&idt_tables[i][TRAP_nmi],
>> -                            SYS_DESC_irq_gate, 0, &trap_nop);
>> -            set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE);
>> -        }
>> -        else
>> -        {
>> -            /* Do not update stack table for other pcpus. */
>> -            _update_gate_addr_lower(&idt_tables[i][TRAP_nmi], &nmi_crash);
>> -        }
>> -    }
>> +    _set_gate_lower(&idt_tables[cpu][TRAP_nmi],
>> +                    SYS_DESC_irq_gate, 0, &trap_nop);
>> +    set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
>> +
>> +    /*
>> +     * Ideally would be:
>> +     *   exception_table[TRAP_nmi] = &do_nmi_crash;
>> +     *
>> +     * but the exception_table is read only.  Borrow and unused fixmap entry
> ... an unused ...
>
>> +     * to construct a writable mapping.
>> +     */
>> +    set_fixmap(FIX_TBOOT_MAP_ADDRESS, __pa(&exception_table[TRAP_nmi]));
>> +    write_atomic((unsigned long *)
>> +                 (fix_to_virt(FIX_TBOOT_MAP_ADDRESS) +
>> +                  ((unsigned long)&exception_table[TRAP_nmi] & ~PAGE_MASK)),
>> +                 (unsigned long)&do_nmi_crash);
> By converting the first cast to (void **) or even
> (typeof(do_nmi_crash) **) it would seem possible to drop the last
> cast altogether. While at it you could also drop the unnecessary &.

None of these casts can be dropped, because write_atomic() uses explicit
uint??_t casts.  Gcc objects because of -Werror=pointer-to-int-cast.

~Andrew