Re: [Xen-devel] [PATCH v2] x86/mm: also flush TLB when putting writable foreign page reference

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 02:59 -0600 on 25 Apr (1493089158), Jan Beulich wrote:
> Jann's explanation of the problem:
> 
> "start situation:
>  - domain A and domain B are PV domains
>  - domain A and B both have currently scheduled vCPUs, and the vCPUs
>    are not scheduled away
>  - domain A has XSM_TARGET access to domain B
>  - page X is owned by domain B and has no mappings
>  - page X is zeroed
> 
>  steps:
>  - domain A uses do_mmu_update() to map page X in domain A as writable
>  - domain A accesses page X through the new PTE, creating a TLB entry
>  - domain A removes its mapping of page X
>    - type count of page X goes to 0
>    - tlbflush_timestamp of page X is bumped
>  - domain B maps page X as L1 pagetable
>    - type of page X changes to PGT_l1_page_table
>    - TLB flush is forced using domain_dirty_cpumask of domain B
>    - page X is mapped as L1 pagetable in domain B
> 
>  At this point, domain B's vCPUs are guaranteed to have no
>  incorrectly-typed stale TLB entries for page X, but AFAICS domain A's
>  vCPUs can still have stale TLB entries that map page X as writable,
>  permitting domain A to control a live pagetable of domain B."

AIUI this patch solves the problem by immediately flushing domain A's
TLB entries at the point where domain A removes its mapping of page X.

Could we, instead, bitwise OR domain A's domain_dirty_cpumask into
domain B's domain_dirty_cpumask at the same point?

Then when domain B flushes TLBs in the last step (in __get_page_type())
it will catch any stale TLB entries from domain A as well.  But in the
(hopefully common) case where there's a delay between domain A's
__put_page_type() and domain B's __get_page_type(), the usual TLB
timestamp filtering will suppress some of the IPIs/flushes.

Cheers,

Tim.


> Domain A necessarily is Dom0 (DomU-s with XSM_TARGET permission are
> being created only for HVM domains, but domain B needs to be PV here),
> so this is not a security issue, but nevertheless seems desirable to
> correct.
> 
> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Don't consider page's time stamp (relevant only for the owning
>     domain).
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1266,6 +1266,18 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>      if ( (l1e_get_flags(l1e) & _PAGE_RW) && 
>           ((l1e_owner == pg_owner) || !paging_mode_external(pg_owner)) )
>      {
> +        /*
> +         * Don't leave stale writable TLB entries in the unmapping domain's
> +         * page tables, to prevent them allowing access to pages required to
> +         * be read-only (e.g. after pg_owner changed them to page table or
> +         * segment descriptor pages).
> +         */
> +        if ( unlikely(l1e_owner != pg_owner) )
> +        {
> +            perfc_incr(need_flush_tlb_flush);
> +            flush_tlb_mask(l1e_owner->domain_dirty_cpumask);
> +        }
> +
>          put_page_and_type(page);
>      }
>      else

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