From da4f24dcdf0de4fd97835edaac6f449242699cbb Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Thu, 12 Oct 2017 16:00:47 +0200 Subject: [PATCH] x86: don't store possibly stale TLB flush time stamp While the timing window is extremely narrow, it is theoretically possible for an update to the TLB flush clock and a subsequent flush IPI to happen between the read and write parts of the update of the per-page stamp. Exclude this possibility by disabling interrupts across the update, preventing the IPI to be serviced in the middle. This is XSA-241. Reported-by: Jann Horn Suggested-by: George Dunlap Signed-off-by: Jan Beulich Reviewed-by: George Dunlap master commit: 23a183607a427572185fc51c76cc5ab11c00c4cc master date: 2017-10-12 14:48:25 +0200 --- xen/arch/arm/smp.c | 1 + xen/arch/x86/mm.c | 8 ++++---- xen/arch/x86/mm/shadow/common.c | 2 +- xen/common/page_alloc.c | 2 +- xen/include/asm-arm/flushtlb.h | 5 +++++ xen/include/asm-x86/flushtlb.h | 14 ++++++++++++++ 6 files changed, 26 insertions(+), 6 deletions(-) diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c index 917d490ce7..f49223feeb 100644 --- a/xen/arch/arm/smp.c +++ b/xen/arch/arm/smp.c @@ -1,4 +1,5 @@ #include +#include #include #include #include diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 46376e0f84..16fc39c1cf 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2334,7 +2334,7 @@ static int _put_final_page_type(struct page_info *page, unsigned long type, */ if ( !(shadow_mode_enabled(page_get_owner(page)) && (page->count_info & PGC_page_table)) ) - page->tlbflush_timestamp = tlbflush_current_time(); + page_set_tlbflush_timestamp(page); wmb(); page->u.inuse.type_info--; } @@ -2344,7 +2344,7 @@ static int _put_final_page_type(struct page_info *page, unsigned long type, (PGT_count_mask|PGT_validated|PGT_partial)) == 1); if ( !(shadow_mode_enabled(page_get_owner(page)) && (page->count_info & PGC_page_table)) ) - page->tlbflush_timestamp = tlbflush_current_time(); + page_set_tlbflush_timestamp(page); wmb(); page->u.inuse.type_info |= PGT_validated; } @@ -2398,7 +2398,7 @@ static int _put_page_type(struct page_info *page, bool_t preemptible, if ( ptpg && PGT_type_equal(x, ptpg->u.inuse.type_info) ) { /* - * page_set_tlbflush_timestamp() accesses the same union + * set_tlbflush_timestamp() accesses the same union * linear_pt_count lives in. Unvalidated page table pages, * however, should occur during domain destruction only * anyway. Updating of linear_pt_count luckily is not @@ -2419,7 +2419,7 @@ static int _put_page_type(struct page_info *page, bool_t preemptible, */ if ( !(shadow_mode_enabled(page_get_owner(page)) && (page->count_info & PGC_page_table)) ) - page->tlbflush_timestamp = tlbflush_current_time(); + page_set_tlbflush_timestamp(page); } if ( likely((y = cmpxchg(&page->u.inuse.type_info, x, nx)) == x) ) diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index 455106683e..4fb3de8681 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -1583,7 +1583,7 @@ void shadow_free(struct domain *d, mfn_t smfn) * TLBs when we reuse the page. Because the destructors leave the * contents of the pages in place, we can delay TLB flushes until * just before the allocator hands the page out again. */ - sp->tlbflush_timestamp = tlbflush_current_time(); + page_set_tlbflush_timestamp(sp); perfc_decr(shadow_alloc_count); page_list_add_tail(sp, &d->arch.paging.shadow.freelist); sp = next; diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index b2e8696777..4cdafcba52 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -881,7 +881,7 @@ static void free_heap_pages( /* If a page has no owner it will need no safety TLB flush. */ pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL); if ( pg[i].u.free.need_tlbflush ) - pg[i].tlbflush_timestamp = tlbflush_current_time(); + page_set_tlbflush_timestamp(&pg[i]); /* This page is not a guest frame any more. */ page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */ diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h index c986b3f9af..ea8c7ed2fd 100644 --- a/xen/include/asm-arm/flushtlb.h +++ b/xen/include/asm-arm/flushtlb.h @@ -14,6 +14,11 @@ do { \ #define tlbflush_current_time() (0) +static inline void page_set_tlbflush_timestamp(struct page_info *page) +{ + page->tlbflush_timestamp = tlbflush_current_time(); +} + #if defined(CONFIG_ARM_32) # include #elif defined(CONFIG_ARM_64) diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h index 3c3e6b2769..f1389daa6f 100644 --- a/xen/include/asm-x86/flushtlb.h +++ b/xen/include/asm-x86/flushtlb.h @@ -24,6 +24,20 @@ DECLARE_PER_CPU(u32, tlbflush_time); #define tlbflush_current_time() tlbflush_clock +static inline void page_set_tlbflush_timestamp(struct page_info *page) +{ + /* + * Prevent storing a stale time stamp, which could happen if an update + * to tlbflush_clock plus a subsequent flush IPI happen between the + * reading of tlbflush_clock and the writing of the struct page_info + * field. + */ + ASSERT(local_irq_is_enabled()); + local_irq_disable(); + page->tlbflush_timestamp = tlbflush_current_time(); + local_irq_enable(); +} + /* * @cpu_stamp is the timestamp at last TLB flush for the CPU we are testing. * @lastuse_stamp is a timestamp taken when the PFN we are testing was last