Re: [PATCH] Revert "x86/mm: drop guest_get_eff_l1e()"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2020 15:16, Andrew Cooper wrote:
> This reverts commit 9ff9705647646aa937b5f5c1426a64c69a62b3bd.
> 
> The change is only correct in the original context of XSA-286, where Xen's use
> of the linear pagetables were dropped.  However, performance problems
> interfered with that plan, and XSA-286 was fixed differently.
> 
> This broke Xen's lazy faulting of the LDT for 64bit PV guests when an access
> was first encountered in user context.  Xen would proceed to read the
> registered LDT virtual address out of the user pagetables, not the kernel
> pagetables.
> 
> Given the nature of the bug, it would have also interfered with the IO
> permisison bitmap functionality of userspace, which similarly needs to read
> data using the kernel pagetables.

This paragraph wants dropping afaict - guest_io_okay() has
explicit calls to toggle_guest_pt(), and hence is unaffected by
the bug here (and there is in particular page tables reading
involved there). Then ...

> Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Tested-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I was wondering however whether we really want a full revert. I've
locally made the change below as an alternative.

Jan

x86/PV: guest_get_eff_kern_l1e() may still need to switch page tables

While indeed unnecessary for pv_ro_page_fault(), pv_map_ldt_shadow_page()
may run when guest user mode is active, and hence may need to switch to
the kernel page tables in order to retrieve an LDT page mapping.

Fixes: 9ff970564764 ("x86/mm: drop guest_get_eff_l1e()")
Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
This is the alternative to fully reverting the offending commit.

I've also been considering to drop the paging-mode-translate ASSERT(),
now that we always have translate == external.

--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -11,10 +11,14 @@ int new_guest_cr3(mfn_t mfn);
  */
 static inline l1_pgentry_t guest_get_eff_kern_l1e(unsigned long linear)
 {
+    struct vcpu *curr = current;
     l1_pgentry_t l1e;
 
-    ASSERT(!paging_mode_translate(current->domain));
-    ASSERT(!paging_mode_external(current->domain));
+    ASSERT(!paging_mode_translate(curr->domain));
+    ASSERT(!paging_mode_external(curr->domain));
+
+    if ( !(curr->arch.flags & TF_kernel_mode) )
+        toggle_guest_pt(curr);
 
     if ( unlikely(!__addr_ok(linear)) ||
          __copy_from_user(&l1e,
@@ -22,6 +26,9 @@ static inline l1_pgentry_t guest_get_eff
                           sizeof(l1_pgentry_t)) )
         l1e = l1e_empty();
 
+    if ( !(curr->arch.flags & TF_kernel_mode) )
+        toggle_guest_pt(curr);
+
     return l1e;
 }