[PATCH] x86: refine debugging of SMEP/SMAP fix

[PATCH] x86: refine debugging of SMEP/SMAP fix

[Jan Beulich]

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

Instead of just latching cr4_pv32_mask into %rdx, correct the found
wrong value in %cr4 (to avoid triggering another BUG). The value left
in %rdx should be sufficient for deducing cr4_pv32_mask from the
register dump.

Also there is one more place for XEN_CR4_PV32_BITS to be used.

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

--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -182,7 +182,7 @@ ENTRY(compat_restore_all_guest)
         testb $3,UREGS_cs(%rsp)
         jpe   .Lcr4_alt_end
         mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
-        and   $~(X86_CR4_SMEP|X86_CR4_SMAP), %rax
+        and   $~XEN_CR4_PV32_BITS, %rax
         mov   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
         mov   %rax, %cr4
 .Lcr4_alt_end:
@@ -218,8 +218,10 @@ ENTRY(cr4_pv32_restore)
         and   cr4_pv32_mask(%rip), %rax
         cmp   cr4_pv32_mask(%rip), %rax
         je    1f
-        /* Cause cr4_pv32_mask to be visible in the BUG register dump. */
-        mov   cr4_pv32_mask(%rip), %rdx
+        /* Avoid coming back here while handling the #UD we cause below. */
+        mov   %cr4, %rdx
+        or    cr4_pv32_mask(%rip), %rdx
+        mov   %rdx, %cr4
         BUG
 1:
 #endif




[-- Attachment #2: x86-SMEP-SMAP-addendum.patch --]
[-- Type: text/plain, Size: 1326 bytes --]

x86: refine debugging of SMEP/SMAP fix

Instead of just latching cr4_pv32_mask into %rdx, correct the found
wrong value in %cr4 (to avoid triggering another BUG). The value left
in %rdx should be sufficient for deducing cr4_pv32_mask from the
register dump.

Also there is one more place for XEN_CR4_PV32_BITS to be used.

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

--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -182,7 +182,7 @@ ENTRY(compat_restore_all_guest)
         testb $3,UREGS_cs(%rsp)
         jpe   .Lcr4_alt_end
         mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
-        and   $~(X86_CR4_SMEP|X86_CR4_SMAP), %rax
+        and   $~XEN_CR4_PV32_BITS, %rax
         mov   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
         mov   %rax, %cr4
 .Lcr4_alt_end:
@@ -218,8 +218,10 @@ ENTRY(cr4_pv32_restore)
         and   cr4_pv32_mask(%rip), %rax
         cmp   cr4_pv32_mask(%rip), %rax
         je    1f
-        /* Cause cr4_pv32_mask to be visible in the BUG register dump. */
-        mov   cr4_pv32_mask(%rip), %rdx
+        /* Avoid coming back here while handling the #UD we cause below. */
+        mov   %cr4, %rdx
+        or    cr4_pv32_mask(%rip), %rdx
+        mov   %rdx, %cr4
         BUG
 1:
 #endif

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

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

Re: [PATCH] x86: refine debugging of SMEP/SMAP fix

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1735 bytes --]

On 17/05/16 10:54, Jan Beulich wrote:
> Instead of just latching cr4_pv32_mask into %rdx, correct the found
> wrong value in %cr4 (to avoid triggering another BUG). The value left
> in %rdx should be sufficient for deducing cr4_pv32_mask from the
> register dump.

Alternatively, you can reuse %rax (as its value is useless by this
point) and leave %rdx as exactly cr4_pv32_mask.  This avoids needing a
subsequent step to reverse engineer cr4_pv32_mask.

>
> Also there is one more place for XEN_CR4_PV32_BITS to be used.

So there is.  Sorry for missing it.

~Andrew

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -182,7 +182,7 @@ ENTRY(compat_restore_all_guest)
>          testb $3,UREGS_cs(%rsp)
>          jpe   .Lcr4_alt_end
>          mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
> -        and   $~(X86_CR4_SMEP|X86_CR4_SMAP), %rax
> +        and   $~XEN_CR4_PV32_BITS, %rax
>          mov   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
>          mov   %rax, %cr4
>  .Lcr4_alt_end:
> @@ -218,8 +218,10 @@ ENTRY(cr4_pv32_restore)
>          and   cr4_pv32_mask(%rip), %rax
>          cmp   cr4_pv32_mask(%rip), %rax
>          je    1f
> -        /* Cause cr4_pv32_mask to be visible in the BUG register dump. */
> -        mov   cr4_pv32_mask(%rip), %rdx
> +        /* Avoid coming back here while handling the #UD we cause below. */
> +        mov   %cr4, %rdx
> +        or    cr4_pv32_mask(%rip), %rdx
> +        mov   %rdx, %cr4
>          BUG
>  1:
>  #endif
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2713 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86: refine debugging of SMEP/SMAP fix

[Jan Beulich]

>>> On 17.05.16 at 12:28, <andrew.cooper3@citrix.com> wrote:
> On 17/05/16 10:54, Jan Beulich wrote:
>> Instead of just latching cr4_pv32_mask into %rdx, correct the found
>> wrong value in %cr4 (to avoid triggering another BUG). The value left
>> in %rdx should be sufficient for deducing cr4_pv32_mask from the
>> register dump.
> 
> Alternatively, you can reuse %rax (as its value is useless by this
> point) and leave %rdx as exactly cr4_pv32_mask.  This avoids needing a
> subsequent step to reverse engineer cr4_pv32_mask.

I don't view the value in %rax as useless - that's the set of bits
we have found set, which didn't match our expectation. Hence I
specifically don't want to re-use that register.

Jan


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

Re: [PATCH] x86: refine debugging of SMEP/SMAP fix

[Andrew Cooper]

On 17/05/16 11:42, Jan Beulich wrote:
>>>> On 17.05.16 at 12:28, <andrew.cooper3@citrix.com> wrote:
>> On 17/05/16 10:54, Jan Beulich wrote:
>>> Instead of just latching cr4_pv32_mask into %rdx, correct the found
>>> wrong value in %cr4 (to avoid triggering another BUG). The value left
>>> in %rdx should be sufficient for deducing cr4_pv32_mask from the
>>> register dump.
>> Alternatively, you can reuse %rax (as its value is useless by this
>> point) and leave %rdx as exactly cr4_pv32_mask.  This avoids needing a
>> subsequent step to reverse engineer cr4_pv32_mask.
> I don't view the value in %rax as useless - that's the set of bits
> we have found set, which didn't match our expectation. Hence I
> specifically don't want to re-use that register.

Ok.  We don't need to preserve any registers from the caller, so using a
different one such as %rbx or %rcx would be fine.

The issue is that it is important to see precisely what cr4_pv32_mask is.

~Andrew

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

Re: [PATCH] x86: refine debugging of SMEP/SMAP fix

[Jan Beulich]

>>> On 17.05.16 at 14:56, <andrew.cooper3@citrix.com> wrote:
> On 17/05/16 11:42, Jan Beulich wrote:
>>>>> On 17.05.16 at 12:28, <andrew.cooper3@citrix.com> wrote:
>>> On 17/05/16 10:54, Jan Beulich wrote:
>>>> Instead of just latching cr4_pv32_mask into %rdx, correct the found
>>>> wrong value in %cr4 (to avoid triggering another BUG). The value left
>>>> in %rdx should be sufficient for deducing cr4_pv32_mask from the
>>>> register dump.
>>> Alternatively, you can reuse %rax (as its value is useless by this
>>> point) and leave %rdx as exactly cr4_pv32_mask.  This avoids needing a
>>> subsequent step to reverse engineer cr4_pv32_mask.
>> I don't view the value in %rax as useless - that's the set of bits
>> we have found set, which didn't match our expectation. Hence I
>> specifically don't want to re-use that register.
> 
> Ok.  We don't need to preserve any registers from the caller, so using a
> different one such as %rbx or %rcx would be fine.

Well, I can certainly (yet hesitantly) do that (and I see no harm in
clobbering another register), but ...

> The issue is that it is important to see precisely what cr4_pv32_mask is.

... it's not really clear to me what case you see where the value
just written to CR4 won't give you all the information you need.
After all cr4_pv32_mask doesn't have an awful lot of possible
values.

Jan


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