[kvm-unit-tests PATCH] x86: access: revert PTE changes if CR4.SMEP change failed

[kvm-unit-tests PATCH] x86: access: revert PTE changes if CR4.SMEP change failed

[Evgeny Yakovlev]

When calling set_cr4_smep(1) to enable SMEP implementation will first
drop user access bit in ptl2 and then attempt to change actual cr4
value. In case emulated CPU does not support setting CR4.SMEP this will
generate a GP which we expect. However, in that case we should also
revert user access bit change. Othervise supervisor access sticks and
later faults the test binary.

Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
---
 x86/access.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/x86/access.c b/x86/access.c
index a0c19dc..ccdaefc 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -195,6 +195,7 @@ unsigned set_cr4_smep(int smep)
 {
     unsigned long cr4 = read_cr4();
     unsigned long old_cr4 = cr4;
+    unsigned long ptl2_access;
     extern u64 ptl2[];
     unsigned r;
 
@@ -204,9 +205,15 @@ unsigned set_cr4_smep(int smep)
     if (old_cr4 == cr4)
         return 0;
 
+    ptl2_access = ptl2[2];
     if (smep)
         ptl2[2] &= ~PT_USER_MASK;
     r = write_cr4_checking(cr4);
+    if (cr4 != read_cr4()) {
+        if (smep)
+            ptl2[2] = ptl2_access;
+        return r;
+    }
     if (!smep)
         ptl2[2] |= PT_USER_MASK;
     return r;
-- 
2.7.4

Re: [kvm-unit-tests PATCH] x86: access: revert PTE changes if CR4.SMEP change failed

[Paolo Bonzini]

On 19/09/2017 15:33, Evgeny Yakovlev wrote:
> When calling set_cr4_smep(1) to enable SMEP implementation will first
> drop user access bit in ptl2 and then attempt to change actual cr4
> value. In case emulated CPU does not support setting CR4.SMEP this will
> generate a GP which we expect. However, in that case we should also
> revert user access bit change. Othervise supervisor access sticks and
> later faults the test binary.
> 
> Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
> ---
>  x86/access.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/x86/access.c b/x86/access.c
> index a0c19dc..ccdaefc 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -195,6 +195,7 @@ unsigned set_cr4_smep(int smep)
>  {
>      unsigned long cr4 = read_cr4();
>      unsigned long old_cr4 = cr4;
> +    unsigned long ptl2_access;
>      extern u64 ptl2[];
>      unsigned r;
>  
> @@ -204,9 +205,15 @@ unsigned set_cr4_smep(int smep)
>      if (old_cr4 == cr4)
>          return 0;
>  
> +    ptl2_access = ptl2[2];
>      if (smep)
>          ptl2[2] &= ~PT_USER_MASK;
>      r = write_cr4_checking(cr4);
> +    if (cr4 != read_cr4()) {
> +        if (smep)
> +            ptl2[2] = ptl2_access;
> +        return r;
> +    }
>      if (!smep)
>          ptl2[2] |= PT_USER_MASK;
>      return r;
> 

This is more or less the same patch as "x86: fix access.flat on non-SMEP
machines", I think?

Paolo

Re: [kvm-unit-tests PATCH] x86: access: revert PTE changes if CR4.SMEP change failed

[Евгений Яковлев]

On 19.09.2017 17:34, Paolo Bonzini wrote:
> On 19/09/2017 15:33, Evgeny Yakovlev wrote:
>> When calling set_cr4_smep(1) to enable SMEP implementation will first
>> drop user access bit in ptl2 and then attempt to change actual cr4
>> value. In case emulated CPU does not support setting CR4.SMEP this will
>> generate a GP which we expect. However, in that case we should also
>> revert user access bit change. Othervise supervisor access sticks and
>> later faults the test binary.
>>
>> Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
>> ---
>>   x86/access.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/x86/access.c b/x86/access.c
>> index a0c19dc..ccdaefc 100644
>> --- a/x86/access.c
>> +++ b/x86/access.c
>> @@ -195,6 +195,7 @@ unsigned set_cr4_smep(int smep)
>>   {
>>       unsigned long cr4 = read_cr4();
>>       unsigned long old_cr4 = cr4;
>> +    unsigned long ptl2_access;
>>       extern u64 ptl2[];
>>       unsigned r;
>>   
>> @@ -204,9 +205,15 @@ unsigned set_cr4_smep(int smep)
>>       if (old_cr4 == cr4)
>>           return 0;
>>   
>> +    ptl2_access = ptl2[2];
>>       if (smep)
>>           ptl2[2] &= ~PT_USER_MASK;
>>       r = write_cr4_checking(cr4);
>> +    if (cr4 != read_cr4()) {
>> +        if (smep)
>> +            ptl2[2] = ptl2_access;
>> +        return r;
>> +    }
>>       if (!smep)
>>           ptl2[2] |= PT_USER_MASK;
>>       return r;
>>
> This is more or less the same patch as "x86: fix access.flat on non-SMEP
> machines", I think?

Um, yes, i found it the mailing list just now. Yep, it solves the same 
problem.