NACK: [PATCH] xen/events: avoid removing an event channel while handling it
Thadeu Lima de Souza Cascardo
cascardo at canonical.com
Mon Feb 22 21:13:33 UTC 2021
On Mon, Feb 22, 2021 at 01:58:33PM -0700, Tim Gardner wrote:
> From: Juergen Gross <jgross at 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 at vger.kernel.org
> Reported-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
> Reported-by: Jinoh Kang <luke1337 at theori.io>
> Signed-off-by: Juergen Gross <jgross at suse.com>
> Reviewed-by: Stefano Stabellini <sstabellini at kernel.org>
> Reviewed-by: Wei Liu <wl at xen.org>
> (back ported from commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2)
> CVE-2020-27675
> Signed-off-by: Tim Gardner <tim.gardner at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list