From nobody Wed May 15 13:37:19 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) client-ip=192.237.175.120; envelope-from=xen-devel-bounces@lists.xenproject.org; helo=lists.xenproject.org; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org ARC-Seal: i=1; a=rsa-sha256; t=1694782352; cv=none; d=zohomail.com; s=zohoarc; b=YnU/5NT7LBwtqbyv0Zb4MsIGNNB5EAuxuYX8IEN7941zxzA0vlO4+M016ii0KTtj0f+Fbt+B1ivC9U2lQVB1wV8UzzaAI2o2vEviPB//Y9Kq7bO/4NtSzFBow+E12m6Ko5IJLHXdwiuNvqgjsbX76TX9YvW8AuLkZinDh0gSN5Y= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1694782352; h=Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To; bh=TgbWgVqJYFklDRXTfxuDdZGkTlQvmt/ixcBP2ZGtcoY=; b=OW9ygadBAdMjKpU85nJLrMDQb/8GONMmgDoGELkF+bS/q5G9TDZcark1kQRZ0wrwF15MXgWy+mBVzy7eAIrdzaOmz4yUO++EmocGU0LtyOkmCZclsJ50sgIbcuID/Sbt2g3l4hzMgcXPEKuonSL3TrhZioQQTJmJRIKgkEKPR74= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Return-Path: Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) by mx.zohomail.com with SMTPS id 1694782352591215.1834750490042; Fri, 15 Sep 2023 05:52:32 -0700 (PDT) Received: from list by lists.xenproject.org with outflank-mailman.603133.940031 (Exim 4.92) (envelope-from ) id 1qh8Ir-00052P-SN; Fri, 15 Sep 2023 12:52:09 +0000 Received: by outflank-mailman (output) from mailman id 603133.940031; Fri, 15 Sep 2023 12:52:09 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qh8Ir-00052I-PP; Fri, 15 Sep 2023 12:52:09 +0000 Received: by outflank-mailman (input) for mailman id 603133; Fri, 15 Sep 2023 12:52:08 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qh8Iq-00052C-CB for xen-devel@lists.xenproject.org; Fri, 15 Sep 2023 12:52:08 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qh8Ip-0003M5-LO; Fri, 15 Sep 2023 12:52:07 +0000 Received: from 54-240-197-224.amazon.com ([54.240.197.224] helo=dev-dsk-jgrall-1b-035652ec.eu-west-1.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1qh8Ip-0000Am-93; Fri, 15 Sep 2023 12:52:07 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:MIME-Version:Message-Id:Date: Subject:Cc:To:From; bh=TgbWgVqJYFklDRXTfxuDdZGkTlQvmt/ixcBP2ZGtcoY=; b=TNIg38 PztBXw1shwLa6h+aXlyOwp+L9tkMYvvo0GbRzRjZhV4vsOhErzqSP5qBPIoy0FochebSKOc/v4Tv4 Q3ncuN+fg19az95KGIuuL4V4ZwafUx/3F51eECgCzWZXqrQ9KanI3bbYoiBSnwl0EZ+p3zMYkP31p QUNmO1H4riY=; From: Julien Grall To: xen-devel@lists.xenproject.org Cc: Henry.Wang@arm.com, Julien Grall , Wei Liu , Anthony PERARD , Juergen Gross Subject: [PATCH for-4.18 v2] tools/light: Revoke permissions when a PCI detach for HVM domain Date: Fri, 15 Sep 2023 13:52:04 +0100 Message-Id: <20230915125204.22719-1-julien@xen.org> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @xen.org) X-ZM-MESSAGEID: 1694782353364100001 Content-Type: text/plain; charset="utf-8" From: Julien Grall Currently, libxl will grant IOMEM, I/O port and IRQ permissions when a PCI is attached (see pci_add_dm_done()) for all domain types. However, the permissions are only revoked for non-HVM domain (see do_pci_remove()). This means that HVM domains will be left with extra permissions. While this look bad on the paper, the IRQ permissions should be revoked when the Device Model call xc_physdev_unmap_pirq() and such domain cannot directly mapped I/O port and IOMEM regions. Instead, this has to be done by a Device Model. The Device Model can only run in dom0 or PV stubdomain (upstream libxl doesn't have support for HVM/PVH stubdomain). For PV/PVH stubdomain, the permission are properly revoked, so there is no security concern. This leaves dom0. There are two cases: 1) Privileged: Anyone gaining access to the Device Model would already have large control on the host. 2) Deprivileged: PCI passthrough require PHYSDEV operations which are not accessible when the Device Model is restricted. So overall, it is believed that the extra permissions cannot be exploited. Rework the code so the permissions are all removed for HVM domains. This needs to happen after the QEMU has detached the device. So the revocation is now moved to pci_remove_detached(). Also add a comment on top of the error message when the PIRQ cannot be unbind to explain this could be a spurious error as QEMU may have already done it. Signed-off-by: Julien Grall Reviewed-by: Anthony PERARD --- Changes since v1: * Move the code to revoke in pci_remove_detached() * Add a comment on top of the PIRQ unbind error path * Use goto to deal with errors. --- tools/libs/light/libxl_pci.c | 137 ++++++++++++++++++++--------------- 1 file changed, 77 insertions(+), 60 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 7f5f170e6eb0..96cb4da0794e 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1968,7 +1968,6 @@ static void do_pci_remove(libxl__egc *egc, pci_remove= _state *prs) goto out_fail; } =20 - rc =3D ERROR_FAIL; if (type =3D=3D LIBXL_DOMAIN_TYPE_HVM) { prs->hvm =3D true; switch (libxl__device_model_version_running(gc, domid)) { @@ -1989,65 +1988,7 @@ static void do_pci_remove(libxl__egc *egc, pci_remov= e_state *prs) rc =3D ERROR_INVAL; goto out_fail; } - } else { - char *sysfs_path =3D GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource",= pci->domain, - pci->bus, pci->dev, pci->func); - FILE *f =3D fopen(sysfs_path, "r"); - unsigned int start =3D 0, end =3D 0, flags =3D 0, size =3D 0; - int irq =3D 0; - int i; - - if (f =3D=3D NULL) { - LOGED(ERROR, domid, "Couldn't open %s", sysfs_path); - goto skip1; - } - for (i =3D 0; i < PROC_PCI_NUM_RESOURCES; i++) { - if (fscanf(f, "0x%x 0x%x 0x%x\n", &start, &end, &flags) !=3D 3) - continue; - size =3D end - start + 1; - if (start) { - if (flags & PCI_BAR_IO) { - rc =3D xc_domain_ioport_permission(ctx->xch, domid, st= art, size, 0); - if (rc < 0) - LOGED(ERROR, domid, - "xc_domain_ioport_permission error 0x%x/0x%x= ", - start, - size); - } else { - rc =3D xc_domain_iomem_permission(ctx->xch, domid, sta= rt>>XC_PAGE_SHIFT, - (size+(XC_PAGE_SIZE-1)= )>>XC_PAGE_SHIFT, 0); - if (rc < 0) - LOGED(ERROR, domid, - "xc_domain_iomem_permission error 0x%x/0x%x", - start, - size); - } - } - } - fclose(f); -skip1: - if (!pci_supp_legacy_irq()) - goto skip_irq; - sysfs_path =3D GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domai= n, - pci->bus, pci->dev, pci->func); - f =3D fopen(sysfs_path, "r"); - if (f =3D=3D NULL) { - LOGED(ERROR, domid, "Couldn't open %s", sysfs_path); - goto skip_irq; - } - if ((fscanf(f, "%u", &irq) =3D=3D 1) && irq) { - rc =3D xc_physdev_unmap_pirq(ctx->xch, domid, irq); - if (rc < 0) { - LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=3D%d", irq); - } - rc =3D xc_domain_irq_permission(ctx->xch, domid, irq, 0); - if (rc < 0) { - LOGED(ERROR, domid, "xc_domain_irq_permission irq=3D%d", i= rq); - } - } - fclose(f); } -skip_irq: rc =3D 0; out_fail: pci_remove_detached(egc, prs, rc); /* must be last */ @@ -2226,7 +2167,11 @@ static void pci_remove_detached(libxl__egc *egc, int rc) { STATE_AO_GC(prs->aodev->ao); - int stubdomid =3D 0; + libxl_ctx *ctx =3D libxl__gc_owner(gc); + unsigned int start =3D 0, end =3D 0, flags =3D 0, size =3D 0; + int irq =3D 0, i, stubdomid =3D 0; + const char *sysfs_path; + FILE *f; uint32_t domainid =3D prs->domid; bool isstubdom; =20 @@ -2242,6 +2187,78 @@ static void pci_remove_detached(libxl__egc *egc, if (rc && !prs->force) goto out; =20 + /* Revoke the permissions */ + sysfs_path =3D GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource", + pci->domain, pci->bus, pci->dev, pci->func); + + f =3D fopen(sysfs_path, "r"); + if (f =3D=3D NULL) { + LOGED(ERROR, domid, "Couldn't open %s", sysfs_path); + goto skip_bar; + } + + for (i =3D 0; i < PROC_PCI_NUM_RESOURCES; i++) { + if (fscanf(f, "0x%x 0x%x 0x%x\n", &start, &end, &flags) !=3D 3) + continue; + size =3D end - start + 1; + if (start) { + if (flags & PCI_BAR_IO) { + rc =3D xc_domain_ioport_permission(ctx->xch, domid, start, + size, 0); + if (rc < 0) + LOGED(ERROR, domid, + "xc_domain_ioport_permission error 0x%x/0x%x", + start, + size); + } else { + rc =3D xc_domain_iomem_permission(ctx->xch, domid, + start >> XC_PAGE_SHIFT, + (size + (XC_PAGE_SIZE - 1)= ) >> XC_PAGE_SHIFT, + 0); + if (rc < 0) + LOGED(ERROR, domid, + "xc_domain_iomem_permission error 0x%x/0x%x", + start, + size); + } + } + } + fclose(f); + +skip_bar: + if (!pci_supp_legacy_irq()) + goto skip_legacy_irq; + + sysfs_path =3D GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain, + pci->bus, pci->dev, pci->func); + + f =3D fopen(sysfs_path, "r"); + if (f =3D=3D NULL) { + LOGED(ERROR, domid, "Couldn't open %s", sysfs_path); + goto skip_legacy_irq; + } + + if ((fscanf(f, "%u", &irq) =3D=3D 1) && irq) { + rc =3D xc_physdev_unmap_pirq(ctx->xch, domid, irq); + if (rc < 0) { + /* + * QEMU may have already unmapped the IRQ. So the error + * may be spurious. For now, still print an error message as + * it is not easy to distinguished between valid and + * spurious error. + */ + LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=3D%d", irq); + } + rc =3D xc_domain_irq_permission(ctx->xch, domid, irq, 0); + if (rc < 0) { + LOGED(ERROR, domid, "xc_domain_irq_permission irq=3D%d", irq); + } + } + + fclose(f); + +skip_legacy_irq: + isstubdom =3D libxl_is_stubdom(CTX, domid, &domainid); =20 /* don't do multiple resets while some functions are still passed thro= ugh */ --=20 2.40.1