| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251217150755.10d88a04.alex@shazbot.org>
Date: Wed, 17 Dec 2025 15:07:55 -0700
From: Alex Williamson <alex@...zbot.org>
To: Kevin Tian <kevin.tian@...el.com>
Cc: Ankit Agrawal <ankita@...dia.com>, Jason Gunthorpe <jgg@...pe.ca>,
Yishai Hadas <yishaih@...dia.com>, Shameer Kolothum
<skolothumtho@...dia.com>, Ramesh Thomas <ramesh.thomas@...el.com>,
Yunxiang Li <Yunxiang.Li@....com>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, Farrah Chen <farrah.chen@...el.com>,
stable@...r.kernel.org
Subject: Re: [PATCH] vfio/pci: Disable qword access to the PCI ROM bar
On Fri, 12 Dec 2025 02:09:41 +0000
Kevin Tian <kevin.tian@...el.com> wrote:
> Commit 2b938e3db335 ("vfio/pci: Enable iowrite64 and ioread64 for vfio
> pci") enables qword access to the PCI bar resources. However certain
> devices (e.g. Intel X710) are observed with problem upon qword accesses
> to the rom bar, e.g. triggering PCI aer errors.
>
> Instead of trying to identify all broken devices, universally disable
> qword access to the rom bar i.e. going back to the old way which worked
> reliably for years.
Thanks for finding the root cause of this, Kevin. I think it would add
useful context in the commit log here to describe that the ROM is
somewhat unique in this respect because it's cached by QEMU which
simply does a pread() of the remaining size until it gets the full
contents. The other BARs would only perform operations at the same
access width as their guest drivers.
> Reported-by: Farrah Chen <farrah.chen@...el.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220740
> Fixes: 2b938e3db335 ("vfio/pci: Enable iowrite64 and ioread64 for vfio pci")
> Cc: stable@...r.kernel.org
> Signed-off-by: Kevin Tian <kevin.tian@...el.com>
> ---
> drivers/vfio/pci/nvgrace-gpu/main.c | 4 ++--
> drivers/vfio/pci/vfio_pci_rdwr.c | 19 +++++++++++++++----
> include/linux/vfio_pci_core.h | 2 +-
> 3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c
> index e346392b72f6..9b39184f76b7 100644
> --- a/drivers/vfio/pci/nvgrace-gpu/main.c
> +++ b/drivers/vfio/pci/nvgrace-gpu/main.c
> @@ -491,7 +491,7 @@ nvgrace_gpu_map_and_read(struct nvgrace_gpu_pci_core_device *nvdev,
> ret = vfio_pci_core_do_io_rw(&nvdev->core_device, false,
> nvdev->resmem.ioaddr,
> buf, offset, mem_count,
> - 0, 0, false);
> + 0, 0, false, true);
> }
>
> return ret;
> @@ -609,7 +609,7 @@ nvgrace_gpu_map_and_write(struct nvgrace_gpu_pci_core_device *nvdev,
> ret = vfio_pci_core_do_io_rw(&nvdev->core_device, false,
> nvdev->resmem.ioaddr,
> (char __user *)buf, pos, mem_count,
> - 0, 0, true);
> + 0, 0, true, true);
> }
>
> return ret;
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 6192788c8ba3..95dc7e04cb08 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -135,7 +135,7 @@ VFIO_IORDWR(64)
> ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
> void __iomem *io, char __user *buf,
> loff_t off, size_t count, size_t x_start,
> - size_t x_end, bool iswrite)
> + size_t x_end, bool iswrite, bool allow_qword)
I've been trying to think about how we avoid yet another bool arg here.
What do you think about creating an enum:
enum vfio_pci_io_width {
VFIO_PCI_IO_WIDTH_1 = 1,
VFIO_PCI_IO_WIDTH_2 = 2,
VFIO_PCI_IO_WIDTH_4 = 4,
VFIO_PCI_IO_WIDTH_8 = 8,
};
The arg here would then be enum vfio_pci_io_width max_width, and for
each test we'd add a condition && max_width >= 8 (4, 2), where we can
assume byte access as the minimum regardless of the arg. It's a little
more than we need, but it follows a simple pattern and I think makes
the call sites a bit more intuitive.
> {
> ssize_t done = 0;
> int ret;
> @@ -150,7 +150,7 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
> else
> fillable = 0;
>
> - if (fillable >= 8 && !(off % 8)) {
> + if (allow_qword && fillable >= 8 && !(off % 8)) {
> ret = vfio_pci_iordwr64(vdev, iswrite, test_mem,
> io, buf, off, &filled);
> if (ret)
> @@ -234,6 +234,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> void __iomem *io;
> struct resource *res = &vdev->pdev->resource[bar];
> ssize_t done;
> + bool allow_qword = true;
>
> if (pci_resource_start(pdev, bar))
> end = pci_resource_len(pdev, bar);
> @@ -262,6 +263,16 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> if (!io)
> return -ENOMEM;
> x_end = end;
> +
> + /*
> + * Certain devices (e.g. Intel X710) don't support qword
> + * access to the ROM bar. Otherwise PCI AER errors might be
> + * triggered.
> + *
> + * Disable qword access to the ROM bar universally, which
> + * worked reliably for years before qword access is enabled.
> + */
> + allow_qword = false;
> } else {
> int ret = vfio_pci_core_setup_barmap(vdev, bar);
> if (ret) {
> @@ -278,7 +289,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> }
>
> done = vfio_pci_core_do_io_rw(vdev, res->flags & IORESOURCE_MEM, io, buf, pos,
> - count, x_start, x_end, iswrite);
> + count, x_start, x_end, iswrite, allow_qword);
>
> if (done >= 0)
> *ppos += done;
> @@ -352,7 +363,7 @@ ssize_t vfio_pci_vga_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> * to the memory enable bit in the command register.
> */
> done = vfio_pci_core_do_io_rw(vdev, false, iomem, buf, off, count,
> - 0, 0, iswrite);
> + 0, 0, iswrite, true);
I have no basis other than paranoia and "VGA is old and a 64-bit access
to it seem wrong", but I'm tempted to restrict this to dword access as
well. I don't want to take your fix off track from it's specific goal
though. Thanks,
Alex
>
> vga_put(vdev->pdev, rsrc);
>
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index f541044e42a2..3a75b76eaed3 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -133,7 +133,7 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
> ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
> void __iomem *io, char __user *buf,
> loff_t off, size_t count, size_t x_start,
> - size_t x_end, bool iswrite);
> + size_t x_end, bool iswrite, bool allow_qword);
> bool vfio_pci_core_range_intersect_range(loff_t buf_start, size_t buf_cnt,
> loff_t reg_start, size_t reg_cnt,
> loff_t *buf_offset,
Powered by blists - more mailing lists