[PATCH 0/2] x86: XSA_214 follow-up

[PATCH 0/2] x86: XSA_214 follow-up

[Jan Beulich]

Some cleanup the potential for which was recognized while dealing
with that security issue.

1: limit page type width
2: don't allow clearing of TF_kernel_mode for other than 64-bit PV

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/2] x86: limit page type width

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3310 bytes --]

There's no reason to burn 4 bits on page type when we only have 7 types
(plus "none") at present. This requires changing one use of
PGT_shared_page, which so far assumed that the type is both a power of
2 and the only type with the high bit set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -452,7 +452,7 @@ static int audit(void)
         }
 
         /* Check if the MFN has correct type, owner and handle. */ 
-        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
+        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
         {
            MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
                               mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -176,16 +176,19 @@ struct page_info
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
  /* The following page types are MUTUALLY EXCLUSIVE. */
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_l1_page_table PG_mask(1, 4)  /* using as an L1 page table?     */
-#define PGT_l2_page_table PG_mask(2, 4)  /* using as an L2 page table?     */
-#define PGT_l3_page_table PG_mask(3, 4)  /* using as an L3 page table?     */
-#define PGT_l4_page_table PG_mask(4, 4)  /* using as an L4 page table?     */
-#define PGT_seg_desc_page PG_mask(5, 4)  /* using this page in a GDT/LDT?  */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
+#define PGT_none          PG_mask(0, 3)  /* no special uses of this page   */
+#define PGT_l1_page_table PG_mask(1, 3)  /* using as an L1 page table?     */
+#define PGT_l2_page_table PG_mask(2, 3)  /* using as an L2 page table?     */
+#define PGT_l3_page_table PG_mask(3, 3)  /* using as an L3 page table?     */
+#define PGT_l4_page_table PG_mask(4, 3)  /* using as an L4 page table?     */
+#define PGT_seg_desc_page PG_mask(5, 3)  /* using this page in a GDT/LDT?  */
+#define PGT_shared_page   PG_mask(6, 3)  /* CoW sharable page              */
+#define PGT_writable_page PG_mask(7, 3)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(7, 3)  /* Bits 61-63.                    */
 
+ /* Page is locked? */
+#define _PGT_locked       PG_shift(4)
+#define PGT_locked        PG_mask(1, 4)
  /* Owning guest has pinned this page to its current type? */
 #define _PGT_pinned       PG_shift(5)
 #define PGT_pinned        PG_mask(1, 5)
@@ -198,12 +201,9 @@ struct page_info
 /* Has this page been *partially* validated for use as its current type? */
 #define _PGT_partial      PG_shift(8)
 #define PGT_partial       PG_mask(1, 8)
- /* Page is locked? */
-#define _PGT_locked       PG_shift(9)
-#define PGT_locked        PG_mask(1, 9)
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(8)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */




[-- Attachment #2: x86-page-types-3-bits.patch --]
[-- Type: text/plain, Size: 3334 bytes --]

x86: limit page type width

There's no reason to burn 4 bits on page type when we only have 7 types
(plus "none") at present. This requires changing one use of
PGT_shared_page, which so far assumed that the type is both a power of
2 and the only type with the high bit set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -452,7 +452,7 @@ static int audit(void)
         }
 
         /* Check if the MFN has correct type, owner and handle. */ 
-        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
+        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
         {
            MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
                               mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -176,16 +176,19 @@ struct page_info
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
  /* The following page types are MUTUALLY EXCLUSIVE. */
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_l1_page_table PG_mask(1, 4)  /* using as an L1 page table?     */
-#define PGT_l2_page_table PG_mask(2, 4)  /* using as an L2 page table?     */
-#define PGT_l3_page_table PG_mask(3, 4)  /* using as an L3 page table?     */
-#define PGT_l4_page_table PG_mask(4, 4)  /* using as an L4 page table?     */
-#define PGT_seg_desc_page PG_mask(5, 4)  /* using this page in a GDT/LDT?  */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_shared_page   PG_mask(8, 4)  /* CoW sharable page              */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
+#define PGT_none          PG_mask(0, 3)  /* no special uses of this page   */
+#define PGT_l1_page_table PG_mask(1, 3)  /* using as an L1 page table?     */
+#define PGT_l2_page_table PG_mask(2, 3)  /* using as an L2 page table?     */
+#define PGT_l3_page_table PG_mask(3, 3)  /* using as an L3 page table?     */
+#define PGT_l4_page_table PG_mask(4, 3)  /* using as an L4 page table?     */
+#define PGT_seg_desc_page PG_mask(5, 3)  /* using this page in a GDT/LDT?  */
+#define PGT_shared_page   PG_mask(6, 3)  /* CoW sharable page              */
+#define PGT_writable_page PG_mask(7, 3)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(7, 3)  /* Bits 61-63.                    */
 
+ /* Page is locked? */
+#define _PGT_locked       PG_shift(4)
+#define PGT_locked        PG_mask(1, 4)
  /* Owning guest has pinned this page to its current type? */
 #define _PGT_pinned       PG_shift(5)
 #define PGT_pinned        PG_mask(1, 5)
@@ -198,12 +201,9 @@ struct page_info
 /* Has this page been *partially* validated for use as its current type? */
 #define _PGT_partial      PG_shift(8)
 #define PGT_partial       PG_mask(1, 8)
- /* Page is locked? */
-#define _PGT_locked       PG_shift(9)
-#define PGT_locked        PG_mask(1, 9)
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(8)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/2] x86: limit page type width

[Andrew Cooper]

On 31/05/17 08:14, Jan Beulich wrote:
> There's no reason to burn 4 bits on page type when we only have 7 types
> (plus "none") at present. This requires changing one use of
> PGT_shared_page, which so far assumed that the type is both a power of
> 2 and the only type with the high bit set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Any particular reason why you reverse the order of shared and writeable?

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 1/2] x86: limit page type width

[Jan Beulich]

>>> On 31.05.17 at 12:01, <andrew.cooper3@citrix.com> wrote:
> On 31/05/17 08:14, Jan Beulich wrote:
>> There's no reason to burn 4 bits on page type when we only have 7 types
>> (plus "none") at present. This requires changing one use of
>> PGT_shared_page, which so far assumed that the type is both a power of
>> 2 and the only type with the high bit set.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Any particular reason why you reverse the order of shared and writeable?

I've become very used to know that type 7 is "writable", and since
there was a gap at 6 it seemed reasonable to put "shared" there
instead of shifting both down by one.

> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan


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

Re: [PATCH 1/2] x86: limit page type width

[Jan Beulich]

>>> On 31.05.17 at 09:14, <JBeulich@suse.com> wrote:
> There's no reason to burn 4 bits on page type when we only have 7 types
> (plus "none") at present. This requires changing one use of
> PGT_shared_page, which so far assumed that the type is both a power of
> 2 and the only type with the high bit set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -452,7 +452,7 @@ static int audit(void)
>          }
>  
>          /* Check if the MFN has correct type, owner and handle. */ 
> -        if ( !(pg->u.inuse.type_info & PGT_shared_page) )
> +        if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
>          {
>             MEM_SHARING_DEBUG("mfn %lx in audit list, but not PGT_shared_page (%lx)!\n",
>                                mfn_x(mfn), pg->u.inuse.type_info & PGT_type_mask);

Tamas,

I've noticed only now that I did forget to Cc you on the change
above (fixing a latent bug, which would otherwise become an
actual one with the other adjustments done here).

Jan


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

[PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

The flag is really only meant for those, both HVM and 32-bit PV tell
kernel from user mode based on CPL/RPL. Remove the all-question-marks
comment and let's be on the safe side here and also suppress clearing
for 32-bit PV (this isn't a fast path after all).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -950,9 +950,15 @@ int arch_set_info_guest(
 
     v->fpu_initialised = !!(flags & VGCF_I387_VALID);
 
-    v->arch.flags &= ~TF_kernel_mode;
-    if ( (flags & VGCF_in_kernel) || is_hvm_domain(d)/*???*/ )
-        v->arch.flags |= TF_kernel_mode;
+    v->arch.flags |= TF_kernel_mode;
+    if ( unlikely(!(flags & VGCF_in_kernel)) &&
+         /*
+          * TF_kernel_mode is only allowed to be clear for 64-bit PV. See
+          * update_cr3(), sh_update_cr3(), and shadow_one_bit_disable() for
+          * why that is.
+          */
+         !is_hvm_domain(d) && !is_pv_32bit_domain(d) )
+        v->arch.flags &= ~TF_kernel_mode;
 
     v->arch.vgc_flags = flags;
 




[-- Attachment #2: x86-TF_kernel_mode-64bit-only.patch --]
[-- Type: text/plain, Size: 1141 bytes --]

x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

The flag is really only meant for those, both HVM and 32-bit PV tell
kernel from user mode based on CPL/RPL. Remove the all-question-marks
comment and let's be on the safe side here and also suppress clearing
for 32-bit PV (this isn't a fast path after all).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -950,9 +950,15 @@ int arch_set_info_guest(
 
     v->fpu_initialised = !!(flags & VGCF_I387_VALID);
 
-    v->arch.flags &= ~TF_kernel_mode;
-    if ( (flags & VGCF_in_kernel) || is_hvm_domain(d)/*???*/ )
-        v->arch.flags |= TF_kernel_mode;
+    v->arch.flags |= TF_kernel_mode;
+    if ( unlikely(!(flags & VGCF_in_kernel)) &&
+         /*
+          * TF_kernel_mode is only allowed to be clear for 64-bit PV. See
+          * update_cr3(), sh_update_cr3(), and shadow_one_bit_disable() for
+          * why that is.
+          */
+         !is_hvm_domain(d) && !is_pv_32bit_domain(d) )
+        v->arch.flags &= ~TF_kernel_mode;
 
     v->arch.vgc_flags = flags;
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Andrew Cooper]

On 31/05/17 08:15, Jan Beulich wrote:
> The flag is really only meant for those, both HVM and 32-bit PV tell
> kernel from user mode based on CPL/RPL. Remove the all-question-marks
> comment and let's be on the safe side here and also suppress clearing
> for 32-bit PV (this isn't a fast path after all).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Wouldn't it just be safer to disallow starting a 64bit PV guest in user
mode?

No real kernel would do such a thing, and keeping the corner case around
is bad from an attack-surface point of view.

~Andrew

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

Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Jan Beulich]

>>> On 31.05.17 at 13:08, <andrew.cooper3@citrix.com> wrote:
> On 31/05/17 08:15, Jan Beulich wrote:
>> The flag is really only meant for those, both HVM and 32-bit PV tell
>> kernel from user mode based on CPL/RPL. Remove the all-question-marks
>> comment and let's be on the safe side here and also suppress clearing
>> for 32-bit PV (this isn't a fast path after all).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Wouldn't it just be safer to disallow starting a 64bit PV guest in user
> mode?
> 
> No real kernel would do such a thing, and keeping the corner case around
> is bad from an attack-surface point of view.

If it really was "starting a guest", I would probably agree. But we're
talking about starting a vCPU, and I could see uses for this (not the
least in XTF). After all the operation allows for enough state to be
set up such that further initialization inside the guest may not be
necessary.

Jan


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

Ping: Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Jan Beulich]

>>> On 31.05.17 at 13:54,  wrote:
>>>> On 31.05.17 at 13:08, <andrew.cooper3@citrix.com> wrote:
> > On 31/05/17 08:15, Jan Beulich wrote:
> >> The flag is really only meant for those, both HVM and 32-bit PV tell
> >> kernel from user mode based on CPL/RPL. Remove the all-question-marks
> >> comment and let's be on the safe side here and also suppress clearing
> >> for 32-bit PV (this isn't a fast path after all).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Wouldn't it just be safer to disallow starting a 64bit PV guest in user
> > mode?
> > 
> > No real kernel would do such a thing, and keeping the corner case around
> > is bad from an attack-surface point of view.
> 
> If it really was "starting a guest", I would probably agree. But we're
> talking about starting a vCPU, and I could see uses for this (not the
> least in XTF). After all the operation allows for enough state to be
> set up such that further initialization inside the guest may not be
> necessary.

Any opinion here, or change of opinion on the original patch?

Jan


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

Ping#2: Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Jan Beulich]

>>> On 03.07.17 at 16:56,  wrote:
>>>> On 31.05.17 at 13:54,  wrote:
> >>>> On 31.05.17 at 13:08, <andrew.cooper3@citrix.com> wrote:
> > > On 31/05/17 08:15, Jan Beulich wrote:
> > >> The flag is really only meant for those, both HVM and 32-bit PV tell
> > >> kernel from user mode based on CPL/RPL. Remove the all-question-marks
> > >> comment and let's be on the safe side here and also suppress clearing
> > >> for 32-bit PV (this isn't a fast path after all).
> > >>
> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > Wouldn't it just be safer to disallow starting a 64bit PV guest in user
> > > mode?
> > > 
> > > No real kernel would do such a thing, and keeping the corner case around
> > > is bad from an attack-surface point of view.
> > 
> > If it really was "starting a guest", I would probably agree. But we're
> > talking about starting a vCPU, and I could see uses for this (not the
> > least in XTF). After all the operation allows for enough state to be
> > set up such that further initialization inside the guest may not be
> > necessary.
> 
> Any opinion here, or change of opinion on the original patch?

I'd really like to get this off my list.

Jan


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

Re: Ping#2: Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Andrew Cooper]

On 04/12/17 10:15, Jan Beulich wrote:
>>>> On 03.07.17 at 16:56,  wrote:
>>>>> On 31.05.17 at 13:54,  wrote:
>>>>>> On 31.05.17 at 13:08, <andrew.cooper3@citrix.com> wrote:
>>>> On 31/05/17 08:15, Jan Beulich wrote:
>>>>> The flag is really only meant for those, both HVM and 32-bit PV tell
>>>>> kernel from user mode based on CPL/RPL. Remove the all-question-marks
>>>>> comment and let's be on the safe side here and also suppress clearing
>>>>> for 32-bit PV (this isn't a fast path after all).
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Wouldn't it just be safer to disallow starting a 64bit PV guest in user
>>>> mode?
>>>>
>>>> No real kernel would do such a thing, and keeping the corner case around
>>>> is bad from an attack-surface point of view.
>>> If it really was "starting a guest", I would probably agree. But we're
>>> talking about starting a vCPU, and I could see uses for this (not the
>>> least in XTF). After all the operation allows for enough state to be
>>> set up such that further initialization inside the guest may not be
>>> necessary.
>> Any opinion here, or change of opinion on the original patch?
> I'd really like to get this off my list.

My opinion is unchanged.  This isn't a useful piece of functionality,
and it definitely doesn't warrant the attack surface it brings.

~Andrew

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

Re: Ping#2: Re: [PATCH 2/2] x86: don't allow clearing of TF_kernel_mode for other than 64-bit PV

[Jan Beulich]

>>> On 04.12.17 at 16:11, <andrew.cooper3@citrix.com> wrote:
> On 04/12/17 10:15, Jan Beulich wrote:
>>>>> On 03.07.17 at 16:56,  wrote:
>>>>>> On 31.05.17 at 13:54,  wrote:
>>>>>>> On 31.05.17 at 13:08, <andrew.cooper3@citrix.com> wrote:
>>>>> On 31/05/17 08:15, Jan Beulich wrote:
>>>>>> The flag is really only meant for those, both HVM and 32-bit PV tell
>>>>>> kernel from user mode based on CPL/RPL. Remove the all-question-marks
>>>>>> comment and let's be on the safe side here and also suppress clearing
>>>>>> for 32-bit PV (this isn't a fast path after all).
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Wouldn't it just be safer to disallow starting a 64bit PV guest in user
>>>>> mode?
>>>>>
>>>>> No real kernel would do such a thing, and keeping the corner case around
>>>>> is bad from an attack-surface point of view.
>>>> If it really was "starting a guest", I would probably agree. But we're
>>>> talking about starting a vCPU, and I could see uses for this (not the
>>>> least in XTF). After all the operation allows for enough state to be
>>>> set up such that further initialization inside the guest may not be
>>>> necessary.
>>> Any opinion here, or change of opinion on the original patch?
>> I'd really like to get this off my list.
> 
> My opinion is unchanged.  This isn't a useful piece of functionality,
> and it definitely doesn't warrant the attack surface it brings.

Very strange - you therefore prefer the current, even more
permissive code over the one the patch switches to just because
you think it should be even more tight (which I gave reasons why
I disagree with, and which then you would also be free to submit
a patch to further adjust, with suitable justification)?

Jan


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