| 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: <20250428150530.GA670882@bhelgaas>
Date: Mon, 28 Apr 2025 10:05:30 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Chathura Rajapaksha <chathura.abeyrathne.lk@...il.com>
Cc: kvm@...r.kernel.org, Chathura Rajapaksha <chath@...edu>,
William Wang <xwill@...edu>,
Alex Williamson <alex.williamson@...hat.com>,
Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...hat.com>,
Niklas Schnelle <schnelle@...ux.ibm.com>,
Xin Zeng <xin.zeng@...el.com>, Kevin Tian <kevin.tian@...el.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Yahui Cao <yahui.cao@...el.com>, Yunxiang Li <Yunxiang.Li@....com>,
Dongdong Zhang <zhangdongdong@...incomputing.com>,
Avihai Horon <avihaih@...dia.com>, linux-kernel@...r.kernel.org,
audit@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] audit accesses to unassigned PCI config regions
On Sat, Apr 26, 2025 at 09:22:49PM +0000, Chathura Rajapaksha wrote:
> Some PCIe devices trigger PCI bus errors when accesses are made to
> unassigned regions within their PCI configuration space. On certain
> platforms, this can lead to host system hangs or reboots.
>
> The current vfio-pci driver allows guests to access unassigned regions
> in the PCI configuration space. Therefore, when such a device is passed
> through to a guest, the guest can induce a host system hang or reboot
> through crafted configuration space accesses, posing a threat to
> system availability.
>
> This patch introduces auditing support for config space accesses to
> unassigned regions. When enabled, this logs such accesses for all
> passthrough devices.
> This feature is controlled via a new Kconfig option:
Add blank line between paragraphs.
> CONFIG_VFIO_PCI_UNASSIGNED_ACCESS_AUDIT
>
> A new audit event type, AUDIT_VFIO, has been introduced to support
> this, allowing administrators to monitor and investigate suspicious
> behavior by guests.
Use imperative mood ("Introduce" instead of "This patch introduces
..." and "Add ..." instead of "A new type has been introduced").
> Co-developed by: William Wang <xwill@...edu>
> Signed-off-by: William Wang <xwill@...edu>
> Signed-off-by: Chathura Rajapaksha <chath@...edu>
> ---
> drivers/vfio/pci/Kconfig | 12 ++++++++
> drivers/vfio/pci/vfio_pci_config.c | 46 ++++++++++++++++++++++++++++--
> include/uapi/linux/audit.h | 1 +
> 3 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index c3bcb6911c53..7f9f16262b90 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -42,6 +42,18 @@ config VFIO_PCI_IGD
> and LPC bridge config space.
>
> To enable Intel IGD assignment through vfio-pci, say Y.
> +
> +config VFIO_PCI_UNASSIGNED_ACCESS_AUDIT
> + bool "Audit accesses to unassigned PCI configuration regions"
> + depends on AUDIT && VFIO_PCI_CORE
> + help
> + Some PCIe devices are known to cause bus errors when accessing
> + unassigned PCI configuration space, potentially leading to host
> + system hangs on certain platforms. When enabled, this option
> + audits accesses to unassigned PCI configuration regions.
> +
> + If you don't know what to do here, say N.
> +
> endif
>
> config VFIO_PCI_ZDEV_KVM
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index cb4d11aa5598..ddd10904d60f 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -25,6 +25,7 @@
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> #include <linux/slab.h>
> +#include <linux/audit.h>
>
> #include "vfio_pci_priv.h"
>
> @@ -1980,6 +1981,37 @@ static size_t vfio_pci_cap_remaining_dword(struct vfio_pci_core_device *vdev,
> return i;
> }
>
> +enum vfio_audit {
> + VFIO_AUDIT_READ,
> + VFIO_AUDIT_WRITE,
> + VFIO_AUDIT_MAX,
> +};
> +
> +static const char * const vfio_audit_str[VFIO_AUDIT_MAX] = {
> + [VFIO_AUDIT_READ] = "READ",
> + [VFIO_AUDIT_WRITE] = "WRITE",
> +};
> +
> +static void vfio_audit_access(const struct pci_dev *pdev,
> + size_t count, loff_t *ppos, bool blocked, unsigned int op)
> +{
> + struct audit_buffer *ab;
> +
> + if (WARN_ON_ONCE(op >= VFIO_AUDIT_MAX))
> + return;
> + if (audit_enabled == AUDIT_OFF)
> + return;
> + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_VFIO);
> + if (unlikely(!ab))
> + return;
> + audit_log_format(ab,
> + "device=%04x:%02x:%02x.%d access=%s offset=0x%llx size=%ld blocked=%u\n",
> + pci_domain_nr(pdev->bus), pdev->bus->number,
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> + vfio_audit_str[op], *ppos, count, blocked);
> + audit_log_end(ab);
> +}
> +
> static ssize_t vfio_config_do_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> size_t count, loff_t *ppos, bool iswrite)
> {
> @@ -1989,6 +2021,7 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_core_device *vdev, char __user
> int cap_start = 0, offset;
> u8 cap_id;
> ssize_t ret;
> + bool blocked;
>
> if (*ppos < 0 || *ppos >= pdev->cfg_size ||
> *ppos + count > pdev->cfg_size)
> @@ -2011,13 +2044,22 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_core_device *vdev, char __user
> cap_id = vdev->pci_config_map[*ppos];
>
> if (cap_id == PCI_CAP_ID_INVALID) {
> - if (((iswrite && block_pci_unassigned_write) ||
> + blocked = (((iswrite && block_pci_unassigned_write) ||
> (!iswrite && block_pci_unassigned_read)) &&
> - !pci_uaccess_lookup(pdev))
> + !pci_uaccess_lookup(pdev));
> + if (blocked)
> perm = &block_unassigned_perms;
> else
> perm = &unassigned_perms;
> cap_start = *ppos;
> + if (IS_ENABLED(CONFIG_VFIO_PCI_UNASSIGNED_ACCESS_AUDIT)) {
> + if (iswrite)
> + vfio_audit_access(pdev, count, ppos, blocked,
> + VFIO_AUDIT_WRITE);
> + else
> + vfio_audit_access(pdev, count, ppos, blocked,
> + VFIO_AUDIT_READ);
> + }
Simplify this patch by adding "blocked" in the first patch. Then you
won't have to touch the permission checking that is unrelated to the
audit logging. Consider adding a helper to do the checking and return
"blocked" so it doesn't clutter vfio_config_do_rw().
> } else if (cap_id == PCI_CAP_ID_INVALID_VIRT) {
> perm = &virt_perms;
> cap_start = *ppos;
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9a4ecc9f6dc5..c0aace7384f3 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -122,6 +122,7 @@
> #define AUDIT_OPENAT2 1337 /* Record showing openat2 how args */
> #define AUDIT_DM_CTRL 1338 /* Device Mapper target control */
> #define AUDIT_DM_EVENT 1339 /* Device Mapper events */
> +#define AUDIT_VFIO 1340 /* VFIO events */
>
> #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
> --
> 2.34.1
>
Powered by blists - more mailing lists