Re: [PATCH v3 7/9] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

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

On 28/04/14 10:40, Jan Beulich wrote:
>>>> On 28.04.14 at 05:17, <feng.wu@intel.com> wrote:
>> @@ -1394,6 +1398,15 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>                          bootstrap_map, cmdline) != 0)
>>          panic("Could not set up DOM0 guest OS");
>>  
>> +    /*
>> +     * Enable SMAP after constructing domain0, since there are lots of accesses to
>> +     * user pages in construct_dom0(), which is safe at the current stage.
>> +     */
>> +    if ( disable_smap )
>> +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
> You should not have moved this part - this should happen before APs
> get brought up.
>
>> @@ -1379,8 +1399,14 @@ void do_page_fault(struct cpu_user_regs *regs)
>>  
>>      if ( unlikely(!guest_mode(regs)) )
>>      {
>> -        pf_type = spurious_page_fault(addr, error_code);
>> -        BUG_ON(pf_type == smep_fault);
>> +        pf_type = spurious_page_fault(addr, regs);
>> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault) )
>> +        {
>> +            console_start_sync();
>> +            printk("Xen %s violation\n", (pf_type == smep_fault) ? "SMEP" : "SMAP");
> I know it's largely a matter of taste, but could I talk you into doing
>
>             printk("Xen SM%cP violation\n", (pf_type == smep_fault) ? 'E' : 'A');
>
> instead (unless others object)?

I concur.  Neither string is grepable.

>
>> @@ -1406,10 +1432,12 @@ void do_page_fault(struct cpu_user_regs *regs)
>>  
>>      if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
>>      {
>> -        pf_type = spurious_page_fault(addr, error_code);
>> -        if ( pf_type == smep_fault )
>> +        pf_type = spurious_page_fault(addr, regs);
>> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
>>          {
>> -            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
>> +            printk(XENLOG_G_ERR "%p fatal %s fault\n",
>> +                   current, (pf_type == smep_fault) ? "SMEP" : "SMAP");

That should be %pv rather than %p, which is an automagic format
parameter which will get you "d<domid>v<vcpuid>" formatted.

> And similarly here then?
>
> Jan
>

~Andrew