Message ID | 20210222205833.15479-2-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
Series | xen/events: avoid removing an event channel while handling it | expand |
On Mon, Feb 22, 2021 at 01:58:33PM -0700, Tim Gardner wrote: > From: Juergen Gross <jgross@suse.com> > > Today it can happen that an event channel is being removed from the > system while the event handling loop is active. This can lead to a > race resulting in crashes or WARN() splats when trying to access the > irq_info structure related to the event channel. > > Fix this problem by using a rwlock taken as reader in the event > handling loop and as writer when deallocating the irq_info structure. > > As the observed problem was a NULL dereference in evtchn_from_irq() > make this function more robust against races by testing the irq_info > pointer to be not NULL before dereferencing it. > > And finally make all accesses to evtchn_to_irq[row][col] atomic ones > in order to avoid seeing partial updates of an array element in irq > handling. Note that irq handling can be entered only for event channels > which have been valid before, so any not populated row isn't a problem > in this regard, as rows are only ever added and never removed. > > This is XSA-331. > > Cc: stable@vger.kernel.org > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Reported-by: Jinoh Kang <luke1337@theori.io> > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > Reviewed-by: Wei Liu <wl@xen.org> > (back ported from commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2) > CVE-2020-27675 > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > > Conflicts: > drivers/xen/events/events_base.c > > Minor context difference. > --- > drivers/xen/events/events_base.c | 43 +++++++++++++++++++++++++++----- > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index 499eff7d3f65..30dca236fd78 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -33,6 +33,7 @@ > #include <linux/slab.h> > #include <linux/irqnr.h> > #include <linux/pci.h> > +#include <linux/spinlock.h> > > #ifdef CONFIG_X86 > #include <asm/desc.h> > @@ -70,6 +71,23 @@ const struct evtchn_ops *evtchn_ops; > */ > static DEFINE_MUTEX(irq_mapping_update_lock); > > +/* > + * Lock protecting event handling loop against removing event channels. > + * Adding of event channels is no issue as the associated IRQ becomes active > + * only after everything is setup (before request_[threaded_]irq() the handler > + * can't be entered for an event, as the event channel will be unmasked only > + * then). > + */ > +static DEFINE_RWLOCK(evtchn_rwlock); > + > +/* > + * Lock hierarchy: > + * > + * irq_mapping_update_lock > + * evtchn_rwlock > + * IRQ-desc lock > + */ > + > static LIST_HEAD(xen_irq_list_head); > > /* IRQ <-> VIRQ mapping. */ > @@ -102,7 +120,7 @@ static void clear_evtchn_to_irq_row(unsigned row) > unsigned col; > > for (col = 0; col < EVTCHN_PER_ROW; col++) > - evtchn_to_irq[row][col] = -1; > + WRITE_ONCE(evtchn_to_irq[row][col], -1); > } > > static void clear_evtchn_to_irq_all(void) > @@ -139,7 +157,7 @@ static int set_evtchn_to_irq(unsigned evtchn, unsigned irq) > clear_evtchn_to_irq_row(row); > } > > - evtchn_to_irq[row][col] = irq; > + WRITE_ONCE(evtchn_to_irq[row][col], irq); > return 0; > } > > @@ -149,7 +167,7 @@ int get_evtchn_to_irq(unsigned evtchn) > return -1; > if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL) > return -1; > - return evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]; > + return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]); > } > > /* Get info for IRQ */ > @@ -247,10 +265,14 @@ static void xen_irq_info_cleanup(struct irq_info *info) > */ > unsigned int evtchn_from_irq(unsigned irq) > { > - if (WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)) > + const struct irq_info *info = NULL; > + > + if (likely(irq < nr_irqs)) > + info = info_for_irq(irq); > + if (!info) > return 0; > > - return info_for_irq(irq)->evtchn; > + return info->evtchn; > } > > unsigned irq_from_evtchn(unsigned int evtchn) > @@ -425,17 +447,22 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi) > > static void xen_free_irq(unsigned irq) > { > - struct irq_info *info = irq_get_handler_data(irq); > + struct irq_info *info = info_for_irq(irq); This change is not part of commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2. NACK. Cascardo. > + unsigned long flags; > > if (WARN_ON(!info)) > return; > > + write_lock_irqsave(&evtchn_rwlock, flags); > + > list_del(&info->list); > > irq_set_handler_data(irq, NULL); > > WARN_ON(info->refcnt > 0); > > + write_unlock_irqrestore(&evtchn_rwlock, flags); > + > kfree(info); > > /* Legacy IRQ descriptors are managed by the arch. */ > @@ -1218,6 +1245,8 @@ static void __xen_evtchn_do_upcall(void) > struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu); > int cpu = smp_processor_id(); > > + read_lock(&evtchn_rwlock); > + > do { > vcpu_info->evtchn_upcall_pending = 0; > > @@ -1228,6 +1257,8 @@ static void __xen_evtchn_do_upcall(void) > virt_rmb(); /* Hypervisor can set upcall pending. */ > > } while (vcpu_info->evtchn_upcall_pending); > + > + read_unlock(&evtchn_rwlock); > } > > void xen_evtchn_do_upcall(struct pt_regs *regs) > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 499eff7d3f65..30dca236fd78 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -33,6 +33,7 @@ #include <linux/slab.h> #include <linux/irqnr.h> #include <linux/pci.h> +#include <linux/spinlock.h> #ifdef CONFIG_X86 #include <asm/desc.h> @@ -70,6 +71,23 @@ const struct evtchn_ops *evtchn_ops; */ static DEFINE_MUTEX(irq_mapping_update_lock); +/* + * Lock protecting event handling loop against removing event channels. + * Adding of event channels is no issue as the associated IRQ becomes active + * only after everything is setup (before request_[threaded_]irq() the handler + * can't be entered for an event, as the event channel will be unmasked only + * then). + */ +static DEFINE_RWLOCK(evtchn_rwlock); + +/* + * Lock hierarchy: + * + * irq_mapping_update_lock + * evtchn_rwlock + * IRQ-desc lock + */ + static LIST_HEAD(xen_irq_list_head); /* IRQ <-> VIRQ mapping. */ @@ -102,7 +120,7 @@ static void clear_evtchn_to_irq_row(unsigned row) unsigned col; for (col = 0; col < EVTCHN_PER_ROW; col++) - evtchn_to_irq[row][col] = -1; + WRITE_ONCE(evtchn_to_irq[row][col], -1); } static void clear_evtchn_to_irq_all(void) @@ -139,7 +157,7 @@ static int set_evtchn_to_irq(unsigned evtchn, unsigned irq) clear_evtchn_to_irq_row(row); } - evtchn_to_irq[row][col] = irq; + WRITE_ONCE(evtchn_to_irq[row][col], irq); return 0; } @@ -149,7 +167,7 @@ int get_evtchn_to_irq(unsigned evtchn) return -1; if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL) return -1; - return evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]; + return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]); } /* Get info for IRQ */ @@ -247,10 +265,14 @@ static void xen_irq_info_cleanup(struct irq_info *info) */ unsigned int evtchn_from_irq(unsigned irq) { - if (WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)) + const struct irq_info *info = NULL; + + if (likely(irq < nr_irqs)) + info = info_for_irq(irq); + if (!info) return 0; - return info_for_irq(irq)->evtchn; + return info->evtchn; } unsigned irq_from_evtchn(unsigned int evtchn) @@ -425,17 +447,22 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi) static void xen_free_irq(unsigned irq) { - struct irq_info *info = irq_get_handler_data(irq); + struct irq_info *info = info_for_irq(irq); + unsigned long flags; if (WARN_ON(!info)) return; + write_lock_irqsave(&evtchn_rwlock, flags); + list_del(&info->list); irq_set_handler_data(irq, NULL); WARN_ON(info->refcnt > 0); + write_unlock_irqrestore(&evtchn_rwlock, flags); + kfree(info); /* Legacy IRQ descriptors are managed by the arch. */ @@ -1218,6 +1245,8 @@ static void __xen_evtchn_do_upcall(void) struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu); int cpu = smp_processor_id(); + read_lock(&evtchn_rwlock); + do { vcpu_info->evtchn_upcall_pending = 0; @@ -1228,6 +1257,8 @@ static void __xen_evtchn_do_upcall(void) virt_rmb(); /* Hypervisor can set upcall pending. */ } while (vcpu_info->evtchn_upcall_pending); + + read_unlock(&evtchn_rwlock); } void xen_evtchn_do_upcall(struct pt_regs *regs)