Re: [Xen-devel] [PATCH v2 for-4.10] x86/mm: Make PV linear pagetables optional

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/18/2017 02:41 PM, Jan Beulich wrote:
>>>> On 18.10.17 at 12:51, <george.dunlap@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -37,6 +37,26 @@ source "arch/Kconfig"
>>  config PV
>>      def_bool y
>>  
>> +config PV_LINEAR_PT
>> +       bool "Support for PV linear pagetables"
>> +       depends on PV
>> +       default y
>> +       ---help---
>> +         Linear pagetables (also called "recursive pagetables") refers
>> +         to the practice of a guest operating system having pagetable
>> +         entries pointing to other pagetables of the same level (i.e.,
>> +         allowing L2 PTEs to point to other L2 pages).  Some operating
>> +         systems use it as a simple way to consisently map the current
>> +         process's pagetables into its own virtual address space.
>> +
>> +         Linux and MiniOS don't use this technique.  NetBSD and Novell
>> +         Netware do; there may be other custom operating systems which
>> +         do.  If you're certain you don't plan on having PV guests
>> +         which use this feature, turning it off can reduce the attack
>> +         surface.
>> +
>> +         If unsure, say Y.
>> +
>>  config HVM
>>      def_bool y
> 
> Note how the options in context use tab indentation. Granted
> there are other examples of space indentation in this file, but
> at least they're using 8 spaces (except of course of the help
> text), while you're using 7.
> 
>> @@ -2320,6 +2353,7 @@ static int _put_page_type(struct page_info *page, bool 
>> preemptible,
>>                  break;
>>              }
>>  
>> +#ifdef CONFIG_PV_LINEAR_PT
>>              if ( ptpg && PGT_type_equal(x, ptpg->u.inuse.type_info) )
>>              {
>>                  /*
>> @@ -2334,6 +2368,9 @@ static int _put_page_type(struct page_info *page, bool 
>> preemptible,
>>                  ASSERT(ptpg->linear_pt_count > 0);
>>                  ptpg = NULL;
>>              }
>> +#else /* CONFIG_PV_LINEAR_PT */
>> +            BUG_ON(ptpg && PGT_type_equal(x, ptpg->u.inuse.type_info));
>> +#endif
> 
> Along the lines of my most recent reply to v1 (which I realize I
> did send only after v2 had arrived), I'm not really certain about
> the usefulness of the preprocessor conditionals - I'd prefer if
> we went without them, but I can live with them if you strongly
> think they're better than the alternative. If you keep them,
> please convert the BUG_ON() to ASSERT() though, to be in
> line with the #ifdef side.

I would argue that if linear pagetables are disabled, and we nonetheless
detect a linear pagetable, then BUG_ON() is the right behavior.  Since
we're not properly tracking any of it, it is almost certainly the result
of a security vulnerability.  Having a DoS in that case is much
preferrable to having a privilege escalation.

 -George

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