[{"id":3687349,"web_url":"http://patchwork.ozlabs.org/comment/3687349/","msgid":"<20260506195749.25379C2BCB0@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-06T19:57:48","subject":"Re: [PATCH v6 5/6] vfio/pci: Add PCIe TPH GET_ST interface","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:\n- [High] The `argsz` validation incorrectly double-counts the size of the union member, causing valid requests to be rejected.\n- [High] Concurrent TPH ioctls lead to data races on unprotected PCIe TPH state variables and device configuration space.\n- [Medium] The unused `index` field in `struct vfio_pci_tph_entry` is not validated to be zero during `VFIO_PCI_TPH_GET_ST`.\n--\n\ncommit 69a04a0a6e943f789b3703368fa523b4efd43755\nAuthor: Chengwen Feng \n\nvfio/pci: Add PCIe TPH GET_ST interface\n\nThis commit adds support to batch get CPU steering tags for device-specific\nTPH mode that does not implement an ST table. This interface requires\nenabling the enable_unsafe_tph_ds_mode module parameter.\n\n> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c\n> index 69f666d20c4af..45e641ab2a887 100644\n> --- a/drivers/vfio/pci/vfio_pci_core.c\n> +++ b/drivers/vfio/pci/vfio_pci_core.c\n[ ... ]\n> @@ -1529,6 +1529,77 @@ static int vfio_pci_tph_disable(struct vfio_pci_core_device *vdev)\n> \treturn 0;\n> }\n> \n> +static int vfio_pci_tph_get_st(struct vfio_pci_core_device *vdev,\n> +\t\t\t struct vfio_device_pci_tph_op *op,\n> +\t\t\t void __user *uarg)\n> +{\n[ ... ]\n> +\tif (!st.count || st.count > VFIO_PCI_TPH_MAX_ENTRIES)\n> +\t\treturn -EINVAL;\n> +\n> +\tsize = st.count * sizeof(*ents);\n> +\tif (op->argsz < offsetofend(struct vfio_device_pci_tph_op, st) +\n> +\t\t\tsizeof(struct vfio_pci_tph_st) + size)\n> +\t\treturn -EINVAL;\n\nIs it possible the size calculation double-counts\nsizeof(struct vfio_pci_tph_st)?\n\nThe macro offsetofend(TYPE, MEMBER) evaluates to offsetof(TYPE, MEMBER) +\nsizeof_field(TYPE, MEMBER). For the st member, offsetofend() correctly\ncalculates the end boundary. Adding sizeof(struct vfio_pci_tph_st) a\nsecond time makes the expected minimum argsz larger than it should be.\n\nIf userspace passes the properly calculated size, will the kernel\nincorrectly evaluate this as false and reject valid ioctl calls with -EINVAL?\n\n(Note: The identical calculation error appears to be present in\nvfio_pci_tph_enable() introduced in a prior patch.)\n\n> +\n> +\tents = kvmalloc(size, GFP_KERNEL);\n> +\tif (!ents)\n> +\t\treturn -ENOMEM;\n> +\n> +\tents_off = offsetof(struct vfio_pci_tph_st, ents);\n> +\tif (copy_from_user(ents, uarg + ents_off, size)) {\n> +\t\terr = -EFAULT;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tfor (i = 0; i < st.count; i++) {\n> +\t\t/* Check reserved fields are zero */\n> +\t\tif (memchr_inv(&ents[i].reserved0, 0, sizeof(ents[i].reserved0)) ||\n> +\t\t memchr_inv(&ents[i].reserved1, 0, sizeof(ents[i].reserved1))) {\n> +\t\t\terr = -EINVAL;\n> +\t\t\tgoto out;\n> +\t\t}\n\nDoes this code need to validate that the unused index field in\nstruct vfio_pci_tph_entry is zero?\n\nWhile reserved0 and reserved1 are checked, the index field appears to\nbe ignored during GET_ST operations. Because userspace is not required to\nzero-fill unused arguments, this field may contain uninitialized garbage.\n\nIf a future kernel extension assigns meaning to index for GET_ST, will\nthis lack of validation break backwards compatibility by making it\nimpossible to distinguish intentional new values from uninitialized data in\nolder applications?\n\n[ ... ]\n> static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,\n> \t\t\t void __user *uarg)\n> {\n> @@ -1545,6 +1616,8 @@ static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,\n> \t\treturn vfio_pci_tph_enable(vdev, &op, uarg + minsz);\n> \tcase VFIO_PCI_TPH_DISABLE:\n> \t\treturn vfio_pci_tph_disable(vdev);\n> +\tcase VFIO_PCI_TPH_GET_ST:\n> +\t\treturn vfio_pci_tph_get_st(vdev, &op, uarg + minsz);\n> \tdefault:\n> \t\t/* Other ops are not implemented yet */\n> \t\treturn -EINVAL;\n\nAre concurrent TPH ioctls adequately serialized here?\n\nThe vfio_pci_ioctl_tph() handler processes requests without acquiring any\nserialization locks (such as vdev->igate). If userspace issues multiple\nTPH ioctls concurrently, vfio_pci_tph_enable(), vfio_pci_tph_disable(),\nand vfio_pci_tph_get_st() can execute in parallel.\n\nThe underlying PCI core functions pcie_enable_tph() and pcie_disable_tph()\nperform lockless read-modify-write operations on the PCI_TPH_CTRL config\nregister and mutate pdev->tph_mode, pdev->tph_req_type, and\npdev->tph_enabled. Simultaneously, vfio_pci_tph_get_st() invokes\npcie_tph_get_cpu_st(), which relies on these state variables.\n\nCan this lead to data races on unprotected PCIe TPH state variables,\nresulting in corrupted device configuration space or inconsistent state\nreads?","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=o78CvqPF;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.234.253.10; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-53942-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"o78CvqPF\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org [172.234.253.10])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g9mQ81sQmz1yJV\n\tfor ; Thu, 07 May 2026 05:57:52 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 385473009F8E\n\tfor ; Wed, 6 May 2026 19:57:50 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 7A63434AB03;\n\tWed, 6 May 2026 19:57:49 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 5713E33E35F\n\tfor ; Wed, 6 May 2026 19:57:49 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 25379C2BCB0;\n\tWed, 6 May 2026 19:57:49 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778097469; cv=none;\n b=Jv+6XLcjIunwuBi2pr7iuCnNSlbkkd1ton+JwJIpoaL76B1A9ZWnMH6XY0MBbn9SdNPwryJC1SvMZnr3Wos7OCA4+Ck9MLM9g0Vpi5ddnPhblbyKOjK6+BTAdhJjnDTpLUiyBFbRzvFOoHXWYXv3k4KlGvnpHmZP3btQRr8/oe4=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778097469; c=relaxed/simple;\n\tbh=d0e9gqoyXvUGYMxgStyL5E55xLpSj2jNvNc1sTlxOjw=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=TcvSniKxWB6Dx9K1SHMFF6jnFXux2RL5fXH655gu06ggC8P7IbJW/juio7UWLCy7J9SVSV0ntnbFrzZ1Cmrs8hDEdiSl7MRXC7fZK7v4c+wzHTEQXDaBaI9YWsZZmgrAy34NT/DIMoJuKj8siduztxH4N7VH7Yco6qVid4z1lXc=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=o78CvqPF; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778097469;\n\tbh=d0e9gqoyXvUGYMxgStyL5E55xLpSj2jNvNc1sTlxOjw=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=o78CvqPFweAcQtwZVLFxuXZ9k1A+HouDonL99onUwoNwYKmyxo38lnArRBuXg3YRh\n\t SeC08zXYJmaGSy5XV67X7w8szbiHeyXvaFhk7WEWpAWN2LqJDDr+ivk/IV00p+4Vh0\n\t oWF33BL/0e5CM6Bx178XQ/p3pGSLX+abW/44ahTd/ATmS8+PfBrTeOEvzb94zWwlW8\n\t dTSY2+UOQ7H6FKy7ujvN/XwzH7KXt6qLy9Ov+9yuIVWuJeiwozPeOipjbIHzK7COto\n\t logqT8zLj8H2+etXA/FFMOnPw7ZbrAf93+AyosUCGJuzz/NjX39iGuEZw2saiM5dHG\n\t vK4MYVMBbva1w==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH v6 5/6] vfio/pci: Add PCIe TPH GET_ST interface","Reply-To":"sashiko@lists.linux.dev","To":"\"Chengwen Feng\" ","Cc":"linux-pci@vger.kernel.org","In-Reply-To":"<20260506094623.29327-6-fengchengwen@huawei.com>","References":"<20260506094623.29327-6-fengchengwen@huawei.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Wed, 06 May 2026 19:57:48 +0000","Message-Id":"<20260506195749.25379C2BCB0@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":""}}]