[PATCH] x86/pagewalk: Read guest PTEs with ACCESS_ONCE()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

This has been a plain C read for as far back as I can trace in history.

Research into invented-loads has flagged it as a possible vulnerability.
After careful analysis, it is believed to be a bug only, not a security
vulnerability.

The code fits the pattern for invented loads, and it is a risk.

The analysis suggests that we can read one value out of the guest, operate on
another, and that this could be an in-guest privliege escalation.  Any entity
in the guest able to modify the pagetables already has full privilege, so
while Xen can potentially malfunction, the effects don't cross a privilege
boundary.

The analysis also suggests that this is worse for shadow guests because we may
put the TOCTOU entry in the shadows, but this is inaccurate.  What we put in
the shadows is still translated under the P2M and refers to guest physical
address space.

Either way, harden the accesses.

Link: 
https://github.com/xoreaxeaxeax/schrodingers-toctou/blob/main/observer-effect/audits/audit-xen-ptwalk-RELEASE-4.21.1.md#86-per-candidate-finding
Fixes: 49f7c7364e0a ("Replace shadow pagetable code with shadow2.")
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
CC: Teddy Astie <teddy.astie@xxxxxxxxxx>

I'm not really sure about the fixes tag.  That's the oldest commit which bares
any reseblence to the current code, and it was a bulk rewrite of the whole
shadow pagetable code.  Prior to that, it was all mixed up and it's not
completely obvious what's (definiely) walking the guest pagetables as opposed
to the shadows.

Bloat-o-meter shows this clearly makes a code-gen difference in all cases:

  add/remove: 0/0 grow/shrink: 1/2 up/down: 16/-19 (-3)
  Function                                     old     new   delta
  guest_walk_tables_2_levels                  1688    1704     +16
  guest_walk_tables_4_levels                  3708    3703      -5
  guest_walk_tables_3_levels                  2233    2219     -14

To start with, l?e_read() looked to be the right helper, but they don't exist
for guest pagetable types, leading to:

arch/x86/mm/guest_walk.c: In function ‘guest_walk_tables_2_levels’:
./arch/x86/include/asm/page.h:135:36: error: incompatible types when assigning 
to type ‘guest_l2e_t’ from type ‘l2_pgentry_t’
  135 | #define l2e_from_intpte(intpte)    ((l2_pgentry_t) { (intpte_t)(intpte) 
})
      |                                    ^
---
 xen/arch/x86/mm/guest_walk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index f48c3ef75f48..df2ccaa67475 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -129,7 +129,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain 
*p2m,
             guest_l4_table_offset(va) * sizeof(gw->l4e);
     if ( !hvmemul_read_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e)) )
     {
-        gw->l4e = l4p[guest_l4_table_offset(va)];
+        gw->l4e = (guest_l4e_t){ 
ACCESS_ONCE(l4p[guest_l4_table_offset(va)].l4) };
         hvmemul_write_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e));
     }
     gflags = guest_l4e_get_flags(gw->l4e);
@@ -164,7 +164,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain 
*p2m,
             guest_l3_table_offset(va) * sizeof(gw->l3e);
     if ( !hvmemul_read_cache(v, l3gpa, &gw->l3e, sizeof(gw->l3e)) )
     {
-        gw->l3e = l3p[guest_l3_table_offset(va)];
+        gw->l3e = (guest_l3e_t){ 
ACCESS_ONCE(l3p[guest_l3_table_offset(va)].l3) };
         hvmemul_write_cache(v, l3gpa, &gw->l3e, sizeof(gw->l3e));
     }
     gflags = guest_l3e_get_flags(gw->l3e);
@@ -264,7 +264,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain 
*p2m,
     l2gpa += guest_l2_table_offset(va) * sizeof(gw->l2e);
     if ( !hvmemul_read_cache(v, l2gpa, &gw->l2e, sizeof(gw->l2e)) )
     {
-        gw->l2e = l2p[guest_l2_table_offset(va)];
+        gw->l2e = (guest_l2e_t){ 
ACCESS_ONCE(l2p[guest_l2_table_offset(va)].l2) };
         hvmemul_write_cache(v, l2gpa, &gw->l2e, sizeof(gw->l2e));
     }
 
@@ -353,7 +353,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain 
*p2m,
             guest_l1_table_offset(va) * sizeof(gw->l1e);
     if ( !hvmemul_read_cache(v, l1gpa, &gw->l1e, sizeof(gw->l1e)) )
     {
-        gw->l1e = l1p[guest_l1_table_offset(va)];
+        gw->l1e = (guest_l1e_t){ 
ACCESS_ONCE(l1p[guest_l1_table_offset(va)].l1) };
         hvmemul_write_cache(v, l1gpa, &gw->l1e, sizeof(gw->l1e));
     }
 

base-commit: e888192d133eeec8a94275eaf4194117f198a7e2
-- 
2.39.5