Re: [Xen-devel] [PATCH 4/4] x86: switch default mapping attributes to non-executable

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.15 at 20:53, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/05/15 13:47, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -895,6 +895,33 @@ void __init subarch_init_memory(void)
>>              share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
>>          }
>>      }
>> +
>> +    /* Mark low 16Mb of direct map NX if hardware supports it. */
>> +    if ( !cpu_has_nx )
>> +        return;
>> +
>> +    v = DIRECTMAP_VIRT_START + (1UL << 20);
> 
> What about 0-1MB ?

I'd like to avoid fiddling with that range, as the trampoline living there
will want to continue to be RWX anyway.

>> +    l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[l3_table_offset(v)];
>> +    ASSERT(l3e_get_flags(l3e) & _PAGE_PRESENT);
>> +    do {
>> +        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
>> +        ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
>> +        if ( l2e_get_flags(l2e) & _PAGE_PSE )
>> +        {
>> +            l2e_add_flags(l2e, _PAGE_NX_BIT);
>> +            l3e_to_l2e(l3e)[l2_table_offset(v)] = l2e;
>> +            v += 1 << L2_PAGETABLE_SHIFT;
>> +        }
>> +        else
>> +        {
>> +            l1_pgentry_t l1e = l2e_to_l1e(l2e)[l1_table_offset(v)];
>> +
>> +            ASSERT(l1e_get_flags(l1e) & _PAGE_PRESENT);
>> +            l1e_add_flags(l1e, _PAGE_NX_BIT);
>> +            l2e_to_l1e(l2e)[l1_table_offset(v)] = l1e;
>> +            v += 1 << L1_PAGETABLE_SHIFT;
>> +        }
>> +    } while ( v < DIRECTMAP_VIRT_START + (16UL << 20) );
> 
> How about just setting l3e.nx and leaving all lower tables alone?

Apart from the trampoline aspect (see above) I don't think it's a
good idea to restrict permissions in non-leaf entries - that just
calls for more intrusive patches if later an individual page needs
them lifted.

>> --- a/xen/include/asm-x86/x86_64/page.h
>> +++ b/xen/include/asm-x86/x86_64/page.h
>> @@ -147,9 +147,20 @@ typedef l4_pgentry_t root_pgentry_t;
>>   */
>>  #define _PAGE_GUEST_KERNEL (1U<<12)
>>  
>> -#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>> +#define PAGE_HYPERVISOR_RO      (__PAGE_HYPERVISOR_RO      | _PAGE_GLOBAL)
>> +#define PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RW      | _PAGE_GLOBAL)
>>  #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
>> -#define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
>> +#define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
>> +
>> +#ifdef __ASSEMBLY__
>> +/* Dependency on NX being available can't be expressed. */
>> +# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
>> +# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
>> +#else
>> +# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
>> +# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | \
>> +                                  _PAGE_GLOBAL | _PAGE_NX)
>> +#endif
> 
> This patch is clearly an improvement, so my comments can possibly be
> deferred to subsequent patches.
> 
> After boot, I can't think of a legitimate reason for Xen to have a RWX
> mapping.  As such, leaving __PAGE_HYPERVISOR around constitutes a risk
> that RWX mappings will be used.  It would be nice if we could remove all
> constants (which aren't BOOT_*) which could lead to accidental use of
> RWX mappings on NX-enabled hardware.

But you realize that __PAGE_HYPERVISOR is particularly used for
intermediate page table entry permissions?

> I think we also want a warning on boot if we find NX unavailable.  The
> only 64bit capable CPUs which do not support NX are now 11 years old,
> and I don't expect many of those to be running Xen these days.  However,
> given that "disable NX" is a common BIOS option for compatibility
> reasons, we should press people to alter their settings if possible.

Sure, easily done.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel