head 1.2; access; symbols pkgsrc-2020Q3:1.1.0.2; locks; strict; comment @# @; 1.2 date 2020.11.06.21.45.49; author bouyer; state dead; branches; next 1.1; commitid WDPVMlrHGXeceSuC; 1.1 date 2020.10.01.12.41.19; author bouyer; state Exp; branches 1.1.2.1; next ; commitid t8BQtD5cJssqocqC; 1.1.2.1 date 2020.10.01.12.41.19; author bsiegert; state dead; branches; next 1.1.2.2; commitid 6fMH1kUIpq0AYCqC; 1.1.2.2 date 2020.10.04.20.44.32; author bsiegert; state Exp; branches; next ; commitid 6fMH1kUIpq0AYCqC; desc @@ 1.2 log @Update xenkernel413 and xentools413 to 4.13.2. This includes fixes for XSA up to XSA347, and an improved fix for XSA 286. @ text @$NetBSD: patch-XSA336,v 1.1 2020/10/01 12:41:19 bouyer Exp $ From: Roger Pau Monné Subject: x86/vpt: fix race when migrating timers between vCPUs The current vPT code will migrate the emulated timers between vCPUs (change the pt->vcpu field) while just holding the destination lock, either from create_periodic_time or pt_adjust_global_vcpu_target if the global target is adjusted. Changing the periodic_timer vCPU field in this way creates a race where a third party could grab the lock in the unlocked region of pt_adjust_global_vcpu_target (or before create_periodic_time performs the vcpu change) and then release the lock from a different vCPU, creating a locking imbalance. Introduce a per-domain rwlock in order to protect periodic_time migration between vCPU lists. Taking the lock in read mode prevents any timer from being migrated to a different vCPU, while taking it in write mode allows performing migration of timers across vCPUs. The per-vcpu locks are still used to protect all the other fields from the periodic_timer struct. Note that such migration shouldn't happen frequently, and hence there's no performance drop as a result of such locking. This is XSA-336. Reported-by: Igor Druzhinin Tested-by: Igor Druzhinin Signed-off-by: Roger Pau Monné Reviewed-by: Jan Beulich --- Changes since v2: - Re-order pt_adjust_vcpu to remove one if. - Fix pt_lock to not call pt_vcpu_lock, as we might end up using a stale value of pt->vcpu when taking the per-vcpu lock. Changes since v1: - Use a per-domain rwlock to protect timer vCPU migration. --- xen/arch/x86/hvm/hvm.c.orig +++ xen/arch/x86/hvm/hvm.c @@@@ -658,6 +658,8 @@@@ int hvm_domain_initialise(struct domain /* need link to containing domain */ d->arch.hvm.pl_time->domain = d; + rwlock_init(&d->arch.hvm.pl_time->pt_migrate); + /* Set the default IO Bitmap. */ if ( is_hardware_domain(d) ) { --- xen/arch/x86/hvm/vpt.c.orig +++ xen/arch/x86/hvm/vpt.c @@@@ -153,23 +153,32 @@@@ static int pt_irq_masked(struct periodic return 1; } -static void pt_lock(struct periodic_time *pt) +static void pt_vcpu_lock(struct vcpu *v) { - struct vcpu *v; + read_lock(&v->domain->arch.hvm.pl_time->pt_migrate); + spin_lock(&v->arch.hvm.tm_lock); +} - for ( ; ; ) - { - v = pt->vcpu; - spin_lock(&v->arch.hvm.tm_lock); - if ( likely(pt->vcpu == v) ) - break; - spin_unlock(&v->arch.hvm.tm_lock); - } +static void pt_vcpu_unlock(struct vcpu *v) +{ + spin_unlock(&v->arch.hvm.tm_lock); + read_unlock(&v->domain->arch.hvm.pl_time->pt_migrate); +} + +static void pt_lock(struct periodic_time *pt) +{ + /* + * We cannot use pt_vcpu_lock here, because we need to acquire the + * per-domain lock first and then (re-)fetch the value of pt->vcpu, or + * else we might be using a stale value of pt->vcpu. + */ + read_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); + spin_lock(&pt->vcpu->arch.hvm.tm_lock); } static void pt_unlock(struct periodic_time *pt) { - spin_unlock(&pt->vcpu->arch.hvm.tm_lock); + pt_vcpu_unlock(pt->vcpu); } static void pt_process_missed_ticks(struct periodic_time *pt) @@@@ -219,7 +228,7 @@@@ void pt_save_timer(struct vcpu *v) if ( v->pause_flags & VPF_blocked ) return; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) if ( !pt->do_not_freeze ) @@@@ -227,7 +236,7 @@@@ void pt_save_timer(struct vcpu *v) pt_freeze_time(v); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } void pt_restore_timer(struct vcpu *v) @@@@ -235,7 +244,7 @@@@ void pt_restore_timer(struct vcpu *v) struct list_head *head = &v->arch.hvm.tm_list; struct periodic_time *pt; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) { @@@@ -248,7 +257,7 @@@@ void pt_restore_timer(struct vcpu *v) pt_thaw_time(v); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } static void pt_timer_fn(void *data) @@@@ -309,7 +318,7 @@@@ int pt_update_irq(struct vcpu *v) int irq, pt_vector = -1; bool level; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); earliest_pt = NULL; max_lag = -1ULL; @@@@ -339,7 +348,7 @@@@ int pt_update_irq(struct vcpu *v) if ( earliest_pt == NULL ) { - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); return -1; } @@@@ -347,7 +356,7 @@@@ int pt_update_irq(struct vcpu *v) irq = earliest_pt->irq; level = earliest_pt->level; - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); switch ( earliest_pt->source ) { @@@@ -394,7 +403,7 @@@@ int pt_update_irq(struct vcpu *v) time_cb *cb = NULL; void *cb_priv; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); /* Make sure the timer is still on the list. */ list_for_each_entry ( pt, &v->arch.hvm.tm_list, list ) if ( pt == earliest_pt ) @@@@ -404,7 +413,7 @@@@ int pt_update_irq(struct vcpu *v) cb_priv = pt->priv; break; } - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); if ( cb != NULL ) cb(v, cb_priv); @@@@ -441,12 +450,12 @@@@ void pt_intr_post(struct vcpu *v, struct if ( intack.source == hvm_intsrc_vector ) return; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); pt = is_pt_irq(v, intack); if ( pt == NULL ) { - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); return; } @@@@ -455,7 +464,7 @@@@ void pt_intr_post(struct vcpu *v, struct cb = pt->cb; cb_priv = pt->priv; - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); if ( cb != NULL ) cb(v, cb_priv); @@@@ -466,12 +475,12 @@@@ void pt_migrate(struct vcpu *v) struct list_head *head = &v->arch.hvm.tm_list; struct periodic_time *pt; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) migrate_timer(&pt->timer, v->processor); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } void create_periodic_time( @@@@ -490,7 +499,7 @@@@ void create_periodic_time( destroy_periodic_time(pt); - spin_lock(&v->arch.hvm.tm_lock); + write_lock(&v->domain->arch.hvm.pl_time->pt_migrate); pt->pending_intr_nr = 0; pt->do_not_freeze = 0; @@@@ -540,7 +549,7 @@@@ void create_periodic_time( init_timer(&pt->timer, pt_timer_fn, pt, v->processor); set_timer(&pt->timer, pt->scheduled); - spin_unlock(&v->arch.hvm.tm_lock); + write_unlock(&v->domain->arch.hvm.pl_time->pt_migrate); } void destroy_periodic_time(struct periodic_time *pt) @@@@ -565,30 +574,20 @@@@ void destroy_periodic_time(struct period static void pt_adjust_vcpu(struct periodic_time *pt, struct vcpu *v) { - int on_list; - ASSERT(pt->source == PTSRC_isa || pt->source == PTSRC_ioapic); if ( pt->vcpu == NULL ) return; - pt_lock(pt); - on_list = pt->on_list; - if ( pt->on_list ) - list_del(&pt->list); - pt->on_list = 0; - pt_unlock(pt); - - spin_lock(&v->arch.hvm.tm_lock); + write_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); pt->vcpu = v; - if ( on_list ) + if ( pt->on_list ) { - pt->on_list = 1; + list_del(&pt->list); list_add(&pt->list, &v->arch.hvm.tm_list); - migrate_timer(&pt->timer, v->processor); } - spin_unlock(&v->arch.hvm.tm_lock); + write_unlock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); } void pt_adjust_global_vcpu_target(struct vcpu *v) --- xen/include/asm-x86/hvm/vpt.h.orig +++ xen/include/asm-x86/hvm/vpt.h @@@@ -128,6 +128,13 @@@@ struct pl_time { /* platform time */ struct RTCState vrtc; struct HPETState vhpet; struct PMTState vpmt; + /* + * rwlock to prevent periodic_time vCPU migration. Take the lock in read + * mode in order to prevent the vcpu field of periodic_time from changing. + * Lock must be taken in write mode when changes to the vcpu field are + * performed, as it allows exclusive access to all the timers of a domain. + */ + rwlock_t pt_migrate; /* guest_time = Xen sys time + stime_offset */ int64_t stime_offset; /* Ensures monotonicity in appropriate timer modes. */ @ 1.1 log @Add uptream fixes for XSA333, XSA334, XSA336, XSA337, XSA338, XSA339, XSA340, XSA342, XSA343, XSA344 bump PKGREVISION @ text @d1 1 a1 1 $NetBSD: $ @ 1.1.2.1 log @file patch-XSA336 was added on branch pkgsrc-2020Q3 on 2020-10-04 20:44:32 +0000 @ text @d1 285 @ 1.1.2.2 log @Pullup ticket #6332 - requested by bouyer sysutils/xenkernel411: security fix sysutils/xenkernel413: security fix Revisions pulled up: - sysutils/xenkernel411/Makefile 1.16 - sysutils/xenkernel411/distinfo 1.14 - sysutils/xenkernel411/patches/patch-XSA333 1.1 - sysutils/xenkernel411/patches/patch-XSA336 1.1 - sysutils/xenkernel411/patches/patch-XSA337 1.1 - sysutils/xenkernel411/patches/patch-XSA338 1.1 - sysutils/xenkernel411/patches/patch-XSA339 1.1 - sysutils/xenkernel411/patches/patch-XSA340 1.1 - sysutils/xenkernel411/patches/patch-XSA342 1.1 - sysutils/xenkernel411/patches/patch-XSA343 1.1 - sysutils/xenkernel411/patches/patch-XSA344 1.1 - sysutils/xenkernel413/Makefile 1.5 - sysutils/xenkernel413/distinfo 1.3 - sysutils/xenkernel413/patches/patch-XSA333 1.1 - sysutils/xenkernel413/patches/patch-XSA334 1.1 - sysutils/xenkernel413/patches/patch-XSA336 1.1 - sysutils/xenkernel413/patches/patch-XSA337 1.1 - sysutils/xenkernel413/patches/patch-XSA338 1.1 - sysutils/xenkernel413/patches/patch-XSA339 1.1 - sysutils/xenkernel413/patches/patch-XSA340 1.1 - sysutils/xenkernel413/patches/patch-XSA342 1.1 - sysutils/xenkernel413/patches/patch-XSA343 1.1 - sysutils/xenkernel413/patches/patch-XSA344 1.1 --- Module Name: pkgsrc Committed By: bouyer Date: Thu Oct 1 12:41:19 UTC 2020 Modified Files: pkgsrc/sysutils/xenkernel413: Makefile distinfo Added Files: pkgsrc/sysutils/xenkernel413/patches: patch-XSA333 patch-XSA334 patch-XSA336 patch-XSA337 patch-XSA338 patch-XSA339 patch-XSA340 patch-XSA342 patch-XSA343 patch-XSA344 Log Message: Add uptream fixes for XSA333, XSA334, XSA336, XSA337, XSA338, XSA339, XSA340, XSA342, XSA343, XSA344 bump PKGREVISION --- Module Name: pkgsrc Committed By: bouyer Date: Fri Oct 2 13:00:48 UTC 2020 Modified Files: pkgsrc/sysutils/xenkernel411: Makefile distinfo Added Files: pkgsrc/sysutils/xenkernel411/patches: patch-XSA333 patch-XSA336 patch-XSA337 patch-XSA338 patch-XSA339 patch-XSA340 patch-XSA342 patch-XSA343 patch-XSA344 Log Message: dd uptream fixes for XSA333, XSA336, XSA337, XSA338, XSA339, XSA340, XSA342, XSA343, XSA344 bump PKGREVISION @ text @a0 285 $NetBSD: patch-XSA336,v 1.1 2020/10/01 12:41:19 bouyer Exp $ From: Roger Pau Monné Subject: x86/vpt: fix race when migrating timers between vCPUs The current vPT code will migrate the emulated timers between vCPUs (change the pt->vcpu field) while just holding the destination lock, either from create_periodic_time or pt_adjust_global_vcpu_target if the global target is adjusted. Changing the periodic_timer vCPU field in this way creates a race where a third party could grab the lock in the unlocked region of pt_adjust_global_vcpu_target (or before create_periodic_time performs the vcpu change) and then release the lock from a different vCPU, creating a locking imbalance. Introduce a per-domain rwlock in order to protect periodic_time migration between vCPU lists. Taking the lock in read mode prevents any timer from being migrated to a different vCPU, while taking it in write mode allows performing migration of timers across vCPUs. The per-vcpu locks are still used to protect all the other fields from the periodic_timer struct. Note that such migration shouldn't happen frequently, and hence there's no performance drop as a result of such locking. This is XSA-336. Reported-by: Igor Druzhinin Tested-by: Igor Druzhinin Signed-off-by: Roger Pau Monné Reviewed-by: Jan Beulich --- Changes since v2: - Re-order pt_adjust_vcpu to remove one if. - Fix pt_lock to not call pt_vcpu_lock, as we might end up using a stale value of pt->vcpu when taking the per-vcpu lock. Changes since v1: - Use a per-domain rwlock to protect timer vCPU migration. --- xen/arch/x86/hvm/hvm.c.orig +++ xen/arch/x86/hvm/hvm.c @@@@ -658,6 +658,8 @@@@ int hvm_domain_initialise(struct domain /* need link to containing domain */ d->arch.hvm.pl_time->domain = d; + rwlock_init(&d->arch.hvm.pl_time->pt_migrate); + /* Set the default IO Bitmap. */ if ( is_hardware_domain(d) ) { --- xen/arch/x86/hvm/vpt.c.orig +++ xen/arch/x86/hvm/vpt.c @@@@ -153,23 +153,32 @@@@ static int pt_irq_masked(struct periodic return 1; } -static void pt_lock(struct periodic_time *pt) +static void pt_vcpu_lock(struct vcpu *v) { - struct vcpu *v; + read_lock(&v->domain->arch.hvm.pl_time->pt_migrate); + spin_lock(&v->arch.hvm.tm_lock); +} - for ( ; ; ) - { - v = pt->vcpu; - spin_lock(&v->arch.hvm.tm_lock); - if ( likely(pt->vcpu == v) ) - break; - spin_unlock(&v->arch.hvm.tm_lock); - } +static void pt_vcpu_unlock(struct vcpu *v) +{ + spin_unlock(&v->arch.hvm.tm_lock); + read_unlock(&v->domain->arch.hvm.pl_time->pt_migrate); +} + +static void pt_lock(struct periodic_time *pt) +{ + /* + * We cannot use pt_vcpu_lock here, because we need to acquire the + * per-domain lock first and then (re-)fetch the value of pt->vcpu, or + * else we might be using a stale value of pt->vcpu. + */ + read_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); + spin_lock(&pt->vcpu->arch.hvm.tm_lock); } static void pt_unlock(struct periodic_time *pt) { - spin_unlock(&pt->vcpu->arch.hvm.tm_lock); + pt_vcpu_unlock(pt->vcpu); } static void pt_process_missed_ticks(struct periodic_time *pt) @@@@ -219,7 +228,7 @@@@ void pt_save_timer(struct vcpu *v) if ( v->pause_flags & VPF_blocked ) return; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) if ( !pt->do_not_freeze ) @@@@ -227,7 +236,7 @@@@ void pt_save_timer(struct vcpu *v) pt_freeze_time(v); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } void pt_restore_timer(struct vcpu *v) @@@@ -235,7 +244,7 @@@@ void pt_restore_timer(struct vcpu *v) struct list_head *head = &v->arch.hvm.tm_list; struct periodic_time *pt; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) { @@@@ -248,7 +257,7 @@@@ void pt_restore_timer(struct vcpu *v) pt_thaw_time(v); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } static void pt_timer_fn(void *data) @@@@ -309,7 +318,7 @@@@ int pt_update_irq(struct vcpu *v) int irq, pt_vector = -1; bool level; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); earliest_pt = NULL; max_lag = -1ULL; @@@@ -339,7 +348,7 @@@@ int pt_update_irq(struct vcpu *v) if ( earliest_pt == NULL ) { - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); return -1; } @@@@ -347,7 +356,7 @@@@ int pt_update_irq(struct vcpu *v) irq = earliest_pt->irq; level = earliest_pt->level; - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); switch ( earliest_pt->source ) { @@@@ -394,7 +403,7 @@@@ int pt_update_irq(struct vcpu *v) time_cb *cb = NULL; void *cb_priv; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); /* Make sure the timer is still on the list. */ list_for_each_entry ( pt, &v->arch.hvm.tm_list, list ) if ( pt == earliest_pt ) @@@@ -404,7 +413,7 @@@@ int pt_update_irq(struct vcpu *v) cb_priv = pt->priv; break; } - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); if ( cb != NULL ) cb(v, cb_priv); @@@@ -441,12 +450,12 @@@@ void pt_intr_post(struct vcpu *v, struct if ( intack.source == hvm_intsrc_vector ) return; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); pt = is_pt_irq(v, intack); if ( pt == NULL ) { - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); return; } @@@@ -455,7 +464,7 @@@@ void pt_intr_post(struct vcpu *v, struct cb = pt->cb; cb_priv = pt->priv; - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); if ( cb != NULL ) cb(v, cb_priv); @@@@ -466,12 +475,12 @@@@ void pt_migrate(struct vcpu *v) struct list_head *head = &v->arch.hvm.tm_list; struct periodic_time *pt; - spin_lock(&v->arch.hvm.tm_lock); + pt_vcpu_lock(v); list_for_each_entry ( pt, head, list ) migrate_timer(&pt->timer, v->processor); - spin_unlock(&v->arch.hvm.tm_lock); + pt_vcpu_unlock(v); } void create_periodic_time( @@@@ -490,7 +499,7 @@@@ void create_periodic_time( destroy_periodic_time(pt); - spin_lock(&v->arch.hvm.tm_lock); + write_lock(&v->domain->arch.hvm.pl_time->pt_migrate); pt->pending_intr_nr = 0; pt->do_not_freeze = 0; @@@@ -540,7 +549,7 @@@@ void create_periodic_time( init_timer(&pt->timer, pt_timer_fn, pt, v->processor); set_timer(&pt->timer, pt->scheduled); - spin_unlock(&v->arch.hvm.tm_lock); + write_unlock(&v->domain->arch.hvm.pl_time->pt_migrate); } void destroy_periodic_time(struct periodic_time *pt) @@@@ -565,30 +574,20 @@@@ void destroy_periodic_time(struct period static void pt_adjust_vcpu(struct periodic_time *pt, struct vcpu *v) { - int on_list; - ASSERT(pt->source == PTSRC_isa || pt->source == PTSRC_ioapic); if ( pt->vcpu == NULL ) return; - pt_lock(pt); - on_list = pt->on_list; - if ( pt->on_list ) - list_del(&pt->list); - pt->on_list = 0; - pt_unlock(pt); - - spin_lock(&v->arch.hvm.tm_lock); + write_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); pt->vcpu = v; - if ( on_list ) + if ( pt->on_list ) { - pt->on_list = 1; + list_del(&pt->list); list_add(&pt->list, &v->arch.hvm.tm_list); - migrate_timer(&pt->timer, v->processor); } - spin_unlock(&v->arch.hvm.tm_lock); + write_unlock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate); } void pt_adjust_global_vcpu_target(struct vcpu *v) --- xen/include/asm-x86/hvm/vpt.h.orig +++ xen/include/asm-x86/hvm/vpt.h @@@@ -128,6 +128,13 @@@@ struct pl_time { /* platform time */ struct RTCState vrtc; struct HPETState vhpet; struct PMTState vpmt; + /* + * rwlock to prevent periodic_time vCPU migration. Take the lock in read + * mode in order to prevent the vcpu field of periodic_time from changing. + * Lock must be taken in write mode when changes to the vcpu field are + * performed, as it allows exclusive access to all the timers of a domain. + */ + rwlock_t pt_migrate; /* guest_time = Xen sys time + stime_offset */ int64_t stime_offset; /* Ensures monotonicity in appropriate timer modes. */ @