From nobody Mon May 13 19:05:13 2024 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C158C1586E1; Wed, 27 Mar 2024 12:16:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711541817; cv=none; b=F0tG/Krxdq7mjjeOhtXsDKlrfBmLf5nm5xJcUELrWRJqGfpfMyOxOvWH96ZgjWrjeGAYdwAl3utoIAEq7Mq5M1wTf4q2q/HR1IbXtHr7CHyds/el599Zcz92bDfDAzKsraRvkAmmHV/XzFm5GQVNfyKj3Hr5MEAAjBMu+twiyLI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711541817; c=relaxed/simple; bh=7Fiu/+zYl615fnO/PXo8KhB4yYZ3wdALVuWw+qLJ3og=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version; b=aUI48jCCH9DS/gIItX2+B+JXRE4BWPWOVQT4OHQ4RnK8XQ7mOVDVvUmrH6b4MsOBN79MgNXIoYMLvuO2aBpYQg2e2Bb/B3vniZyYkx+YuxLWnzB3V086GrsL1X65tV0pUCAz73czdRociAUjWN8Cd9AOKjVWlViKWqE7gXlsIKM= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=DDoGitgD; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="DDoGitgD" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6C52BC433C7; Wed, 27 Mar 2024 12:16:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1711541817; bh=7Fiu/+zYl615fnO/PXo8KhB4yYZ3wdALVuWw+qLJ3og=; h=From:To:Cc:Subject:Date:From; b=DDoGitgDEsheYXGzgCAp/bW3+E6dW4o0nRO+U3LfCGFqboRjTJ3+HJERz0b+MXD9v MefWPB7FPO9RWQOW9rTWX/vhYC8D/cEevYpi2uYtAYmfMcHAkPPsI4Ff3qcTt1XKzd 2Mqq9RcPEQjQiW/5WUzbNft33aeKSzhiqBaXRMK4YgInZxenxqCLfuMVlIzePmwpE1 TadxPoVrxjN6pshpIfhBqDgA1RdM2PUn1LPvBsUkzaVIq/Y4ER9b2TmJsjukLJMArj yWIMu2Ben2t1b34f31xXaAIpRj+NqreoPs0phMibREi+DmQ060LttUsv7IYU3UQzFw FdcbqNbqa+o/Q== From: Sasha Levin To: stable@vger.kernel.org, alex.williamson@redhat.com Cc: Reinette Chatre , Kevin Tian , Eric Auger , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: FAILED: Patch "vfio/pci: Create persistent INTx handler" failed to apply to 5.15-stable tree Date: Wed, 27 Mar 2024 08:16:55 -0400 Message-ID: <20240327121655.2833110-1-sashal@kernel.org> X-Mailer: git-send-email 2.43.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Hint: ignore X-stable: review Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" The patch below does not apply to the 5.15-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to . Thanks, Sasha Reported-by: Reinette Chatre Reviewed-by: Eric Auger Reviewed-by: Kevin Tian Reviewed-by: Reinette Chatre ------------------ original commit in Linus's tree ------------------ From 18c198c96a815c962adc2b9b77909eec0be7df4d Mon Sep 17 00:00:00 2001 From: Alex Williamson Date: Fri, 8 Mar 2024 16:05:25 -0700 Subject: [PATCH] vfio/pci: Create persistent INTx handler A vulnerability exists where the eventfd for INTx signaling can be deconfigured, which unregisters the IRQ handler but still allows eventfds to be signaled with a NULL context through the SET_IRQS ioctl or through unmask irqfd if the device interrupt is pending. Ideally this could be solved with some additional locking; the igate mutex serializes the ioctl and config space accesses, and the interrupt handler is unregistered relative to the trigger, but the irqfd path runs asynchronous to those. The igate mutex cannot be acquired from the atomic context of the eventfd wake function. Disabling the irqfd relative to the eventfd registration is potentially incompatible with existing userspace. As a result, the solution implemented here moves configuration of the INTx interrupt handler to track the lifetime of the INTx context object and irq_type configuration, rather than registration of a particular trigger eventfd. Synchronization is added between the ioctl path and eventfd_signal() wrapper such that the eventfd trigger can be dynamically updated relative to in-flight interrupts or irqfd callbacks. Cc: Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver") Reported-by: Reinette Chatre Reviewed-by: Kevin Tian Reviewed-by: Reinette Chatre Reviewed-by: Eric Auger Link: https://lore.kernel.org/r/20240308230557.805580-5-alex.williamson@red= hat.com Signed-off-by: Alex Williamson --- drivers/vfio/pci/vfio_pci_intrs.c | 145 ++++++++++++++++-------------- 1 file changed, 78 insertions(+), 67 deletions(-) diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_= intrs.c index 75c85eec21b3c..fb5392b749fff 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -90,11 +90,15 @@ static void vfio_send_intx_eventfd(void *opaque, void *= unused) =20 if (likely(is_intx(vdev) && !vdev->virq_disabled)) { struct vfio_pci_irq_ctx *ctx; + struct eventfd_ctx *trigger; =20 ctx =3D vfio_irq_ctx_get(vdev, 0); if (WARN_ON_ONCE(!ctx)) return; - eventfd_signal(ctx->trigger); + + trigger =3D READ_ONCE(ctx->trigger); + if (likely(trigger)) + eventfd_signal(trigger); } } =20 @@ -253,100 +257,100 @@ static irqreturn_t vfio_intx_handler(int irq, void = *dev_id) return ret; } =20 -static int vfio_intx_enable(struct vfio_pci_core_device *vdev) +static int vfio_intx_enable(struct vfio_pci_core_device *vdev, + struct eventfd_ctx *trigger) { + struct pci_dev *pdev =3D vdev->pdev; struct vfio_pci_irq_ctx *ctx; + unsigned long irqflags; + char *name; + int ret; =20 if (!is_irq_none(vdev)) return -EINVAL; =20 - if (!vdev->pdev->irq) + if (!pdev->irq) return -ENODEV; =20 + name =3D kasprintf(GFP_KERNEL_ACCOUNT, "vfio-intx(%s)", pci_name(pdev)); + if (!name) + return -ENOMEM; + ctx =3D vfio_irq_ctx_alloc(vdev, 0); if (!ctx) return -ENOMEM; =20 + ctx->name =3D name; + ctx->trigger =3D trigger; + /* - * If the virtual interrupt is masked, restore it. Devices - * supporting DisINTx can be masked at the hardware level - * here, non-PCI-2.3 devices will have to wait until the - * interrupt is enabled. + * Fill the initial masked state based on virq_disabled. After + * enable, changing the DisINTx bit in vconfig directly changes INTx + * masking. igate prevents races during setup, once running masked + * is protected via irqlock. + * + * Devices supporting DisINTx also reflect the current mask state in + * the physical DisINTx bit, which is not affected during IRQ setup. + * + * Devices without DisINTx support require an exclusive interrupt. + * IRQ masking is performed at the IRQ chip. Again, igate protects + * against races during setup and IRQ handlers and irqfds are not + * yet active, therefore masked is stable and can be used to + * conditionally auto-enable the IRQ. + * + * irq_type must be stable while the IRQ handler is registered, + * therefore it must be set before request_irq(). */ ctx->masked =3D vdev->virq_disabled; - if (vdev->pci_2_3) - pci_intx(vdev->pdev, !ctx->masked); + if (vdev->pci_2_3) { + pci_intx(pdev, !ctx->masked); + irqflags =3D IRQF_SHARED; + } else { + irqflags =3D ctx->masked ? IRQF_NO_AUTOEN : 0; + } =20 vdev->irq_type =3D VFIO_PCI_INTX_IRQ_INDEX; =20 + ret =3D request_irq(pdev->irq, vfio_intx_handler, + irqflags, ctx->name, vdev); + if (ret) { + vdev->irq_type =3D VFIO_PCI_NUM_IRQS; + kfree(name); + vfio_irq_ctx_free(vdev, ctx, 0); + return ret; + } + return 0; } =20 -static int vfio_intx_set_signal(struct vfio_pci_core_device *vdev, int fd) +static int vfio_intx_set_signal(struct vfio_pci_core_device *vdev, + struct eventfd_ctx *trigger) { struct pci_dev *pdev =3D vdev->pdev; - unsigned long irqflags =3D IRQF_SHARED; struct vfio_pci_irq_ctx *ctx; - struct eventfd_ctx *trigger; - unsigned long flags; - int ret; + struct eventfd_ctx *old; =20 ctx =3D vfio_irq_ctx_get(vdev, 0); if (WARN_ON_ONCE(!ctx)) return -EINVAL; =20 - if (ctx->trigger) { - free_irq(pdev->irq, vdev); - kfree(ctx->name); - eventfd_ctx_put(ctx->trigger); - ctx->trigger =3D NULL; - } - - if (fd < 0) /* Disable only */ - return 0; - - ctx->name =3D kasprintf(GFP_KERNEL_ACCOUNT, "vfio-intx(%s)", - pci_name(pdev)); - if (!ctx->name) - return -ENOMEM; - - trigger =3D eventfd_ctx_fdget(fd); - if (IS_ERR(trigger)) { - kfree(ctx->name); - return PTR_ERR(trigger); - } + old =3D ctx->trigger; =20 - ctx->trigger =3D trigger; + WRITE_ONCE(ctx->trigger, trigger); =20 - /* - * Devices without DisINTx support require an exclusive interrupt, - * IRQ masking is performed at the IRQ chip. The masked status is - * protected by vdev->irqlock. Setup the IRQ without auto-enable and - * unmask as necessary below under lock. DisINTx is unmodified by - * the IRQ configuration and may therefore use auto-enable. - */ - if (!vdev->pci_2_3) - irqflags =3D IRQF_NO_AUTOEN; - - ret =3D request_irq(pdev->irq, vfio_intx_handler, - irqflags, ctx->name, vdev); - if (ret) { - ctx->trigger =3D NULL; - kfree(ctx->name); - eventfd_ctx_put(trigger); - return ret; + /* Releasing an old ctx requires synchronizing in-flight users */ + if (old) { + synchronize_irq(pdev->irq); + vfio_virqfd_flush_thread(&ctx->unmask); + eventfd_ctx_put(old); } =20 - spin_lock_irqsave(&vdev->irqlock, flags); - if (!vdev->pci_2_3 && !ctx->masked) - enable_irq(pdev->irq); - spin_unlock_irqrestore(&vdev->irqlock, flags); - return 0; } =20 static void vfio_intx_disable(struct vfio_pci_core_device *vdev) { + struct pci_dev *pdev =3D vdev->pdev; struct vfio_pci_irq_ctx *ctx; =20 ctx =3D vfio_irq_ctx_get(vdev, 0); @@ -354,10 +358,13 @@ static void vfio_intx_disable(struct vfio_pci_core_de= vice *vdev) if (ctx) { vfio_virqfd_disable(&ctx->unmask); vfio_virqfd_disable(&ctx->mask); + free_irq(pdev->irq, vdev); + if (ctx->trigger) + eventfd_ctx_put(ctx->trigger); + kfree(ctx->name); + vfio_irq_ctx_free(vdev, ctx, 0); } - vfio_intx_set_signal(vdev, -1); vdev->irq_type =3D VFIO_PCI_NUM_IRQS; - vfio_irq_ctx_free(vdev, ctx, 0); } =20 /* @@ -641,19 +648,23 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_= core_device *vdev, return -EINVAL; =20 if (flags & VFIO_IRQ_SET_DATA_EVENTFD) { + struct eventfd_ctx *trigger =3D NULL; int32_t fd =3D *(int32_t *)data; int ret; =20 - if (is_intx(vdev)) - return vfio_intx_set_signal(vdev, fd); + if (fd >=3D 0) { + trigger =3D eventfd_ctx_fdget(fd); + if (IS_ERR(trigger)) + return PTR_ERR(trigger); + } =20 - ret =3D vfio_intx_enable(vdev); - if (ret) - return ret; + if (is_intx(vdev)) + ret =3D vfio_intx_set_signal(vdev, trigger); + else + ret =3D vfio_intx_enable(vdev, trigger); =20 - ret =3D vfio_intx_set_signal(vdev, fd); - if (ret) - vfio_intx_disable(vdev); + if (ret && trigger) + eventfd_ctx_put(trigger); =20 return ret; } --=20 2.43.0