[PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[Andrew Cooper]

c/s 4c5d78a10 was accidentally buggy when handling Protection Keys.
Protection keys applies to all user translations, not just accesses which
originate from user mode.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Julien Grall <julien.grall@arm.com>

This regression was introducing during the 4.9 timeframe, so really should be
fixed before 4.9 ships.
---
 xen/arch/x86/mm/guest_walk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index 32d818e..ba72432 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -408,7 +408,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
      * N.B. In the case that the walk ended with a superpage, the fabricated
      * gw->l1e contains the appropriate leaf pkey.
      */
-    if ( (walk & PFEC_user_mode) && !(walk & PFEC_insn_fetch) &&
+    if ( (ar & _PAGE_USER) && !(walk & PFEC_insn_fetch) &&
          guest_pku_enabled(v) )
     {
         unsigned int pkey = guest_l1e_get_pkey(gw->l1e);
-- 
2.1.4


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

Re: [PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[Tim Deegan]

At 17:51 +0100 on 16 May (1494957116), Andrew Cooper wrote:
> c/s 4c5d78a10 was accidentally buggy when handling Protection Keys.
> Protection keys applies to all user translations, not just accesses which
> originate from user mode.

Reviewed-by: Tim Deegan <tim@xen.org>

Does the test for write-protection just below have the opposite bug?
It seems to check whether the page is writable, when AFAICS it should
be checking whether the action is a write (modulo CR0.WP).

Tim.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> This regression was introducing during the 4.9 timeframe, so really should be
> fixed before 4.9 ships.
> ---
>  xen/arch/x86/mm/guest_walk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index 32d818e..ba72432 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -408,7 +408,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>       * N.B. In the case that the walk ended with a superpage, the fabricated
>       * gw->l1e contains the appropriate leaf pkey.
>       */
> -    if ( (walk & PFEC_user_mode) && !(walk & PFEC_insn_fetch) &&
> +    if ( (ar & _PAGE_USER) && !(walk & PFEC_insn_fetch) &&
>           guest_pku_enabled(v) )
>      {
>          unsigned int pkey = guest_l1e_get_pkey(gw->l1e);
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[Andrew Cooper]

On 16/05/17 18:06, Tim Deegan wrote:
> At 17:51 +0100 on 16 May (1494957116), Andrew Cooper wrote:
>> c/s 4c5d78a10 was accidentally buggy when handling Protection Keys.
>> Protection keys applies to all user translations, not just accesses which
>> originate from user mode.
> Reviewed-by: Tim Deegan <tim@xen.org>
>
> Does the test for write-protection just below have the opposite bug?
> It seems to check whether the page is writable, when AFAICS it should
> be checking whether the action is a write (modulo CR0.WP).

Very good point.  Let me draft a v2.

~Andrew

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

Re: [PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[Jan Beulich]

>>> On 16.05.17 at 18:51, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -408,7 +408,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>       * N.B. In the case that the walk ended with a superpage, the fabricated
>       * gw->l1e contains the appropriate leaf pkey.
>       */
> -    if ( (walk & PFEC_user_mode) && !(walk & PFEC_insn_fetch) &&
> +    if ( (ar & _PAGE_USER) && !(walk & PFEC_insn_fetch) &&
>           guest_pku_enabled(v) )
>      {
>          unsigned int pkey = guest_l1e_get_pkey(gw->l1e);

Perhaps the comment ahead of this could then also do with some
improvement ("user data accesses" -> "accesses to user data")?

Jan


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

Re: [PATCH for-4.9] x86/pagewalk: Fix determination of Protection Key access rights

[Julien Grall]

Hi Andrew,

On 16/05/17 18:12, Andrew Cooper wrote:
> On 16/05/17 18:06, Tim Deegan wrote:
>> At 17:51 +0100 on 16 May (1494957116), Andrew Cooper wrote:
>>> c/s 4c5d78a10 was accidentally buggy when handling Protection Keys.
>>> Protection keys applies to all user translations, not just accesses which
>>> originate from user mode.
>> Reviewed-by: Tim Deegan <tim@xen.org>
>>
>> Does the test for write-protection just below have the opposite bug?
>> It seems to check whether the page is writable, when AFAICS it should
>> be checking whether the action is a write (modulo CR0.WP).
>
> Very good point.  Let me draft a v2.

FIY, I will wait the v2 before considering it for the release.

Cheers,

-- 
Julien Grall

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