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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/2020 08:27, Jan Beulich wrote:
> 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.

Hmm yes - I think I prefer this, because we don't really want to keep
the non-kern alternative.

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> however ...

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

I'd suggest not making that change here in this bugfix.  I think we do
want to alter how we do asserts like this, and there are other similarly
impacted code blocks.

>
> --- 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) )

... pull this out into a variable, like the original code used to do.

bool user_mode = !(curr->arch.flags & TF_kernel_mode);

I've forgotten which static checker tripped up over this form, but one
did IIRC.

~Andrew

> +        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;
>  }
>