Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings

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

On 03/05/17 09:49, Jan Beulich wrote:
>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
>> where the guest has full read access and a lot of direct or indirect control
>> over the written content.  It isn't hard for a PV guest to hide shellcode
>> here.
>>
>> Therefore, increase defence in depth by auditing our current pagetable
>> mappings.
>>
>>  * The regular linear, shadow linear, and per-domain slots have no business
>>    being executable (but need to be written), so are updated to be NX.
>>  * The Read Only mappings of the M2P (compat and regular) don't need to be
>>    writeable or executable.
>>  * The PV GDT mappings don't need to be executable.
>>
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two remarks:
>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -384,7 +384,7 @@ void __init arch_init_memory(void)
>>                      for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
>>                          l3tab[i] = l3e_empty();
>>                      split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
>> -                                             __PAGE_HYPERVISOR);
>> +                                             __PAGE_HYPERVISOR_RW);
> Would be nice if this change (affecting the direct map) was also
> mentioned in the commit message, even if it's only debugging
> code.
>
>> @@ -515,7 +515,7 @@ void __init paging_init(void)
>>              l3_ro_mpt = page_to_virt(l3_pg);
>>              clear_page(l3_ro_mpt);
>>              l4e_write(&idle_pg_table[l4_table_offset(va)],
>> -                      l4e_from_page(l3_pg, __PAGE_HYPERVISOR));
>> +                      l4e_from_page(l3_pg, __PAGE_HYPERVISOR_RW));
> Similarly here (again affecting the direct map).

Updated.  The text now reads

 * The PV GDT mappings and bits of the directmap don't need to be
executable.

My method of working out which areas to change were to consider all uses
of __PAGE_HYPERVISOR.  I have half a mind to submit a change renaming it
to __PAGE_PGTABLE, as it should only really be used to build
intermediate pagetable entries where we control X/NX, R/W or S/U at a
more fine grained level.

~Andrew

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